Re: [PATCH v19 0/4] overlayfs override_creds=off & nested get xattr fix

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Dec 3, 2021 at 5:38 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Nov 17, 2021 at 09:36:42AM +0200, Amir Goldstein wrote:
> > On Wed, Nov 17, 2021 at 3:58 AM David Anderson <dvander@google.com> wrote:
> > >
> > > Mark Salyzyn (3):
> > >   Add flags option to get xattr method paired to __vfs_getxattr
> > >   overlayfs: handle XATTR_NOSECURITY flag for get xattr method
> > >   overlayfs: override_creds=off option bypass creator_cred
> > >
> > > Mark Salyzyn + John Stultz (1):
> > >   overlayfs: inode_owner_or_capable called during execv
> > >
> > > The first three patches address fundamental security issues that should
> > > be solved regardless of the override_creds=off feature.
> > >
> > > The fourth adds the feature depends on these other fixes.
> > >
> > > By default, all access to the upper, lower and work directories is the
> > > recorded mounter's MAC and DAC credentials.  The incoming accesses are
> > > checked against the caller's credentials.
> > >
> > > If the principles of least privilege are applied for sepolicy, the
> > > mounter's credentials might not overlap the credentials of the caller's
> > > when accessing the overlayfs filesystem.  For example, a file that a
> > > lower DAC privileged caller can execute, is MAC denied to the
> > > generally higher DAC privileged mounter, to prevent an attack vector.
> > >
> > > We add the option to turn off override_creds in the mount options; all
> > > subsequent operations after mount on the filesystem will be only the
> > > caller's credentials.  The module boolean parameter and mount option
> > > override_creds is also added as a presence check for this "feature",
> > > existence of /sys/module/overlay/parameters/overlay_creds
> > >
> > > Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> > > Signed-off-by: David Anderson <dvander@google.com>
> > > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > > Cc: Jonathan Corbet <corbet@lwn.net>
> > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > > Cc: Amir Goldstein <amir73il@gmail.com>
> > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > Cc: Stephen Smalley <sds@tycho.nsa.gov>
> > > Cc: John Stultz <john.stultz@linaro.org>
> > > Cc: linux-doc@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Cc: linux-unionfs@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org
> > > Cc: kernel-team@android.com
> > > Cc: selinux@vger.kernel.org
> > > Cc: paulmoore@microsoft.com
> > > Cc: Luca.Boccassi@microsoft.com
> > >
> > > ---
> > >
> > > v19
> > > - rebase.
> > >
> >
> > Hi David,
> >
> > I see that the patch set has changed hands (presumably to Android upstreaming
> > team), but you just rebased v18 without addressing the maintainers concerns [1].
> >
>
> BTW, where is patch 1 of the series. I can't seem to find it.
>
> I think I was running into issues with getxattr() on underlying filesystem
> as well (if mounter did not have sufficient privileges) and tried to fix
> it. But did not find a good solution at that point of time.
>
> https://lore.kernel.org/linux-unionfs/1467733854-6314-6-git-send-email-vgoyal@redhat.com/
>
> So basically when overlay inode is being initialized, code will try to
> query "security.selinux" xattr on underlying file to initialize selinux
> label on the overlay inode. For regular filesystems, they bypass the
> security check by calling __vfs_getxattr() when trying to initialize
> this selinux security label. But with layered filesystem, it still
> ends up calling vfs_getxattr() on underlying filesyste. Which means
> it checks for caller's creds and if caller is not priviliged enough,
> access will be denied.
>
> To solve this problem, looks like this patch set is passing a flag
> XATTR_NOSECUROTY so that permission checks are skipped in getxattr()
> path in underlying filesystem. As long as this information is
> not leaked to user space (and remains in overlayfs), it probably is
> fine? And if information is not going to user space, then it probably
> is fine for unprivileged overlayfs mounts as well?
>
> I see a comment from Miklos as well as you that it is not safe to
> do for unprivileged mounts. Can you help me understand why that's
> the case.
>
>
> > Specifically, the patch 2/4 is very wrong for unprivileged mount and
>
> Can you help me understand why it is wrong. (/me should spend more
> time reading the patch. But I am taking easy route of asking you. :-)).
>

I should have spent more time reading the patch too :-)
I was not referring to the selinux part. That looks fine I guess.

I was referring to the part of:
"Check impure, opaque, origin & meta xattr with no sepolicy audit
(using __vfs_getxattr) since these operations are internal to
overlayfs operations and do not disclose any data."
I don't know how safe that really is to ignore the security checks
for reading trusted xattr and allow non-privileged mounts to do that.
Certainly since non privileged mounts are likely to use userxattr
anyway, so what's the reason to bypass security?

> > I think that the very noisy patch 1/4 could be completely avoided:
>
> How can it completely avoided. If mounter is not privileged then
> vfs_getxattr() on underlying filesystem will fail. Or if
> override_creds=off, then caller might not be privileged enough to
> do getxattr() but we still should be able to initialize overlay
> inode security label.
>

My bad. I didn't read the description of the selinux problem
with the re-post and forgot about it.

> > Can't you use -o userxattr mount option
>
> user xattrs done't work for device nodes and symlinks.
>
> BTW, how will userxattr solve the problem completely. It can be used
> to store overlay specific xattrs but accessing security xattrs on
> underlying filesystem will still be a problem?

It cannot.
As long as the patch sticks with passing through the
getxattr flags, it looks fine to me.
passing security for trusted.overlay seems dodgy.

Thanks,
Amir.