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

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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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.

v18
- rebase + fix minor cut and paste error for inode argument in __vfs_getxattr

v17
- correct some zero-day build failures.
- fix up documentation

v16
- rebase and merge of two patches.
- add adjustment to deal with execv when overrides is off.

v15
- Revert back to v4 with fixes from on the way from v5-v14. The single
  structure argument passing to address the complaints about too many
  arguments was rejected by the community.
- Drop the udner discussion fix for an additional CAP_DAC_READ_SEARCH
  check. Can address that independently.
- ToDo: upstream test frame for thes security fixes (currently testing
  is all in Android).

v14:
- Rejoin, rebase and a few adjustments.

v13:
- Pull out first patch and try to get it in alone feedback, some
  Acks, and then <crickets> because people forgot why we were doing i.

v12:
- Restore squished out patch 2 and 3 in the series,
  then change algorithm to add flags argument.
  Per-thread flag is a large security surface.

v11:
- Squish out v10 introduced patch 2 and 3 in the series,
  then and use per-thread flag instead for nesting.
- Switch name to ovl_do_vds_getxattr for __vds_getxattr wrapper.
- Add sb argument to ovl_revert_creds to match future work.

v10:
- Return NULL on CAP_DAC_READ_SEARCH
- Add __get xattr method to solve sepolicy logging issue
- Drop unnecessary sys_admin sepolicy checking for administrative
  driver internal xattr functions.

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.

v5:
- beefed up the caveats in the Documentation
- Is dependent on
  "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
  "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v4:
- spelling and grammar errors in text

v3:
- Change name from caller_credentials / creator_credentials to the
  boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

David Anderson (4):
  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
  overlayfs: inode_owner_or_capable called during execv

 Documentation/filesystems/locking.rst   |  2 +-
 Documentation/filesystems/overlayfs.rst | 26 ++++++++++++++-
 fs/9p/acl.c                             |  3 +-
 fs/9p/xattr.c                           |  3 +-
 fs/afs/xattr.c                          | 10 +++---
 fs/attr.c                               |  2 +-
 fs/btrfs/xattr.c                        |  3 +-
 fs/ceph/xattr.c                         |  3 +-
 fs/cifs/xattr.c                         |  2 +-
 fs/ecryptfs/inode.c                     |  6 ++--
 fs/ecryptfs/mmap.c                      |  5 +--
 fs/erofs/xattr.c                        |  3 +-
 fs/ext2/xattr_security.c                |  2 +-
 fs/ext2/xattr_trusted.c                 |  2 +-
 fs/ext2/xattr_user.c                    |  2 +-
 fs/ext4/xattr_hurd.c                    |  2 +-
 fs/ext4/xattr_security.c                |  2 +-
 fs/ext4/xattr_trusted.c                 |  2 +-
 fs/ext4/xattr_user.c                    |  2 +-
 fs/f2fs/xattr.c                         |  4 +--
 fs/fuse/xattr.c                         |  4 +--
 fs/gfs2/xattr.c                         |  3 +-
 fs/hfs/attr.c                           |  2 +-
 fs/hfsplus/xattr.c                      |  3 +-
 fs/hfsplus/xattr_security.c             |  3 +-
 fs/hfsplus/xattr_trusted.c              |  3 +-
 fs/hfsplus/xattr_user.c                 |  3 +-
 fs/inode.c                              |  7 +++--
 fs/internal.h                           |  3 +-
 fs/jffs2/security.c                     |  3 +-
 fs/jffs2/xattr_trusted.c                |  3 +-
 fs/jffs2/xattr_user.c                   |  3 +-
 fs/jfs/xattr.c                          |  5 +--
 fs/kernfs/inode.c                       |  3 +-
 fs/nfs/nfs4proc.c                       |  9 ++++--
 fs/ntfs3/xattr.c                        |  2 +-
 fs/ocfs2/xattr.c                        |  9 ++++--
 fs/open.c                               |  2 +-
 fs/orangefs/xattr.c                     |  3 +-
 fs/overlayfs/copy_up.c                  |  2 +-
 fs/overlayfs/dir.c                      | 17 +++++-----
 fs/overlayfs/file.c                     | 25 ++++++++-------
 fs/overlayfs/inode.c                    | 29 ++++++++---------
 fs/overlayfs/namei.c                    |  6 ++--
 fs/overlayfs/overlayfs.h                |  7 +++--
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/readdir.c                  |  8 ++---
 fs/overlayfs/super.c                    | 34 ++++++++++++++++----
 fs/overlayfs/util.c                     | 13 ++++++--
 fs/posix_acl.c                          |  2 +-
 fs/reiserfs/xattr_security.c            |  3 +-
 fs/reiserfs/xattr_trusted.c             |  3 +-
 fs/reiserfs/xattr_user.c                |  3 +-
 fs/squashfs/xattr.c                     |  2 +-
 fs/ubifs/xattr.c                        |  3 +-
 fs/xattr.c                              | 42 +++++++++++++------------
 fs/xfs/xfs_xattr.c                      |  3 +-
 include/linux/lsm_hook_defs.h           |  3 +-
 include/linux/security.h                |  6 ++--
 include/linux/xattr.h                   |  6 ++--
 include/uapi/linux/xattr.h              |  7 +++--
 mm/shmem.c                              |  3 +-
 net/socket.c                            |  3 +-
 security/commoncap.c                    | 11 ++++---
 security/integrity/evm/evm_main.c       | 13 +++++---
 security/security.c                     |  5 +--
 security/selinux/hooks.c                | 19 ++++++-----
 security/smack/smack_lsm.c              | 18 ++++++-----
 68 files changed, 289 insertions(+), 167 deletions(-)

-- 
2.34.0.rc1.387.gb447b232ab-goog

Because of the overlayfs getxattr recursion, the incoming inode fails
to update the selinux sid resulting in avc denials being reported
against a target context of u:object_r:unlabeled:s0.

Solution is to respond to the XATTR_NOSECURITY flag in get xattr
method that calls the __vfs_getxattr handler instead so that the
context can be read in, rather than being denied with an -EACCES
when vfs_getxattr handler is called.

For the use case where access is to be blocked by the security layer.

The path then would be security(dentry) ->
__vfs_getxattr({dentry...XATTR_NOSECURITY}) ->
handler->get({dentry...XATTR_NOSECURITY}) ->
__vfs_getxattr({realdentry...XATTR_NOSECURITY}) ->
lower_handler->get({realdentry...XATTR_NOSECURITY}) which
would report back through the chain data and success as expected,
the logging security layer at the top would have the data to
determine the access permissions and report back to the logs and
the caller that the target context was blocked.

For selinux this would solve the cosmetic issue of the selinux log
and allow audit2allow to correctly report the rule needed to address
the access problem.

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.  This became
an issue for credential override off since sys_admin would have
been required by the caller; whereas would have been inherently
present for the creator since it performed the mount.

This is a change in operations since we do not check in the new
ovl_do_getxattr function if the credential override is off or not.
Reasoning is that the sepolicy check is unnecessary overhead,
especially since the check can be expensive.

Because for override credentials off, this affects _everyone_ that
underneath performs private xattr calls without the appropriate
sepolicy permissions and sys_admin capability.  Providing blanket
support for sys_admin would be bad for all possible callers.

For the override credentials on, this will affect only the mounter,
should it lack sepolicy permissions. Not considered a security
problem since mounting by definition has sys_admin capabilities,
but sepolicy contexts would still need to be crafted.

It should be noted that there is precedence, __vfs_getxattr is used
in other filesystems for their own internal trusted xattr management.

