On Fri, Dec 3, 2021 at 5:38 PM Vivek Goyal <vgoyal@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxx> > > > Signed-off-by: David Anderson <dvander@xxxxxxxxxx> > > > Cc: Miklos Szeredi <miklos@xxxxxxxxxx> > > > Cc: Jonathan Corbet <corbet@xxxxxxx> > > > Cc: Vivek Goyal <vgoyal@xxxxxxxxxx> > > > Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx> > > > Cc: Amir Goldstein <amir73il@xxxxxxxxx> > > > Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> > > > Cc: Stephen Smalley <sds@xxxxxxxxxxxxx> > > > Cc: John Stultz <john.stultz@xxxxxxxxxx> > > > Cc: linux-doc@xxxxxxxxxxxxxxx > > > Cc: linux-kernel@xxxxxxxxxxxxxxx > > > Cc: linux-fsdevel@xxxxxxxxxxxxxxx > > > Cc: linux-unionfs@xxxxxxxxxxxxxxx > > > Cc: linux-security-module@xxxxxxxxxxxxxxx > > > Cc: kernel-team@xxxxxxxxxxx > > > Cc: selinux@xxxxxxxxxxxxxxx > > > Cc: paulmoore@xxxxxxxxxxxxx > > > Cc: Luca.Boccassi@xxxxxxxxxxxxx > > > > > > --- > > > > > > 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@xxxxxxxxxx/ > > 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.
