Miklos Szeredi <mszeredi@xxxxxxxxxx> 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(-)