Signed-off-by: Mark Salyzyn <redacted>
Signed-off-by: David Anderson <redacted>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-unionfs@vger.kernel.org
Cc: Stephen Smalley <redacted>
Cc: linux-kernel@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-team@android.com
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Stephen Smalley <redacted>
Cc: linux-doc@vger.kernel.org
Cc: selinux@vger.kernel.org
Cc: paulmoore@microsoft.com
Cc: Luca.Boccassi@microsoft.com

v19 - rebase

v18 - correct inode argument to __vfs_getxattr

v17 - rebase and add inode argument to __vfs_getxattr

v16 - rebase and merge internal getxattr operations patch

v15 - revert to v13 because xattr_gs_args rejected.

v14 - rebase to use xattr_gs_args.

v13 - rebase to use __vfs_getxattr flags option.

v12 - Added back to patch series as get xattr with flag option.

v11 - Squashed out of patch series and replaced with per-thread flag
      solution.

v10 - Added to patch series as __get xattr method.
---
 fs/overlayfs/inode.c     | 5 +++--
 fs/overlayfs/overlayfs.h | 6 ++++--
 fs/overlayfs/super.c     | 4 ++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 1f36158c7dbe..49bfa33bb682 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c

@@ -386,7 +386,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 }
 
 int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
-		  void *value, size_t size)
+		  void *value, size_t size, int flags)
 {
 	ssize_t res;
 	const struct cred *old_cred;

@@ -394,7 +394,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 		ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-	res = vfs_getxattr(&init_user_ns, realdentry, name, value, size);
+	res = __vfs_getxattr(&init_user_ns, realdentry, d_inode(realdentry),
+			     name, value, size, flags);
 	revert_creds(old_cred);
 	return res;
 }

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 2cd5741c873b..3fcd62e72aad 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h

@@ -187,7 +187,9 @@ static inline ssize_t ovl_do_getxattr(struct ovl_fs *ofs, struct dentry *dentry,
 				      size_t size)
 {
 	const char *name = ovl_xattr(ofs, ox);
-	int err = vfs_getxattr(&init_user_ns, dentry, name, value, size);
+	struct inode *ip = d_inode(dentry);
+	int err = __vfs_getxattr(&init_user_ns, dentry, ip, name, value, size,
+				 XATTR_NOSECURITY);
 	int len = (value && err > 0) ? err : 0;
 
 	pr_debug("getxattr(%pd2, \"%s\", \"%*pE\", %zu, 0) = %i\n",

@@ -496,7 +498,7 @@ int ovl_permission(struct user_namespace *mnt_userns, struct inode *inode,
 int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 		  const void *value, size_t size, int flags);
 int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
-		  void *value, size_t size);
+		  void *value, size_t size, int flags);
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
 struct posix_acl *ovl_get_acl(struct inode *inode, int type, bool rcu);
 int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 359aa5772cb7..973644af1288 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c

@@ -1006,7 +1006,7 @@ ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
 			struct dentry *dentry, struct inode *inode,
 			const char *name, void *buffer, size_t size, int flags)
 {
-	return ovl_xattr_get(dentry, inode, handler->name, buffer, size);
+	return ovl_xattr_get(dentry, inode, handler->name, buffer, size, flags);
 }
 
 static int __maybe_unused

@@ -1087,7 +1087,7 @@ static int ovl_other_xattr_get(const struct xattr_handler *handler,
 			       const char *name, void *buffer, size_t size,
 			       int flags)
 {
-	return ovl_xattr_get(dentry, inode, name, buffer, size);
+	return ovl_xattr_get(dentry, inode, name, buffer, size, flags);
 }
 
 static int ovl_other_xattr_set(const struct xattr_handler *handler,

-- 
2.34.0.rc1.387.gb447b232ab-goog

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, 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/override_creds.

It was not always this way.  Circa 4.6 there was no recorded mounter's
credentials, instead privileged access to upper or work directories
were temporarily increased to perform the operations.  The MAC
(selinux) policies were caller's in all cases.  override_creds=off
partially returns us to this older access model minus the insecure
temporary credential increases.  This is to permit use in a system
with non-overlapping security models for each executable including
the agent that mounts the overlayfs filesystem.  In Android
this is the case since init, which performs the mount operations,
has a minimal MAC set of privileges to reduce any attack surface,
and services that use the content have a different set of MAC
privileges (eg: read, for vendor labelled configuration, execute for
vendor libraries and modules).  The caveats are not a problem in
the Android usage model, however they should be fixed for
completeness and for general use in time.

Signed-off-by: Mark Salyzyn <redacted>
Signed-off-by: David Anderson <redacted>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-unionfs@vger.kernel.org
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: linux-security-module@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: selinux@vger.kernel.org
Cc: paulmoore@microsoft.com
Cc: Luca.Boccassi@microsoft.com

v19
- rebase

v18
- rebase

v17
- move credential section for override_creds=off as a level 3 heading
  subsection of the permissions section.

v16
- Rebase, cover a few more new ovl_revert_creds callpoints.

v15
- Rebase

v14:
- fix an issue in ovl_create_or_link which leaks credentials.

v12 + v13
- Rebase

v11:
- add sb argument to ovl_revert_creds to match future work in progress
  in other commiter's hands.

v10:
- Rebase (and expand because of increased revert_cred usage)

v9:
- Add to the caveats

v8:
- drop pr_warn message after straw poll to remove it.
- added a use case in the commit message

v7:
- change name of internal parameter to ovl_override_creds_def
- report override_creds only if different than default

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation.
- pr_warn message adjusted to report consequences.

v5:
- beefed up the caveats in the Documentation
- Is dependent on
  "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
  "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v4:
- spelling and grammar errors in text

v3:
- Change name from caller_credentials / creator_credentials to the
  boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.
---
 Documentation/filesystems/overlayfs.rst | 26 ++++++++++++++++++++++++-
 fs/overlayfs/copy_up.c                  |  2 +-
 fs/overlayfs/dir.c                      | 17 +++++++++-------
 fs/overlayfs/file.c                     | 22 ++++++++++-----------
 fs/overlayfs/inode.c                    | 24 +++++++++++------------
 fs/overlayfs/namei.c                    |  6 +++---
 fs/overlayfs/overlayfs.h                |  1 +
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/readdir.c                  |  8 ++++----
 fs/overlayfs/super.c                    | 22 ++++++++++++++++++++-
 fs/overlayfs/util.c                     | 13 +++++++++++--
 11 files changed, 100 insertions(+), 42 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 7da6c30ed596..d7cd4032134a 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst

@@ -195,7 +195,7 @@ handle it in two different ways:
 
 1. return EXDEV error: this error is returned by rename(2) when trying to
    move a file or directory across filesystem boundaries.  Hence
-   applications are usually prepared to hande this error (mv(1) for example
+   applications are usually prepared to handle this error (mv(1) for example
    recursively copies the directory tree).  This is the default behavior.
 
 2. If the "redirect_dir" feature is enabled, then the directory will be

@@ -324,6 +324,30 @@ and
 The resulting access permissions should be the same.  The difference is in
 the time of copy (on-demand vs. up-front).
 
+### Non overlapping credentials
+
+As noted above, 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.
+
+In the case where caller MAC or DAC credentials do not overlap the mounter, a
+use case available in older versions of the driver, the override_creds mount
+flag can be turned off.  For when the use pattern has caller with legitimate
+credentials where the mounter does not.  For example init may have been the
+mounter, but the caller would have execute or read MAC permissions where
+init would not.  override_creds off means all access, incoming, upper, lower
+or working, will be tested against the caller.
+
+Several unintended side effects will occur though.  The caller without certain
+key capabilities or lower privilege will not always be able to delete files or
+directories, create nodes, or search some restricted directories.  The ability
+to search and read a directory entry is spotty as a result of the cache
+mechanism not re-testing the credentials because of the assumption, a
+privileged caller can fill cache, then a lower privilege can read the directory
+cache.  The uneven security model where cache, upperdir and workdir are opened
+at privilege, but accessed without creating a form of privilege escalation,
+should only be used with strict understanding of the side effects and of the
+security policies.
 
 Multiple lower layers
 ---------------------

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b193d08a3dc3..de7fdcb3eb6c 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c

@@ -1032,7 +1032,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		dput(parent);
 		dput(next);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 	return err;
 }

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index f18490813170..7449825b6cbd 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c

@@ -569,7 +569,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			      struct ovl_cattr *attr, bool origin)
 {
 	int err;
-	const struct cred *old_cred;
+	const struct cred *old_cred, *hold_cred = NULL;
 	struct cred *override_cred;
 	struct dentry *parent = dentry->d_parent;
 

@@ -596,14 +596,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		override_cred->fsgid = inode->i_gid;
 		if (!attr->hardlink) {
 			err = security_dentry_create_files_as(dentry,
-					attr->mode, &dentry->d_name, old_cred,
+					attr->mode, &dentry->d_name,
+					old_cred ? old_cred : current_cred(),
 					override_cred);
 			if (err) {
 				put_cred(override_cred);
 				goto out_revert_creds;
 			}
 		}
-		put_cred(override_creds(override_cred));
+		hold_cred = override_creds(override_cred);
 		put_cred(override_cred);
 
 		if (!ovl_dentry_is_whiteout(dentry))

@@ -612,7 +613,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			err = ovl_create_over_whiteout(dentry, inode, attr);
 	}
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred ?: hold_cred);
+	if (old_cred && hold_cred)
+		put_cred(hold_cred);
 	return err;
 }
 

