| Message ID | 20210119162204.2081137-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | capability conversion fixes | expand |
Miklos Szeredi <mszeredi@redhat.com> writes: > It turns out overlayfs is actually okay wrt. mutliple conversions, because > it uses the right context for lower operations. I.e. before calling > vfs_{set,get}xattr() on underlying fs, it overrides creds with that of the > mounter, so the current user ns will now match that of > overlay_sb->s_user_ns, meaning that the caps will be converted to just the > right format for the next layer > > OTOH ecryptfs, which is the only other one affected by commit 7c03e2cda4a5 > ("vfs: move cap_convert_nscap() call into vfs_setxattr()") needs to be > fixed up, since it doesn't do the cap override thing that overlayfs does. > > I don't have an ecryptfs setup, so untested, but it's a fairly trivial > change. > > My other observation was that cap_inode_getsecurity() messes up conversion > of caps in more than one case. This is independent of the overlayfs user > ns enablement but affects it as well. > > Maybe we can revisit the infrastructure improvements we discussed, but I > think these fixes are more appropriate for the current cycle. I mostly agree. Fixing the bugs in a back-portable way is important. However we need to sort out the infrastructure, and implementation. As far as I can tell it is only the fact that overlayfs does not support the new mount api aka fs_context that allows this fix to work and be correct. I believe the new mount api would allow specifying a different userns thatn curent_user_ns for the overlay filesystem and that would break this. So while I agree with the making a minimal fix for now. We need a good fix because this code is much too subtle, and it can break very easily with no one noticing. Eric > Thanks, > Miklos > > Miklos Szeredi (2): > ecryptfs: fix uid translation for setxattr on security.capability > security.capability: fix conversions on getxattr > > fs/ecryptfs/inode.c | 10 +++++-- > security/commoncap.c | 67 ++++++++++++++++++++++++++++---------------- > 2 files changed, 50 insertions(+), 27 deletions(-)
On Tue, Jan 19, 2021 at 10:15 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Miklos Szeredi <mszeredi@redhat.com> writes: > > > It turns out overlayfs is actually okay wrt. mutliple conversions, because > > it uses the right context for lower operations. I.e. before calling > > vfs_{set,get}xattr() on underlying fs, it overrides creds with that of the > > mounter, so the current user ns will now match that of > > overlay_sb->s_user_ns, meaning that the caps will be converted to just the > > right format for the next layer > > > > OTOH ecryptfs, which is the only other one affected by commit 7c03e2cda4a5 > > ("vfs: move cap_convert_nscap() call into vfs_setxattr()") needs to be > > fixed up, since it doesn't do the cap override thing that overlayfs does. > > > > I don't have an ecryptfs setup, so untested, but it's a fairly trivial > > change. > > > > My other observation was that cap_inode_getsecurity() messes up conversion > > of caps in more than one case. This is independent of the overlayfs user > > ns enablement but affects it as well. > > > > Maybe we can revisit the infrastructure improvements we discussed, but I > > think these fixes are more appropriate for the current cycle. > > I mostly agree. Fixing the bugs in a back-portable way is important. > > However we need to sort out the infrastructure, and implementation. > > As far as I can tell it is only the fact that overlayfs does not support > the new mount api aka fs_context that allows this fix to work and be > correct. > > I believe the new mount api would allow specifying a different userns > thatn curent_user_ns for the overlay filesystem and that would break > this. This is a valid concern. I'll add a WARN_ON() to make sure that whenever this changes it doesn't go unnoticed. Fixing it would also be easy: just update creds->user_ns field to that of sb->s_user_ns in ovl_fill_super(). For now I'll go with the WARNING though, since this cannot be tested. Thanks, Miklos
It turns out overlayfs is actually okay wrt. mutliple conversions, because it uses the right context for lower operations. I.e. before calling vfs_{set,get}xattr() on underlying fs, it overrides creds with that of the mounter, so the current user ns will now match that of overlay_sb->s_user_ns, meaning that the caps will be converted to just the right format for the next layer OTOH ecryptfs, which is the only other one affected by commit 7c03e2cda4a5 ("vfs: move cap_convert_nscap() call into vfs_setxattr()") needs to be fixed up, since it doesn't do the cap override thing that overlayfs does. I don't have an ecryptfs setup, so untested, but it's a fairly trivial change. My other observation was that cap_inode_getsecurity() messes up conversion of caps in more than one case. This is independent of the overlayfs user ns enablement but affects it as well. Maybe we can revisit the infrastructure improvements we discussed, but I think these fixes are more appropriate for the current cycle. Thanks, Miklos Miklos Szeredi (2): ecryptfs: fix uid translation for setxattr on security.capability security.capability: fix conversions on getxattr fs/ecryptfs/inode.c | 10 +++++-- security/commoncap.c | 67 ++++++++++++++++++++++++++++---------------- 2 files changed, 50 insertions(+), 27 deletions(-)