@@ -689,7 +692,7 @@ static int ovl_set_link_redirect(struct dentry *dentry)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_set_redirect(dentry, false);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 	return err;
 }

@@ -908,7 +911,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 		err = ovl_remove_upper(dentry, is_dir, &list);
 	else
 		err = ovl_remove_and_whiteout(dentry, &list);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	if (!err) {
 		if (is_dir)
 			clear_nlink(dentry->d_inode);

@@ -1283,7 +1286,7 @@ static int ovl_rename(struct user_namespace *mnt_userns, struct inode *olddir,
 out_unlock:
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old->d_sb, old_cred);
 	if (update_nlink)
 		ovl_nlink_end(new);
 out_drop_write:

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index fa125feed0ff..11d8277c94cd 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c

@@ -61,7 +61,7 @@ static struct file *ovl_open_realfile(const struct file *file,
 		realfile = open_with_fake_path(&file->f_path, flags, realinode,
 					       current_cred());
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
 		 file, file, ovl_whatisit(inode, realinode), file->f_flags,

@@ -205,7 +205,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	ret = vfs_llseek(real.file, offset, whence);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	file->f_pos = real.file->f_pos;
 	ovl_inode_unlock(inode);

@@ -334,7 +334,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			ovl_aio_cleanup_handler(aio_req);
 	}
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 	ovl_file_accessed(file);
 out_fdput:
 	fdput(real);

@@ -407,7 +407,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 			ovl_aio_cleanup_handler(aio_req);
 	}
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 out_fdput:
 	fdput(real);
 

@@ -453,7 +453,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 	file_end_write(real.file);
 	/* Update size */
 	ovl_copyattr(realinode, inode);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 	fdput(real);
 
 out_unlock:

@@ -480,7 +480,7 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
 		ret = vfs_fsync_range(real.file, start, end, datasync);
-		revert_creds(old_cred);
+		ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 	}
 
 	fdput(real);

@@ -504,7 +504,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = call_mmap(vma->vm_file, vma);
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 	ovl_file_accessed(file);
 
 	return ret;

@@ -523,7 +523,7 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = vfs_fallocate(real.file, mode, offset, len);
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 
 	/* Update size */
 	ovl_copyattr(ovl_inode_real(inode), inode);

@@ -545,7 +545,7 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = vfs_fadvise(real.file, offset, len, advice);
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 
 	fdput(real);
 

@@ -595,7 +595,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 						flags);
 		break;
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file_out)->i_sb, old_cred);
 
 	/* Update size */
 	ovl_copyattr(ovl_inode_real(inode_out), inode_out);

@@ -654,7 +654,7 @@ static int ovl_flush(struct file *file, fl_owner_t id)
 	if (real.file->f_op->flush) {
 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
 		err = real.file->f_op->flush(real.file, id);
-		revert_creds(old_cred);
+		ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 	}
 	fdput(real);
 

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 49bfa33bb682..0123270e295f 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c

@@ -78,7 +78,7 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		inode_lock(upperdentry->d_inode);
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = notify_change(&init_user_ns, upperdentry, attr, NULL);
-		revert_creds(old_cred);
+		ovl_revert_creds(dentry->d_sb, old_cred);
 		if (!err)
 			ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
 		inode_unlock(upperdentry->d_inode);

@@ -270,7 +270,7 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
 		stat->nlink = dentry->d_inode->i_nlink;
 
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 	return err;
 }

@@ -305,7 +305,7 @@ int ovl_permission(struct user_namespace *mnt_userns,
 		mask |= MAY_READ;
 	}
 	err = inode_permission(&init_user_ns, realinode, mask);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	return err;
 }

@@ -322,7 +322,7 @@ static const char *ovl_get_link(struct dentry *dentry,
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	p = vfs_get_link(ovl_dentry_real(dentry), done);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	return p;
 }
 

@@ -353,7 +353,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 	if (!value && !upperdentry) {
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = vfs_getxattr(&init_user_ns, realdentry, name, NULL, 0);
-		revert_creds(old_cred);
+		ovl_revert_creds(dentry->d_sb, old_cred);
 		if (err < 0)
 			goto out_drop_write;
 	}

@@ -374,7 +374,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 		WARN_ON(flags != XATTR_REPLACE);
 		err = vfs_removexattr(&init_user_ns, realdentry, name);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 	/* copy c/mtime */
 	ovl_copyattr(d_inode(realdentry), inode);

@@ -396,7 +396,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = __vfs_getxattr(&init_user_ns, realdentry, d_inode(realdentry),
 			     name, value, size, flags);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	return res;
 }
 

@@ -424,7 +424,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = vfs_listxattr(realdentry, list, size);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	if (res <= 0 || size == 0)
 		return res;
 

@@ -462,7 +462,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type, bool rcu)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	acl = get_acl(realinode, type);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	return acl;
 }

@@ -496,7 +496,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	return err;
 }

@@ -567,7 +567,7 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 		err = ovl_set_protattr(inode, upperpath.dentry, fa);
 		if (!err)
 			err = ovl_real_fileattr_set(&upperpath, fa);
-		revert_creds(old_cred);
+		ovl_revert_creds(inode->i_sb, old_cred);
 
 		/*
 		 * Merge real inode flags with inode flags read from

@@ -629,7 +629,7 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = ovl_real_fileattr_get(&realpath, fa);
 	ovl_fileattr_prot_flags(inode, fa);
-	revert_creds(old_cred);
+	ovl_revert_creds(inode->i_sb, old_cred);
 
 	return err;
 }

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 1a9b515fc45d..0ecb6bab0f2a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c

@@ -1106,7 +1106,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	ovl_dentry_update_reval(dentry, upperdentry,
 			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
 
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	if (origin_path) {
 		dput(origin_path->dentry);
 		kfree(origin_path);

@@ -1133,7 +1133,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	kfree(upperredirect);
 out:
 	kfree(d.redirect);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	return ERR_PTR(err);
 }
 

@@ -1185,7 +1185,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 			dput(this);
 		}
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 	return positive;
 }

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3fcd62e72aad..16c6280d8201 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h

@@ -282,6 +282,7 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
+void ovl_revert_creds(struct super_block *sb, const struct cred *oldcred);
 int ovl_can_decode_fh(struct super_block *sb);
 struct dentry *ovl_indexdir(struct super_block *sb);
 bool ovl_index_all(struct super_block *sb);

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 63efee554f69..31aebf6d2ea4 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h

@@ -20,6 +20,7 @@ struct ovl_config {
 	bool metacopy;
 	bool userxattr;
 	bool ovl_volatile;
+	bool override_creds;
 };
 
 struct ovl_sb {

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 150fdf3bc68d..c6038e6ad753 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c

@@ -286,7 +286,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
 		}
 		inode_unlock(dir->d_inode);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(rdd->dentry->d_sb, old_cred);
 
 	return err;
 }

@@ -789,7 +789,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	}
 	err = 0;
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	return err;
 }
 

@@ -841,7 +841,7 @@ static struct file *ovl_dir_open_realfile(const struct file *file,
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	res = ovl_path_open(realpath, O_RDONLY | (file->f_flags & O_LARGEFILE));
-	revert_creds(old_cred);
+	ovl_revert_creds(file_inode(file)->i_sb, old_cred);
 
 	return res;
 }

@@ -967,7 +967,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_dir_read_merged(dentry, list, &root);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 	if (err)
 		return err;
 

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 973644af1288..d1c6e883790d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c

@@ -53,6 +53,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
 MODULE_PARM_DESC(xino_auto,
 		 "Auto enable xino feature");
 
+static bool __read_mostly ovl_override_creds_def = true;
+module_param_named(override_creds, ovl_override_creds_def, bool, 0644);
+MODULE_PARM_DESC(ovl_override_creds_def,
+		 "Use mounter's credentials for accesses");
+
 static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;

@@ -382,6 +387,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 		seq_puts(m, ",volatile");
 	if (ofs->config.userxattr)
 		seq_puts(m, ",userxattr");
+	if (ofs->config.override_creds != ovl_override_creds_def)
+		seq_show_option(m, "override_creds",
+				ofs->config.override_creds ? "on" : "off");
 	return 0;
 }
 

@@ -437,6 +445,8 @@ enum {
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
 	OPT_VOLATILE,
+	OPT_OVERRIDE_CREDS_ON,
+	OPT_OVERRIDE_CREDS_OFF,
 	OPT_ERR,
 };
 

@@ -459,6 +469,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_VOLATILE,			"volatile"},
+	{OPT_OVERRIDE_CREDS_ON,		"override_creds=on"},
+	{OPT_OVERRIDE_CREDS_OFF,	"override_creds=off"},
 	{OPT_ERR,			NULL}
 };
 

@@ -518,6 +530,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
 	if (!config->redirect_mode)
 		return -ENOMEM;
+	config->override_creds = ovl_override_creds_def;
 
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;

@@ -619,6 +632,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->userxattr = true;
 			break;
 
+		case OPT_OVERRIDE_CREDS_ON:
+			config->override_creds = true;
+			break;
+
+		case OPT_OVERRIDE_CREDS_OFF:
+			config->override_creds = false;
+			break;
+
 		default:
 			pr_err("unrecognized mount option \"%s\" or missing value\n",
 					p);

@@ -2144,7 +2165,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	kfree(splitlower);
 
 	sb->s_root = root_dentry;
-
 	return 0;
 
 out_free_oe:

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f48284a2a896..32eceb495202 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c

@@ -38,9 +38,18 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
 
+	if (!ofs->config.override_creds)
+		return NULL;
 	return override_creds(ofs->creator_cred);
 }
 
+void ovl_revert_creds(struct super_block *sb, const struct cred *old_cred)
+{
+	if (old_cred)
+		revert_creds(old_cred);
+}
+
+
 /*
  * Check if underlying fs supports file handles and try to determine encoding
  * type, in order to deduce maximum inode number used by fs.

@@ -899,7 +908,7 @@ int ovl_nlink_start(struct dentry *dentry)
 	 * value relative to the upper inode nlink in an upper inode xattr.
 	 */
 	err = ovl_set_nlink_upper(dentry);
-	revert_creds(old_cred);
+	ovl_revert_creds(dentry->d_sb, old_cred);
 
 out:
 	if (err)

@@ -917,7 +926,7 @@ void ovl_nlink_end(struct dentry *dentry)
 
 		old_cred = ovl_override_creds(dentry->d_sb);
 		ovl_cleanup_index(dentry);
-		revert_creds(old_cred);
+		ovl_revert_creds(dentry->d_sb, old_cred);
 	}
 
 	ovl_inode_unlock(inode);

-- 
2.34.0.rc1.387.gb447b232ab-goog

On 11/16/2021 5:58 PM, David Anderson 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.

This isn't very clear. Are you saying that the security attributes
of the upper, lower, and work directories are determined by the
attributes of the process that mounted the filesystem? What is an
"incoming access"? I'm sure that means something if you're steeped
in the lore of overlayfs, but it isn't obvious to me.

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.

I'm sorry, but I've tried pretty hard, and can't puzzle that one out.

   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.

DAC privileges are not hierarchical. This doesn't make any sense.

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.

I think I might have figured that one out, but in order to do so
I have to make way too many assumptions about the earlier paragraph.
Could you please try to explain what you're doing with more context?

   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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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.

v18
- rebase + fix minor cut and paste error for inode argument in __vfs_getxattr

v17
- correct some zero-day build failures.
- fix up documentation

v16
- rebase and merge of two patches.
- add adjustment to deal with execv when overrides is off.

v15
- Revert back to v4 with fixes from on the way from v5-v14. The single
   structure argument passing to address the complaints about too many
   arguments was rejected by the community.
- Drop the udner discussion fix for an additional CAP_DAC_READ_SEARCH
   check. Can address that independently.
- ToDo: upstream test frame for thes security fixes (currently testing
   is all in Android).

v14:
- Rejoin, rebase and a few adjustments.

v13:
- Pull out first patch and try to get it in alone feedback, some
   Acks, and then <crickets> because people forgot why we were doing i.

v12:
- Restore squished out patch 2 and 3 in the series,
   then change algorithm to add flags argument.
   Per-thread flag is a large security surface.

v11:
- Squish out v10 introduced patch 2 and 3 in the series,
   then and use per-thread flag instead for nesting.
- Switch name to ovl_do_vds_getxattr for __vds_getxattr wrapper.
- Add sb argument to ovl_revert_creds to match future work.

v10:
- Return NULL on CAP_DAC_READ_SEARCH
- Add __get xattr method to solve sepolicy logging issue
- Drop unnecessary sys_admin sepolicy checking for administrative
   driver internal xattr functions.

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.

v5:
- beefed up the caveats in the Documentation
- Is dependent on
   "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
   "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v4:
- spelling and grammar errors in text

v3:
- Change name from caller_credentials / creator_credentials to the
   boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

David Anderson (4):
   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
   overlayfs: inode_owner_or_capable called during execv

  Documentation/filesystems/locking.rst   |  2 +-
  Documentation/filesystems/overlayfs.rst | 26 ++++++++++++++-
  fs/9p/acl.c                             |  3 +-
  fs/9p/xattr.c                           |  3 +-
  fs/afs/xattr.c                          | 10 +++---
  fs/attr.c                               |  2 +-
  fs/btrfs/xattr.c                        |  3 +-
  fs/ceph/xattr.c                         |  3 +-
  fs/cifs/xattr.c                         |  2 +-
  fs/ecryptfs/inode.c                     |  6 ++--
  fs/ecryptfs/mmap.c                      |  5 +--
  fs/erofs/xattr.c                        |  3 +-
  fs/ext2/xattr_security.c                |  2 +-
  fs/ext2/xattr_trusted.c                 |  2 +-
  fs/ext2/xattr_user.c                    |  2 +-
  fs/ext4/xattr_hurd.c                    |  2 +-
  fs/ext4/xattr_security.c                |  2 +-
  fs/ext4/xattr_trusted.c                 |  2 +-
  fs/ext4/xattr_user.c                    |  2 +-
  fs/f2fs/xattr.c                         |  4 +--
  fs/fuse/xattr.c                         |  4 +--
  fs/gfs2/xattr.c                         |  3 +-
  fs/hfs/attr.c                           |  2 +-
  fs/hfsplus/xattr.c                      |  3 +-
  fs/hfsplus/xattr_security.c             |  3 +-
  fs/hfsplus/xattr_trusted.c              |  3 +-
  fs/hfsplus/xattr_user.c                 |  3 +-
  fs/inode.c                              |  7 +++--
  fs/internal.h                           |  3 +-
  fs/jffs2/security.c                     |  3 +-
  fs/jffs2/xattr_trusted.c                |  3 +-
  fs/jffs2/xattr_user.c                   |  3 +-
  fs/jfs/xattr.c                          |  5 +--
  fs/kernfs/inode.c                       |  3 +-
  fs/nfs/nfs4proc.c                       |  9 ++++--
  fs/ntfs3/xattr.c                        |  2 +-
  fs/ocfs2/xattr.c                        |  9 ++++--
  fs/open.c                               |  2 +-
  fs/orangefs/xattr.c                     |  3 +-
  fs/overlayfs/copy_up.c                  |  2 +-
  fs/overlayfs/dir.c                      | 17 +++++-----
  fs/overlayfs/file.c                     | 25 ++++++++-------
  fs/overlayfs/inode.c                    | 29 ++++++++---------
  fs/overlayfs/namei.c                    |  6 ++--
  fs/overlayfs/overlayfs.h                |  7 +++--
  fs/overlayfs/ovl_entry.h                |  1 +
  fs/overlayfs/readdir.c                  |  8 ++---
  fs/overlayfs/super.c                    | 34 ++++++++++++++++----
  fs/overlayfs/util.c                     | 13 ++++++--
  fs/posix_acl.c                          |  2 +-
  fs/reiserfs/xattr_security.c            |  3 +-
  fs/reiserfs/xattr_trusted.c             |  3 +-
  fs/reiserfs/xattr_user.c                |  3 +-
  fs/squashfs/xattr.c                     |  2 +-
  fs/ubifs/xattr.c                        |  3 +-
  fs/xattr.c                              | 42 +++++++++++++------------
  fs/xfs/xfs_xattr.c                      |  3 +-
  include/linux/lsm_hook_defs.h           |  3 +-
  include/linux/security.h                |  6 ++--
  include/linux/xattr.h                   |  6 ++--
  include/uapi/linux/xattr.h              |  7 +++--
  mm/shmem.c                              |  3 +-
  net/socket.c                            |  3 +-
  security/commoncap.c                    | 11 ++++---
  security/integrity/evm/evm_main.c       | 13 +++++---
  security/security.c                     |  5 +--
  security/selinux/hooks.c                | 19 ++++++-----
  security/smack/smack_lsm.c              | 18 ++++++-----
  68 files changed, 289 insertions(+), 167 deletions(-)

On Tue, Nov 16, 2021 at 6:18 PM Casey Schaufler [off-list ref] wrote:

On 11/16/2021 5:58 PM, David Anderson wrote:

quoted

Mark Salyzyn (3):

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.

This isn't very clear. Are you saying that the security attributes
of the upper, lower, and work directories are determined by the
attributes of the process that mounted the filesystem? What is an
"incoming access"? I'm sure that means something if you're steeped
in the lore of overlayfs, but it isn't obvious to me.

(Sorry, hitting "Reply All" this time...)

Thanks for taking a look - Yes. An "incoming access" is the user
application security context accessing the filesystem.

quoted

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.

I'm sorry, but I've tried pretty hard, and can't puzzle that one out.

If your sepolicy is designed to give processes minimal privileges (as ours is),
then "init" might lack privileges even though other processes have them. For
example, init can mount /x but not access /x/y/z. But, process XYZ can access
/x/y/z. In our system processes have no privileges to anything by default,
and permissions are granted as needed, as narrowly as possible.

DAC privileges are not hierarchical. This doesn't make any sense.

Sorry, that was probably not the right word. The intent was to say that a
process with minimal DAC privileges might be able to access a file, but
a process with expansive DAC privileges might be denied access to the
same file due to MAC restrictions.

I think I might have figured that one out, but in order to do so
I have to make way too many assumptions about the earlier paragraph.
Could you please try to explain what you're doing with more context?

Hopefully the above helps explain: overlayfs uses the mounter's privileges,
which does not work on a system where the mounter does not have a
superset of child processes' privileges. That's the crux of the issue and
I'll keep working on how it's communicated in the patch description.

-David

On Tue, Nov 16, 2021 at 6:18 PM Casey Schaufler [off-list ref] wrote:

On 11/16/2021 5:58 PM, David Anderson wrote:

quoted

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.

This isn't very clear. Are you saying that the security attributes
of the upper, lower, and work directories are determined by the
attributes of the process that mounted the filesystem? What is an
"incoming access"? I'm sure that means something if you're steeped
in the lore of overlayfs, but it isn't obvious to me.

quoted

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.

I'm sorry, but I've tried pretty hard, and can't puzzle that one out.

quoted

   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.

DAC privileges are not hierarchical. This doesn't make any sense.

quoted

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.

I think I might have figured that one out, but in order to do so
I have to make way too many assumptions about the earlier paragraph.
Could you please try to explain what you're doing with more context?

quoted

   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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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.

v18
- rebase + fix minor cut and paste error for inode argument in __vfs_getxattr

v17
- correct some zero-day build failures.
- fix up documentation

v16
- rebase and merge of two patches.
- add adjustment to deal with execv when overrides is off.

v15
- Revert back to v4 with fixes from on the way from v5-v14. The single
   structure argument passing to address the complaints about too many
   arguments was rejected by the community.
- Drop the udner discussion fix for an additional CAP_DAC_READ_SEARCH
   check. Can address that independently.
- ToDo: upstream test frame for thes security fixes (currently testing
   is all in Android).

v14:
- Rejoin, rebase and a few adjustments.

v13:
- Pull out first patch and try to get it in alone feedback, some
   Acks, and then <crickets> because people forgot why we were doing i.

v12:
- Restore squished out patch 2 and 3 in the series,
   then change algorithm to add flags argument.
   Per-thread flag is a large security surface.

v11:
- Squish out v10 introduced patch 2 and 3 in the series,
   then and use per-thread flag instead for nesting.
- Switch name to ovl_do_vds_getxattr for __vds_getxattr wrapper.
- Add sb argument to ovl_revert_creds to match future work.

v10:
- Return NULL on CAP_DAC_READ_SEARCH
- Add __get xattr method to solve sepolicy logging issue
- Drop unnecessary sys_admin sepolicy checking for administrative
   driver internal xattr functions.

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.

v5:
- beefed up the caveats in the Documentation
- Is dependent on
   "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
   "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v4:
- spelling and grammar errors in text

v3:
- Change name from caller_credentials / creator_credentials to the
   boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

David Anderson (4):
   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
   overlayfs: inode_owner_or_capable called during execv

  Documentation/filesystems/locking.rst   |  2 +-
  Documentation/filesystems/overlayfs.rst | 26 ++++++++++++++-
  fs/9p/acl.c                             |  3 +-
  fs/9p/xattr.c                           |  3 +-
  fs/afs/xattr.c                          | 10 +++---
  fs/attr.c                               |  2 +-
  fs/btrfs/xattr.c                        |  3 +-
  fs/ceph/xattr.c                         |  3 +-
  fs/cifs/xattr.c                         |  2 +-
  fs/ecryptfs/inode.c                     |  6 ++--
  fs/ecryptfs/mmap.c                      |  5 +--
  fs/erofs/xattr.c                        |  3 +-
  fs/ext2/xattr_security.c                |  2 +-
  fs/ext2/xattr_trusted.c                 |  2 +-
  fs/ext2/xattr_user.c                    |  2 +-
  fs/ext4/xattr_hurd.c                    |  2 +-
  fs/ext4/xattr_security.c                |  2 +-
  fs/ext4/xattr_trusted.c                 |  2 +-
  fs/ext4/xattr_user.c                    |  2 +-
  fs/f2fs/xattr.c                         |  4 +--
  fs/fuse/xattr.c                         |  4 +--
  fs/gfs2/xattr.c                         |  3 +-
  fs/hfs/attr.c                           |  2 +-
  fs/hfsplus/xattr.c                      |  3 +-
  fs/hfsplus/xattr_security.c             |  3 +-
  fs/hfsplus/xattr_trusted.c              |  3 +-
  fs/hfsplus/xattr_user.c                 |  3 +-
  fs/inode.c                              |  7 +++--
  fs/internal.h                           |  3 +-
  fs/jffs2/security.c                     |  3 +-
  fs/jffs2/xattr_trusted.c                |  3 +-
  fs/jffs2/xattr_user.c                   |  3 +-
  fs/jfs/xattr.c                          |  5 +--
  fs/kernfs/inode.c                       |  3 +-
  fs/nfs/nfs4proc.c                       |  9 ++++--
  fs/ntfs3/xattr.c                        |  2 +-
  fs/ocfs2/xattr.c                        |  9 ++++--
  fs/open.c                               |  2 +-
  fs/orangefs/xattr.c                     |  3 +-
  fs/overlayfs/copy_up.c                  |  2 +-
  fs/overlayfs/dir.c                      | 17 +++++-----
  fs/overlayfs/file.c                     | 25 ++++++++-------
  fs/overlayfs/inode.c                    | 29 ++++++++---------
  fs/overlayfs/namei.c                    |  6 ++--
  fs/overlayfs/overlayfs.h                |  7 +++--
  fs/overlayfs/ovl_entry.h                |  1 +
  fs/overlayfs/readdir.c                  |  8 ++---
  fs/overlayfs/super.c                    | 34 ++++++++++++++++----
  fs/overlayfs/util.c                     | 13 ++++++--
  fs/posix_acl.c                          |  2 +-
  fs/reiserfs/xattr_security.c            |  3 +-
  fs/reiserfs/xattr_trusted.c             |  3 +-
  fs/reiserfs/xattr_user.c                |  3 +-
  fs/squashfs/xattr.c                     |  2 +-
  fs/ubifs/xattr.c                        |  3 +-
  fs/xattr.c                              | 42 +++++++++++++------------
  fs/xfs/xfs_xattr.c                      |  3 +-
  include/linux/lsm_hook_defs.h           |  3 +-
  include/linux/security.h                |  6 ++--
  include/linux/xattr.h                   |  6 ++--
  include/uapi/linux/xattr.h              |  7 +++--
  mm/shmem.c                              |  3 +-
  net/socket.c                            |  3 +-
  security/commoncap.c                    | 11 ++++---
  security/integrity/evm/evm_main.c       | 13 +++++---
  security/security.c                     |  5 +--
  security/selinux/hooks.c                | 19 ++++++-----
  security/smack/smack_lsm.c              | 18 ++++++-----
  68 files changed, 289 insertions(+), 167 deletions(-)

On Wed, Nov 17, 2021 at 3:58 AM David Anderson [off-list ref] 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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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].

Specifically, the patch 2/4 is very wrong for unprivileged mount and
I think that the very noisy patch 1/4 could be completely avoided:
Can't you use -o userxattr mount option for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Also, IMO, the Documentation section about "Non overlapping credentials" does
not hold the same standards as the section about "Permission model" -
it does not
state the requirements clear enough for my non-security-oriented brain to be
able to understand if the "Ignore mounter's credentials" model can be exploited.

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I think that the core problem with the approach is using Non-uniform
credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegtMoD85j5namV592sJD23QeUMD=+tq4SvFDqjVxsAszYQ@mail.gmail.com/

On Tue, Nov 16, 2021 at 11:36 PM Amir Goldstein [off-list ref] wrote:

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].

Indeed I'm carrying this forward as Mark is no longer working on it.
My apologies for
missing those comments!

Specifically, the patch 2/4 is very wrong for unprivileged mount and
I think that the very noisy patch 1/4 could be completely avoided:
Can't you use -o userxattr mount option for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Can you clarify a bit more? The patch is definitely super noisy and I'd love
to have a better solution. The problem it's trying to solve is:
 1. Kernel-privileged init mounts /mnt/blah-lower and /mnt/blah-upper.
 2. Kernel-privileged init mounts /blah with overlayfs using the above dirs.
 2. Kernel-privileged init loads sepolicy off /blah/policy. Enforcing begins.
 3. Kernel-privileged init tries to execute /blah/init to initiate a
domain transition.
 4. exec() fails because the overlayfs mounter creds (kernel domain) does
     not have getxattr permission to /blah/init.

Eg, we're hitting this problem without even making changes to the mount, and
without anything being written to /mnt/blah-upper.

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I'll need to think about this more and test to verify. It's not a scenario that
would come up in our use case (both dirs effectively have the same permissions).

If the answer is "yes, that can happen" - do you see this as a problem of
clarifying the model, or a problem of fixing that loophole?

quoted

I think that the core problem with the approach is using Non-uniform

credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

In a security model like ours, I think there's no way around it, that
we really need
accesses to be from the caller's credentials and not the mounter's. It's even
worse than earlier iterations of this patch perhaps let on: we mount
before sepolicy
is loaded (so we can overlay the policy itself), and thus the
mounter's creds are
effectively "the kernel". This domain is highly restricted in our
sepolicy for obvious
reasons. There's no way our security team will let us unrestrict it.

Best,

-David

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegtMoD85j5namV592sJD23QeUMD=+tq4SvFDqjVxsAszYQ@mail.gmail.com/

On Thu, Nov 18, 2021 at 11:53 AM David Anderson [off-list ref] wrote:

On Tue, Nov 16, 2021 at 11:36 PM Amir Goldstein [off-list ref] wrote:

quoted

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].

Indeed I'm carrying this forward as Mark is no longer working on it.
My apologies for
missing those comments!

quoted

Specifically, the patch 2/4 is very wrong for unprivileged mount and
I think that the very noisy patch 1/4 could be completely avoided:
Can't you use -o userxattr mount option for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Can you clarify a bit more? The patch is definitely super noisy and I'd love
to have a better solution. The problem it's trying to solve is:
 1. Kernel-privileged init mounts /mnt/blah-lower and /mnt/blah-upper.
 2. Kernel-privileged init mounts /blah with overlayfs using the above dirs.
 2. Kernel-privileged init loads sepolicy off /blah/policy. Enforcing begins.
 3. Kernel-privileged init tries to execute /blah/init to initiate a
domain transition.
 4. exec() fails because the overlayfs mounter creds (kernel domain) does
     not have getxattr permission to /blah/init.

Eg, we're hitting this problem without even making changes to the mount, and
without anything being written to /mnt/blah-upper.

So what is your solution?
Remove the security check from overlayfs setting xattr?
How does that work for the person who composed the security policy?
You will need to grant some basic privileges to the mounter.
If you do not want to grant the mounter privileges to set trusted.overlay xattr
you may use mount option -o userxattr and grant it permissions to set
user.overlay xattrs.

quoted

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I'll need to think about this more and test to verify. It's not a scenario that
would come up in our use case (both dirs effectively have the same permissions).

Your argument is taking the wrong POV.
The reason that previous posts of this patch set have been rejected
is not because it doesn't work for your use case.
It is because other players can exploit the feature to bypass security
policies, so the feature cannot be merged as is.

If the answer is "yes, that can happen" - do you see this as a problem of
clarifying the model, or a problem of fixing that loophole?

It is something that is not at all easy to fix.
In the example above, instead of checking permissions against the
overlay inode (on "incoming" readdir) will need to check permissions of every
accessing user against all layers, before allowing access to the merged
directory content (which is cached).
A lot more work - and this is just for this one example.

quoted

quoted

I think that the core problem with the approach is using Non-uniform

credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

In a security model like ours, I think there's no way around it, that
we really need
accesses to be from the caller's credentials and not the mounter's. It's even
worse than earlier iterations of this patch perhaps let on: we mount
before sepolicy
is loaded (so we can overlay the policy itself), and thus the
mounter's creds are
effectively "the kernel". This domain is highly restricted in our
sepolicy for obvious
reasons. There's no way our security team will let us unrestrict it.

Not sure what that means or what I can do with this information.
If I had a simple suggestion on how to solve your problem I would have
suggested it, but I cannot think of any right now.
The best I can do is to try to make you understand the problems that your
patch causes to others, so you can figure out a way that meets your goals
without breaking other use cases.

Thanks,
Amir.

On Thu, Nov 18, 2021 at 2:20 AM Amir Goldstein [off-list ref] wrote:

On Thu, Nov 18, 2021 at 11:53 AM David Anderson [off-list ref] wrote:

quoted

On Tue, Nov 16, 2021 at 11:36 PM Amir Goldstein [off-list ref] wrote:

quoted

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].

Indeed I'm carrying this forward as Mark is no longer working on it.
My apologies for
missing those comments!

quoted

Specifically, the patch 2/4 is very wrong for unprivileged mount and
I think that the very noisy patch 1/4 could be completely avoided:
Can't you use -o userxattr mount option for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Can you clarify a bit more? The patch is definitely super noisy and I'd love
to have a better solution. The problem it's trying to solve is:
 1. Kernel-privileged init mounts /mnt/blah-lower and /mnt/blah-upper.
 2. Kernel-privileged init mounts /blah with overlayfs using the above dirs.
 2. Kernel-privileged init loads sepolicy off /blah/policy. Enforcing begins.
 3. Kernel-privileged init tries to execute /blah/init to initiate a
domain transition.
 4. exec() fails because the overlayfs mounter creds (kernel domain) does
     not have getxattr permission to /blah/init.

Eg, we're hitting this problem without even making changes to the mount, and
without anything being written to /mnt/blah-upper.

So what is your solution?
Remove the security check from overlayfs setting xattr?
How does that work for the person who composed the security policy?
You will need to grant some basic privileges to the mounter.
If you do not want to grant the mounter privileges to set trusted.overlay xattr
you may use mount option -o userxattr and grant it permissions to set
user.overlay xattrs.

Thanks for the tips. I think it might be possible for us to work
around this one in
userspace, either by granting the mounter this one permission across
all contexts,
or with a much narrower kernel fix that doesn't require us keeping the big noisy
patch in our tree.

quoted

quoted

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I'll need to think about this more and test to verify. It's not a scenario that
would come up in our use case (both dirs effectively have the same permissions).

Your argument is taking the wrong POV.
The reason that previous posts of this patch set have been rejected
is not because it doesn't work for your use case.
It is because other players can exploit the feature to bypass security
policies, so the feature cannot be merged as is.

Ack, understood.

quoted

If the answer is "yes, that can happen" - do you see this as a problem of
clarifying the model, or a problem of fixing that loophole?

It is something that is not at all easy to fix.
In the example above, instead of checking permissions against the
overlay inode (on "incoming" readdir) will need to check permissions of every
accessing user against all layers, before allowing access to the merged
directory content (which is cached).
A lot more work - and this is just for this one example.

I see your point. If we could implement that, behind a mount flag, would that be
an acceptable solution?

quoted

quoted

quoted

I think that the core problem with the approach is using Non-uniform

credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

In a security model like ours, I think there's no way around it, that
we really need
accesses to be from the caller's credentials and not the mounter's. It's even
worse than earlier iterations of this patch perhaps let on: we mount
before sepolicy
is loaded (so we can overlay the policy itself), and thus the
mounter's creds are
effectively "the kernel". This domain is highly restricted in our
sepolicy for obvious
reasons. There's no way our security team will let us unrestrict it.

Not sure what that means or what I can do with this information.
If I had a simple suggestion on how to solve your problem I would have
suggested it, but I cannot think of any right now.
The best I can do is to try to make you understand the problems that your
patch causes to others, so you can figure out a way that meets your goals
without breaking other use cases.

To help clarify, we do not have any processes which have access to everything.
Almost every file in the system has a specific selabel and processes are only
granted access to the labels they need.

To give the mounter (init in kernel domain) access to every single label doesn't
make sense. It only makes sense to have the access be "passthrough", making
overlayfs as transparent as possible.

Best,

-David

Thanks,
Amir.

quoted

It is something that is not at all easy to fix.
In the example above, instead of checking permissions against the
overlay inode (on "incoming" readdir) will need to check permissions of every
accessing user against all layers, before allowing access to the merged
directory content (which is cached).
A lot more work - and this is just for this one example.

I see your point. If we could implement that, behind a mount flag, would that be
an acceptable solution?

As I wrote, this is one specific problem that I identified.
If you propose a different behavior base on mount flag you should
be able to argue that is cannot be exploited to circumvent security
access policies, by peaking into cached copies of objects that the user
has no access to, or by any other way.

I have no idea how to implement what you want and prove that
it is safe.
Maybe if you explained the use case in greater details with some
examples someone could help you reach a possible solution.

Thanks,
Amir.

As I wrote, this is one specific problem that I identified.
If you propose a different behavior base on mount flag you should
be able to argue that is cannot be exploited to circumvent security
access policies, by peaking into cached copies of objects that the user
has no access to, or by any other way.

I have no idea how to implement what you want and prove that
it is safe.
Maybe if you explained the use case in greater details with some
examples someone could help you reach a possible solution.

I'm going to wake up this thread one last time to lay it to rest permanently.
We have now reimplemented our use of overlayfs to no longer need these patches.
We will no longer be attempting to get this patch set accepted.

One issue - remount does not update the mounter credentials, either by default
or via a flag. I was able to work around this, but it would have been much
easier had I simply been able to remount with new credentials. (The specific
use case is that we load sepolicy from a potentially overlaid partition, so the
original mounter will always have the default kernel domain, which will not be
suitable once sepolicy is enforced.)

Is this a design decision? Would a patch to set credentials during remount be
of interest?

Thanks,
Paul

On Tue, Feb 25, 2025 at 2:26 PM Paul Lawrence [off-list ref] wrote:

Would a patch to set credentials during remount be
of interest?

Amir mentioned (in a html email so I'm not sure it will go through the
lists, I haven't seen it yet) that Christian recently proposed an
override_creds option using the new mount API, does anyone have a lore
link they could share?

-- 
paul-moore.com

On Wed, Nov 17, 2021 at 09:36:42AM +0200, Amir Goldstein wrote:

On Wed, Nov 17, 2021 at 3:58 AM David Anderson [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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 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. 

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?

Thanks
Vivek

for Android use case and limit
the manipulation of user.ovrelay.* xattr based on sepolicy for actors
that are allowed
to make changes in overlayfs mount? or not limit at all?
The access to those xattr is forbidden via "incoming" xattr ops on
overlay inodes.

Also, IMO, the Documentation section about "Non overlapping credentials" does
not hold the same standards as the section about "Permission model" -
it does not
state the requirements clear enough for my non-security-oriented brain to be
able to understand if the "Ignore mounter's credentials" model can be exploited.

Can an unprivileged user create an overlay over a directory that they have
access to and redirect an innocent looking file name to an underlying file that
said the mounting user has no access to and by doing that, tricking a privileged
user to modify the innocent looking file on the  mounter's behalf?
Of course this could be avoided by forbidding unprivileged mount with
override_creds=off, but there could be other scenarios, so a clear model
would help to understand the risks.

For example:
If user 1 was able to read in lower dir A, now the content of overlay dir A
is cached and user 2, that has permissions to read upper dir A and does
not have read permissions on lower dir A will see the content of lower dir A.

I think that the core problem with the approach is using Non-uniform
credentials to access underlying layers. I don't see a simple way around
a big audit that checks all those cases, but maybe I'm missing some quick
shortcut or maybe your use case can add some restrictions about the
users that could access this overlay that would simplify the generic problem.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegtMoD85j5namV592sJD23QeUMD=+tq4SvFDqjVxsAszYQ@mail.gmail.com/

On Fri, Dec 3, 2021 at 10:38 AM Vivek Goyal [off-list ref] wrote:

On Wed, Nov 17, 2021 at 09:36:42AM +0200, Amir Goldstein wrote:

quoted

On Wed, Nov 17, 2021 at 3:58 AM David Anderson [off-list ref] wrote:

quoted

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

BTW, where is patch 1 of the series. I can't seem to find it.

Lore to the rescue ...

https://lore.kernel.org/selinux/20211117015806.2192263-2-dvander@google.com/

-- 
paul moore
www.paul-moore.com

On Fri, Dec 3, 2021 at 5:38 PM Vivek Goyal [off-list ref] wrote:

On Wed, Nov 17, 2021 at 09:36:42AM +0200, Amir Goldstein wrote:

quoted

On Wed, Nov 17, 2021 at 3:58 AM David Anderson [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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.

quoted

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?

quoted

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.

quoted

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.

On Fri, Dec 03, 2021 at 06:31:01PM +0200, Amir Goldstein wrote:

On Fri, Dec 3, 2021 at 5:38 PM Vivek Goyal [off-list ref] wrote:

quoted

On Wed, Nov 17, 2021 at 09:36:42AM +0200, Amir Goldstein wrote:

quoted

On Wed, Nov 17, 2021 at 3:58 AM David Anderson [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: David Anderson <redacted>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <redacted>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <redacted>
Cc: Stephen Smalley <redacted>
Cc: John Stultz <redacted>
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.

quoted

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.

I am also concerned about this.

Certainly since non privileged mounts are likely to use userxattr
anyway, so what's the reason to bypass security?

I am not sure. In the early version of patches I think argument was
that do not switch to mounter's creds and use caller's creds on 
underlying filesystem as well. And each caller will be privileged
enough to be able to perform the operation.

Our take was that how is this model better because in current model
only mounter needs to be privileged while in this new model each
caller will have to be privileged. But Android guys seemed to be ok
with that. So has this assumption changed since early days. If callers
are privileged, then vfs_getxattr() on underlying filesystem for
overaly internal xattrs should succeed and there is no need for this
change.

I suspect patches have evolved since then and callers are not as
privileged as we expect them to and that's why we are bypassing this
check on all overlayfs internal trusted xattrs? This definitely requires
much close scrutiny. My initial reaction is that this sounds very scary.

In general I would think overlayfs should not bypass the check on
underlying fs. Either checks should be done in mounter's context or
caller's context (depending on override_creds=on/off).

Thanks
Vivek

quoted

quoted

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.

quoted

quoted

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.

I wanted to try and bring this thread back from the dead (?) as I
believe the use-case is still valid and worth supporting.  Some more
brief comments below ...

On Fri, Dec 3, 2021 at 1:34 PM Vivek Goyal [off-list ref] wrote:

I am not sure. In the early version of patches I think argument was
that do not switch to mounter's creds and use caller's creds on
underlying filesystem as well. And each caller will be privileged
enough to be able to perform the operation.

The basic idea is that we can now build Linux systems with enough
access control granularity such that a given process can have the
necessary privileges to mount a filesystem, but not necessarily access
all of the data on the filesystem, while other processes, with
different access rights, are allowed to read and write data on the
mounted filesystem.  Granted, this is a bit different from how things
are usually done, but in my opinion it's a valid and interesting use
case in that it allows us to remove unneeded access rights from
historically very privileged system startup services/scripts: the
service that runs to mount my homedir shouldn't be allowed to access
my files just to mount the directory.

Unfortunately, this idea falls apart when we attempt to use overlayfs
due to the clever/usual way it caches the mounting processes
credentials and uses that in place of the current process' credentials
when accessing certain parts of the underlying filesystems.  The
current overlayfs implementation assumes that the mounter will always
be more privileged than the processes accessing the filesystem, it
would be nice if we could build a mechanism that didn't have this
assumption baked into the implementation.

This patchset may not have been The Answer, but surely there is
something we can do to support this use-case.

Our take was that how is this model better because in current model
only mounter needs to be privileged while in this new model each
caller will have to be privileged. But Android guys seemed to be ok
with that. So has this assumption changed since early days. If callers
are privileged, then vfs_getxattr() on underlying filesystem for
overaly internal xattrs should succeed and there is no need for this
change.

I suspect patches have evolved since then and callers are not as
privileged as we expect them to and that's why we are bypassing this
check on all overlayfs internal trusted xattrs? This definitely requires
much close scrutiny. My initial reaction is that this sounds very scary.

In general I would think overlayfs should not bypass the check on
underlying fs. Either checks should be done in mounter's context or
caller's context (depending on override_creds=on/off).

Thanks
Vivek

-- 
paul-moore.com