linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/1] Relax restrictions on user.* xattr
@ 2021-09-02 15:22 Vivek Goyal
  2021-09-02 15:22 ` [PATCH v3 1/1] xattr: Allow user.* xattr on symlink and special files Vivek Goyal
                   ` (5 more replies)
  0 siblings, 6 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 15:22 UTC (permalink / raw)
  To: viro
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert, vgoyal,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

Hi,

This is V3 of the patch. Previous versions were posted here.

v2:
https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
v1:
https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
+m/

Changes since v2
----------------
- Do not call inode_permission() for special files as file mode bits
  on these files represent permissions to read/write from/to device
  and not necessarily permission to read/write xattrs. In this case
  now user.* extended xattrs can be read/written on special files
  as long as caller is owner of file or has CAP_FOWNER.
 
- Fixed "man xattr". Will post a patch in same thread little later. (J.
  Bruce Fields)

- Fixed xfstest 062. Changed it to run only on older kernels where
  user extended xattrs are not allowed on symlinks/special files. Added
  a new replacement test 648 which does exactly what 062. Just that
  it is supposed to run on newer kernels where user extended xattrs
  are allowed on symlinks and special files. Will post patch in 
  same thread (Ted Ts'o).

Testing
-------
- Ran xfstest "./check -g auto" with and without patches and did not
  notice any new failures.

- Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
  filesystems and it works.
 
Description
===========

Right now we don't allow setting user.* xattrs on symlinks and special
files at all. Initially I thought that real reason behind this
restriction is quota limitations but from last conversation it seemed
that real reason is that permission bits on symlink and special files
are special and different from regular files and directories, hence
this restriction is in place. (I tested with xfs user quota enabled and
quota restrictions kicked in on symlink).

This version of patch allows reading/writing user.* xattr on symlink and
special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

Who wants to set user.* xattr on symlink/special files
-----------------------------------------------------
I have primarily two users at this point of time.

- virtiofs daemon.

- fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
  fuse-overlay as well and he ran into similar issue. So fuse-overlay
  should benefit from this change as well.

Why virtiofsd wants to set user.* xattr on symlink/special files
----------------------------------------------------------------
In virtiofs, actual file server is virtiosd daemon running on host.
There we have a mode where xattrs can be remapped to something else.
For example security.selinux can be remapped to
user.virtiofsd.securit.selinux on the host.

This remapping is useful when SELinux is enabled in guest and virtiofs
as being used as rootfs. Guest and host SELinux policy might not match
and host policy might deny security.selinux xattr setting by guest
onto host. Or host might have SELinux disabled and in that case to
be able to set security.selinux xattr, virtiofsd will need to have
CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
guest security.selinux (or other xattrs) on host to something else
is also better from security point of view.

But when we try this, we noticed that SELinux relabeling in guest
is failing on some symlinks. When I debugged a little more, I
came to know that "user.*" xattrs are not allowed on symlinks
or special files.

So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
allow virtiofs to arbitrarily remap guests's xattrs to something
else on host and that solves this SELinux issue nicely and provides
two SELinux policies (host and guest) to co-exist nicely without
interfering with each other.

Thanks
Vivek

Vivek Goyal (1):
  xattr: Allow user.* xattr on symlink and special files

 fs/xattr.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v3 1/1] xattr: Allow user.* xattr on symlink and special files
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
@ 2021-09-02 15:22 ` Vivek Goyal
  2021-09-02 15:38 ` [PATCH 2/1] man-pages: xattr.7: Update text for user extended xattr behavior change Vivek Goyal
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 15:22 UTC (permalink / raw)
  To: viro
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert, vgoyal,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

Currently user.* xattr are not allowed on symlink and special files.

man xattr and recent discussion suggested that primary reason for this
restriction is how file permissions for symlinks and special files
are little different from regular files and directories.

For symlinks, they are world readable/writable and if user xattr were
to be permitted, it will allow unpriviliged users to dump a huge amount
of user.* xattrs on symlinks without any control. (I think quota control
still works with symlinks, just that quota is not typically deployed).

For special files, permissions typically control capability to read/write
from devices (and not necessarily from filesystem). So if a user can
write to device (/dev/null), does not necessarily mean it should be allowed
to write large number of user.* xattrs on the filesystem device node is
residing in.

This patch proposes to relax the restrictions a bit and allow file owner
or privileged user (CAP_FOWNER), to be able to read/write user.* xattrs
on symlink and special files.

Note, for special files, file mode bits represent permission to access
device and not necessarily permission to read/write xattrs.
Hence, inode_permission() is not called on special files and just
being owner (or CAP_FOWNER) is enough to read/write user extended
xattrs on special files.

LSM will still get a chance to allow/deny this operation as xattr
related security hooks are still called. (security_inode_setxattr(),
security_inode_getxattr(), security_inode_removexattr(),
security_inode_listxattr())

virtiofs daemon has a need to store user.* xatrrs on all the files
(including symlinks and special files), and currently that fails. This
patch should help.

Link: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/xattr.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 5c8c5175b385..69be1681477f 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -120,13 +120,26 @@ xattr_permission(struct user_namespace *mnt_userns, struct inode *inode,
 	}
 
 	/*
-	 * In the user.* namespace, only regular files and directories can have
-	 * extended attributes. For sticky directories, only the owner and
-	 * privileged users can write attributes.
+	 * In the user.* namespace, for symlinks and special files, only
+	 * the owner and priviliged users can read/write attributes.
+	 * For sticky directories, only the owner and privileged users can
+	 * write attributes.
 	 */
 	if (!strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) {
-		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-			return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
+		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) {
+			if (!inode_owner_or_capable(mnt_userns, inode)) {
+				return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
+			}
+			/*
+			 * This is special file and file mode bits represent
+			 * permission to access device and not
+			 * necessarily permission to read/write xattrs.
+			 * Hence do not call inode_permission() and return
+			 * success.
+			 */
+			if (!S_ISLNK(inode->i_mode))
+				return 0;
+		}
 		if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
 		    (mask & MAY_WRITE) &&
 		    !inode_owner_or_capable(mnt_userns, inode))
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 2/1] man-pages: xattr.7: Update text for user extended xattr behavior change
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
  2021-09-02 15:22 ` [PATCH v3 1/1] xattr: Allow user.* xattr on symlink and special files Vivek Goyal
@ 2021-09-02 15:38 ` Vivek Goyal
  2021-09-02 15:43 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Casey Schaufler
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 15:38 UTC (permalink / raw)
  To: linux-api, mtk.manpages
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, viro

I have proposed a patch to relax restrictions on user extended xattrs and
allow file owner (or CAP_FOWNER) to get/set user extended xattrs on symlink
and device files.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 man7/xattr.7 |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: man-pages/man7/xattr.7
===================================================================
--- man-pages.orig/man7/xattr.7	2021-09-01 13:46:16.165016463 -0400
+++ man-pages/man7/xattr.7	2021-09-01 16:31:51.038016463 -0400
@@ -129,8 +129,13 @@ a way not controllable by disk quotas fo
 special files and directories.
 .PP
 For this reason,
-user extended attributes are allowed only for regular files and directories,
-and access to user extended attributes is restricted to the
+user extended attributes are allowed only for regular files and directories
+till kernel 5.14. In newer kernel (5.15 onwards), restrictions have been
+relaxed a bit and user extended attributes are also allowed on symlinks
+and special files as long as caller is either owner of the file or is
+privileged (CAP_FOWNER).
+
+Access to user extended attributes is restricted to the
 owner and to users with appropriate capabilities for directories with the
 sticky bit set (see the
 .BR chmod (1)


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
  2021-09-02 15:22 ` [PATCH v3 1/1] xattr: Allow user.* xattr on symlink and special files Vivek Goyal
  2021-09-02 15:38 ` [PATCH 2/1] man-pages: xattr.7: Update text for user extended xattr behavior change Vivek Goyal
@ 2021-09-02 15:43 ` Casey Schaufler
  2021-09-02 17:05   ` Vivek Goyal
  2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 39+ messages in thread
From: Casey Schaufler @ 2021-09-02 15:43 UTC (permalink / raw)
  To: Vivek Goyal, viro
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, Casey Schaufler

On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> Hi,
>
> This is V3 of the patch. Previous versions were posted here.
>
> v2:
> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> v1:
> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> +m/
>
> Changes since v2
> ----------------
> - Do not call inode_permission() for special files as file mode bits
>   on these files represent permissions to read/write from/to device
>   and not necessarily permission to read/write xattrs. In this case
>   now user.* extended xattrs can be read/written on special files
>   as long as caller is owner of file or has CAP_FOWNER.
>  
> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>   Bruce Fields)
>
> - Fixed xfstest 062. Changed it to run only on older kernels where
>   user extended xattrs are not allowed on symlinks/special files. Added
>   a new replacement test 648 which does exactly what 062. Just that
>   it is supposed to run on newer kernels where user extended xattrs
>   are allowed on symlinks and special files. Will post patch in 
>   same thread (Ted Ts'o).
>
> Testing
> -------
> - Ran xfstest "./check -g auto" with and without patches and did not
>   notice any new failures.
>
> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>   filesystems and it works.
>  
> Description
> ===========
>
> Right now we don't allow setting user.* xattrs on symlinks and special
> files at all. Initially I thought that real reason behind this
> restriction is quota limitations but from last conversation it seemed
> that real reason is that permission bits on symlink and special files
> are special and different from regular files and directories, hence
> this restriction is in place. (I tested with xfs user quota enabled and
> quota restrictions kicked in on symlink).
>
> This version of patch allows reading/writing user.* xattr on symlink and
> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

This part of your project makes perfect sense. There's no good
security reason that you shouldn't set user.* xattrs on symlinks
and/or special files.

However, your virtiofs use case is unreasonable.

> Who wants to set user.* xattr on symlink/special files
> -----------------------------------------------------
> I have primarily two users at this point of time.
>
> - virtiofs daemon.
>
> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>   should benefit from this change as well.
>
> Why virtiofsd wants to set user.* xattr on symlink/special files
> ----------------------------------------------------------------
> In virtiofs, actual file server is virtiosd daemon running on host.
> There we have a mode where xattrs can be remapped to something else.
> For example security.selinux can be remapped to
> user.virtiofsd.securit.selinux on the host.

As I have stated before, this introduces a breach in security.
It allows an unprivileged process on the host to manipulate the
security state of the guest. This is horribly wrong. It is not
sufficient to claim that the breach requires misconfiguration
to exploit. Don't do this.

> This remapping is useful when SELinux is enabled in guest and virtiofs
> as being used as rootfs. Guest and host SELinux policy might not match
> and host policy might deny security.selinux xattr setting by guest
> onto host. Or host might have SELinux disabled and in that case to
> be able to set security.selinux xattr, virtiofsd will need to have
> CAP_SYS_ADMIN (which we are trying to avoid).

Adding this mapping to virtiofs provides the breach for any
LSM using xattrs.

>  Being able to remap
> guest security.selinux (or other xattrs) on host to something else
> is also better from security point of view.
>
> But when we try this, we noticed that SELinux relabeling in guest
> is failing on some symlinks. When I debugged a little more, I
> came to know that "user.*" xattrs are not allowed on symlinks
> or special files.
>
> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> allow virtiofs to arbitrarily remap guests's xattrs to something
> else on host and that solves this SELinux issue nicely and provides
> two SELinux policies (host and guest) to co-exist nicely without
> interfering with each other.

virtiofs could use security.* or system.* xattrs instead of user.*
xattrs. Don't use user.* xattrs.

>
> Thanks
> Vivek
>
> Vivek Goyal (1):
>   xattr: Allow user.* xattr on symlink and special files
>
>  fs/xattr.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
                   ` (2 preceding siblings ...)
  2021-09-02 15:43 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Casey Schaufler
@ 2021-09-02 15:47 ` Vivek Goyal
  2021-09-03  4:55   ` Dave Chinner
                     ` (2 more replies)
  2021-09-02 15:50 ` [PATCH 4/1] xfstest: Add a new test to test xattr operations Vivek Goyal
  2021-09-02 17:52 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Andreas Gruenbacher
  5 siblings, 3 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 15:47 UTC (permalink / raw)
  To: fstests
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, viro


xfstests: generic/062: Do not run on newer kernels

This test has been written with assumption that setting user.* xattrs will
fail on symlink and special files. When newer kernels support setting
user.* xattrs on symlink and special files, this test starts failing.

Found it hard to change test in such a way that it works on both type of
kernels. Primary problem is 062.out file which hardcodes the output and
output will be different on old and new kernels.

So instead, do not run this test if kernel is new and is expected to
exhibit new behavior. Next patch will create a new test and run that
test on new kernel.

IOW, on old kernels run 062 and on new kernels run new test.

This is a proposed patch. Will need to be fixed if corresponding
kernel changes are merged upstream.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tests/generic/062 |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: xfstests-dev/tests/generic/062
===================================================================
--- xfstests-dev.orig/tests/generic/062	2021-08-31 15:51:08.160307982 -0400
+++ xfstests-dev/tests/generic/062	2021-08-31 16:27:41.678307982 -0400
@@ -55,6 +55,26 @@ _require_attrs
 _require_symlinks
 _require_mknod
 
+user_xattr_allowed()
+{
+	local kernel_version kernel_patchlevel
+
+	kernel_version=`uname -r | awk -F. '{print $1}'`
+	kernel_patchlevel=`uname -r | awk -F. '{print $2}'`
+
+	# Kernel version 5.14 onwards allow user xattr on symlink/special files.
+	[ $kernel_version -lt 5 ] && return 1
+	[ $kernel_patchlevel -lt 14 ] && return 1
+	return 0;
+}
+
+
+# Kernel version 5.14 onwards allow user xattr on symlink/special files.
+# Do not run this test on newer kernels. Instead run the new test
+# which has been written with the assumption that user.* xattr
+# will succeed on symlink and special files.
+user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead."
+
 rm -f $tmp.backup1 $tmp.backup2 $seqres.full
 
 # real QA test starts here


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 4/1] xfstest: Add a new test to test xattr operations
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
                   ` (3 preceding siblings ...)
  2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
@ 2021-09-02 15:50 ` Vivek Goyal
  2021-09-02 17:52 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Andreas Gruenbacher
  5 siblings, 0 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 15:50 UTC (permalink / raw)
  To: fstests
  Cc: linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, viro

generic/062 has been written with assumption that user.* xattrs will fail
on symlinks and special files. On newer kernel this will not be true. Found
it very hard to modify generic/062 so that it can deal with both the
possibilities. So creating a new test which basically is same as 062. Only
difference is that it runs only if user.* xattrs can be set on symlinks
and special files.

Given this test is more or less same as 062, I have retained original copyright
as well.

Modified the test slightly to bail out if kernel is older and user xattrs
are not supposed to be set on symlinks and special files.

This patch will need little modification if corresponding kernel changes
are merged upstream.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tests/generic/648     |  227 ++++++++++++++
 tests/generic/648.out |  766 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 993 insertions(+)

Index: xfstests-dev/tests/generic/648
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ xfstests-dev/tests/generic/648	2021-09-01 13:23:47.271016463 -0400
@@ -0,0 +1,227 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2000-2002 Silicon Graphics, Inc.  All Rights Reserved
+# Copyright (c) 2021 Red Hat, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 648
+#
+# Exercises the getfattr/setfattr tools
+# Derived from test 062. Modified it so that it can run on kernels which
+# support user.* xattr on symlinks and special files.
+#
+. ./common/preamble
+_begin_fstest attr udf auto quick
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# Override the default cleanup function.
+_cleanup()
+{
+        cd /
+	echo; echo "*** unmount"
+	_scratch_unmount 2>/dev/null
+	rm -f $tmp.*
+}
+
+getfattr()
+{
+    _getfattr --absolute-names -dh $@ 2>&1 | _filter_scratch
+}
+
+setfattr()
+{
+    $SETFATTR_PROG $@ 2>&1 | _filter_scratch
+}
+
+_create_test_bed()
+{
+	echo "*** create test bed"
+	touch $SCRATCH_MNT/reg
+	mkdir -p $SCRATCH_MNT/dir
+	ln -s $SCRATCH_MNT/dir $SCRATCH_MNT/lnk
+	mkdir $SCRATCH_MNT/dev
+	mknod $SCRATCH_MNT/dev/b b 0 0
+	mknod $SCRATCH_MNT/dev/c c 1 3
+	mknod $SCRATCH_MNT/dev/p p
+	# sanity check
+	find $SCRATCH_MNT | LC_COLLATE=POSIX sort | _filter_scratch | grep -v "lost+found"
+}
+
+# real QA test starts here
+_supported_fs generic
+
+_require_scratch
+_require_attrs
+_require_symlinks
+_require_mknod
+
+user_xattr_allowed()
+{
+	local kernel_version kernel_patchlevel
+
+	kernel_version=`uname -r | awk -F. '{print $1}'`
+	kernel_patchlevel=`uname -r | awk -F. '{print $2}'`
+
+	# Kernel version 5.14 onwards allow user xattr on symlink/special files.
+	[ $kernel_version -lt 5 ] && return 1
+	[ $kernel_patchlevel -lt 14 ] && return 1
+	return 0;
+}
+
+
+# Kernel version 5.14 onwards allow user xattr on symlink/special files.
+# Do not run this test on older kernels. Instead run the old test 062
+# which has been written with the assumption that user.* xattr
+# will not succeed on symlink and special files.
+user_xattr_allowed || _notrun "Kernel does not allows user.* xattrs on symlinks and special files. Skipping this test. Run test 062 instead."
+
+rm -f $tmp.backup1 $tmp.backup2 $seqres.full
+
+# real QA test starts here
+_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount
+_create_test_bed
+
+# In kernels before 3.0, getxattr() fails with EPERM for an attribute which
+# cannot exist.  Later kernels fail with ENODATA.  Accept both results.
+invalid_attribute_filter() {
+	sed -e "s:\(No such attribute\|Operation not permitted\):No such attribute or operation not permitted:"
+}
+
+if [ "$USE_ATTR_SECURE" = yes ]; then
+    ATTR_MODES="user security trusted"
+    ATTR_FILTER="^(user|security|trusted)"
+else
+    ATTR_MODES="user trusted"
+    ATTR_FILTER="^(user|trusted)"
+fi
+
+_require_attrs $ATTR_MODES
+
+for nsp in $ATTR_MODES; do
+	for inode in reg dir lnk dev/b dev/c dev/p; do
+
+		echo; echo "=== TYPE $inode; NAMESPACE $nsp"; echo
+		echo "*** set/get one initially empty attribute"
+
+		setfattr -h -n $nsp.name $SCRATCH_MNT/$inode
+		getfattr -m $nsp $SCRATCH_MNT/$inode
+
+		echo "*** overwrite empty, set several new attributes"
+		setfattr -h -n $nsp.name -v 0xbabe $SCRATCH_MNT/$inode
+		setfattr -h -n $nsp.name2 -v 0xdeadbeef $SCRATCH_MNT/$inode
+		setfattr -h -n $nsp.name3 -v 0xdeface $SCRATCH_MNT/$inode
+
+		echo "*** fetch several attribute names and values (hex)"
+		getfattr -m $nsp -e hex $SCRATCH_MNT/$inode
+
+		echo "*** fetch several attribute names and values (base64)"
+		getfattr -m $nsp -e base64 $SCRATCH_MNT/$inode
+
+		echo "*** shrink value of an existing attribute"
+		setfattr -h -n $nsp.name2 -v 0xdeaf $SCRATCH_MNT/$inode
+		getfattr -m $nsp -e hex $SCRATCH_MNT/$inode
+
+		echo "*** grow value of existing attribute"
+		setfattr -h -n $nsp.name2 -v 0xdecade $SCRATCH_MNT/$inode
+		getfattr -m $nsp -e hex $SCRATCH_MNT/$inode
+
+		echo "*** set an empty value for second attribute"
+		setfattr -h -n $nsp.name2 $SCRATCH_MNT/$inode
+		getfattr -m $nsp -n $nsp.name2 $SCRATCH_MNT/$inode 2>&1 | invalid_attribute_filter
+
+		echo "*** overwrite empty value"
+		setfattr -h -n $nsp.name2 -v 0xcafe $SCRATCH_MNT/$inode
+		getfattr -m $nsp -e hex -n $nsp.name2 $SCRATCH_MNT/$inode 2>&1 | invalid_attribute_filter
+
+		echo "*** remove attribute"
+		setfattr -h -x $nsp.name2 $SCRATCH_MNT/$inode
+		getfattr -m $nsp -e hex -n $nsp.name2 $SCRATCH_MNT/$inode 2>&1 | invalid_attribute_filter
+
+		echo "*** final list (strings, type=$inode, nsp=$nsp)"
+		getfattr -m $ATTR_FILTER -e hex $SCRATCH_MNT/$inode
+
+	done
+done
+
+#
+# Test the directory descent code
+#
+echo; echo
+
+_extend_test_bed()
+{
+	echo "*** extend test bed"
+	# must set some descents' attributes to be useful
+	mkdir -p $SCRATCH_MNT/here/up/ascend
+	mkdir -p $SCRATCH_MNT/descend/down/here
+	find $SCRATCH_MNT/descend | xargs setfattr -n user.x -v yz
+	find $SCRATCH_MNT/descend | xargs setfattr -n user.1 -v 23
+	find $SCRATCH_MNT/here | xargs setfattr -n trusted.a -v bc
+	find $SCRATCH_MNT/here | xargs setfattr -n trusted.9 -v 87
+	# whack a symlink in the middle, just to be difficult
+	ln -s $SCRATCH_MNT/here/up $SCRATCH_MNT/descend/and
+	# dump out our new starting point
+	find $SCRATCH_MNT | LC_COLLATE=POSIX sort | _filter_scratch | grep -v "lost+found"
+}
+
+_extend_test_bed
+
+echo
+echo "*** directory descent with us following symlinks"
+getfattr -h -L -R -m "$ATTR_FILTER" -e hex $SCRATCH_MNT | _sort_getfattr_output
+
+echo
+echo "*** directory descent without following symlinks"
+getfattr -h -P -R -m "$ATTR_FILTER" -e hex $SCRATCH_MNT | _sort_getfattr_output
+
+#
+# Test the backup/restore code
+#
+echo; echo
+
+_backup()
+{
+	# Note: we don't filter scratch here since we need to restore too.  But
+	# we *do* sort the output by path, since it otherwise would depend on
+	# readdir order, which on some filesystems may change after re-creating
+	# the files.
+	_getfattr --absolute-names -dh -R -m $ATTR_FILTER $SCRATCH_MNT | _sort_getfattr_output >$1
+	echo BACKUP $1 >>$seqres.full
+	cat $1 >> $seqres.full
+	[ ! -s $1 ] && echo "warning: $1 (backup file) is empty"
+}
+
+echo "*** backup everything"
+_backup $tmp.backup1
+
+echo "*** clear out the scratch device"
+rm -rf $(find $SCRATCH_MNT/* | grep -v "lost+found")
+echo "AFTER REMOVE" >>$seqres.full
+getfattr -L -R -m '.' $SCRATCH_MNT >>$seqres.full
+
+echo "*** reset test bed with no extended attributes"
+_create_test_bed
+_extend_test_bed
+
+echo "*** restore everything"
+setfattr -h --restore=$tmp.backup1
+_backup $tmp.backup2
+
+echo "AFTER RESTORE" >>$seqres.full
+getfattr -L -R -m '.' $SCRATCH_MNT >>$seqres.full
+
+echo "*** compare before and after backups"
+diff $tmp.backup1 $tmp.backup2
+if [ $? -ne 0 ]; then
+	echo "urk, failed - creating $seq.backup1 and $seq.backup2"
+	cp $tmp.backup1 $seq.backup1 && cp $tmp.backup2 $seq.backup2
+	status=1
+	exit
+fi
+
+# success, all done
+status=0
+exit
Index: xfstests-dev/tests/generic/648.out
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ xfstests-dev/tests/generic/648.out	2021-09-01 13:08:00.260016463 -0400
@@ -0,0 +1,766 @@
+QA output created by 648
+*** create test bed
+SCRATCH_MNT
+SCRATCH_MNT/dev
+SCRATCH_MNT/dev/b
+SCRATCH_MNT/dev/c
+SCRATCH_MNT/dev/p
+SCRATCH_MNT/dir
+SCRATCH_MNT/lnk
+SCRATCH_MNT/reg
+
+=== TYPE reg; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/reg
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/reg
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/reg
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/reg
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/reg
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/reg
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/reg
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/reg: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=reg, nsp=user)
+# file: SCRATCH_MNT/reg
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dir; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dir
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dir
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dir
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dir
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dir
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dir
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dir
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dir: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=dir, nsp=user)
+# file: SCRATCH_MNT/dir
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE lnk; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/lnk
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/lnk
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/lnk
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/lnk
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/lnk
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/lnk
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/lnk
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/lnk: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=lnk, nsp=user)
+# file: SCRATCH_MNT/lnk
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/b; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/b
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/b
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/b
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/b
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/b
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/b
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/b
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/b: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/b, nsp=user)
+# file: SCRATCH_MNT/dev/b
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/c; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/c
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/c
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/c
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/c
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/c
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/c
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/c
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/c: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/c, nsp=user)
+# file: SCRATCH_MNT/dev/c
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/p; NAMESPACE user
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/p
+user.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/p
+user.name=0xbabe
+user.name2=0xdeadbeef
+user.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/p
+user.name=0sur4=
+user.name2=0s3q2+7w==
+user.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/p
+user.name=0xbabe
+user.name2=0xdeaf
+user.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/p
+user.name=0xbabe
+user.name2=0xdecade
+user.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/p
+user.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/p
+user.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/p: user.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/p, nsp=user)
+# file: SCRATCH_MNT/dev/p
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE reg; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/reg
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/reg
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/reg
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/reg
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/reg: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=reg, nsp=trusted)
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dir; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dir
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dir
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dir
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dir
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dir: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=dir, nsp=trusted)
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE lnk; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/lnk
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/lnk
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/lnk
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/lnk
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/lnk: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=lnk, nsp=trusted)
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/b; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/b
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/b
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/b
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/b
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/b: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/b, nsp=trusted)
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/c; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/c
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/c
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/c
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/c
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/c: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/c, nsp=trusted)
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+=== TYPE dev/p; NAMESPACE trusted
+
+*** set/get one initially empty attribute
+# file: SCRATCH_MNT/dev/p
+trusted.name
+
+*** overwrite empty, set several new attributes
+*** fetch several attribute names and values (hex)
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name2=0xdeadbeef
+trusted.name3=0xdeface
+
+*** fetch several attribute names and values (base64)
+# file: SCRATCH_MNT/dev/p
+trusted.name=0sur4=
+trusted.name2=0s3q2+7w==
+trusted.name3=0s3vrO
+
+*** shrink value of an existing attribute
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name2=0xdeaf
+trusted.name3=0xdeface
+
+*** grow value of existing attribute
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name2=0xdecade
+trusted.name3=0xdeface
+
+*** set an empty value for second attribute
+# file: SCRATCH_MNT/dev/p
+trusted.name2
+
+*** overwrite empty value
+# file: SCRATCH_MNT/dev/p
+trusted.name2=0xcafe
+
+*** remove attribute
+SCRATCH_MNT/dev/p: trusted.name2: No such attribute or operation not permitted
+*** final list (strings, type=dev/p, nsp=trusted)
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+
+*** extend test bed
+SCRATCH_MNT
+SCRATCH_MNT/descend
+SCRATCH_MNT/descend/and
+SCRATCH_MNT/descend/down
+SCRATCH_MNT/descend/down/here
+SCRATCH_MNT/dev
+SCRATCH_MNT/dev/b
+SCRATCH_MNT/dev/c
+SCRATCH_MNT/dev/p
+SCRATCH_MNT/dir
+SCRATCH_MNT/here
+SCRATCH_MNT/here/up
+SCRATCH_MNT/here/up/ascend
+SCRATCH_MNT/lnk
+SCRATCH_MNT/reg
+
+*** directory descent with us following symlinks
+# file: SCRATCH_MNT/descend
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/descend/and/ascend
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/descend/down
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/descend/down/here
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/here
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/here/up
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/here/up/ascend
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+*** directory descent without following symlinks
+# file: SCRATCH_MNT/descend
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/descend/down
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/descend/down/here
+user.1=0x3233
+user.x=0x797a
+
+# file: SCRATCH_MNT/dev/b
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dev/c
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dev/p
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/dir
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/here
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/here/up
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/here/up/ascend
+trusted.9=0x3837
+trusted.a=0x6263
+
+# file: SCRATCH_MNT/lnk
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+# file: SCRATCH_MNT/reg
+trusted.name=0xbabe
+trusted.name3=0xdeface
+user.name=0xbabe
+user.name3=0xdeface
+
+
+
+*** backup everything
+*** clear out the scratch device
+*** reset test bed with no extended attributes
+*** create test bed
+SCRATCH_MNT
+SCRATCH_MNT/dev
+SCRATCH_MNT/dev/b
+SCRATCH_MNT/dev/c
+SCRATCH_MNT/dev/p
+SCRATCH_MNT/dir
+SCRATCH_MNT/lnk
+SCRATCH_MNT/reg
+*** extend test bed
+SCRATCH_MNT
+SCRATCH_MNT/descend
+SCRATCH_MNT/descend/and
+SCRATCH_MNT/descend/down
+SCRATCH_MNT/descend/down/here
+SCRATCH_MNT/dev
+SCRATCH_MNT/dev/b
+SCRATCH_MNT/dev/c
+SCRATCH_MNT/dev/p
+SCRATCH_MNT/dir
+SCRATCH_MNT/here
+SCRATCH_MNT/here/up
+SCRATCH_MNT/here/up/ascend
+SCRATCH_MNT/lnk
+SCRATCH_MNT/reg
+*** restore everything
+*** compare before and after backups
+
+*** unmount


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 15:43 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Casey Schaufler
@ 2021-09-02 17:05   ` Vivek Goyal
  2021-09-02 17:42     ` Vivek Goyal
  0 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 17:05 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> > Hi,
> >
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2:
> > https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1:
> > https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> > +m/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >  
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in 
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >  
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> This part of your project makes perfect sense. There's no good
> security reason that you shouldn't set user.* xattrs on symlinks
> and/or special files.
> 
> However, your virtiofs use case is unreasonable.

Ok. So we can merge this patch irrespective of the fact whether virtiofs
should make use of this mechanism or not, right?

> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> 
> As I have stated before, this introduces a breach in security.
> It allows an unprivileged process on the host to manipulate the
> security state of the guest. This is horribly wrong. It is not
> sufficient to claim that the breach requires misconfiguration
> to exploit. Don't do this.

So couple of things.

- Right now whole virtiofs model is relying on the fact that host
  unpriviliged users don't have access to shared directory. Otherwise
  guest process can simply drop a setuid root binary in shared directory
  and unpriviliged process can execute it and take over host system.

  So if virtiofs makes use of this mechanism, we are well with-in
  the existing constraints. If users don't follow the constraints,
  bad things can happen.

- I think Smalley provided a solution for your concern in other thread
  we discussed this issue.

  https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6


  "So for example if the host policy says that only virtiofsd can set
attributes on those files, then the guest MAC labels along with all
the other attributes are protected against tampering by any other
process on the host."

 Apart from hiding the shared directory from unpriviliged processes,
 we could design selinux policy in such a way that only virtiofsd
 is allowed "setattr". That should make sure even in case of
 misconfiguration, unprivileged process is not able to change
 guest security xattrs stored in "user.virtiofs.security.selinux".

 I think that sounds like a very reasonable proposition.

> 
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid).
> 
> Adding this mapping to virtiofs provides the breach for any
> LSM using xattrs.

I think both the points above answer this as well.

> 
> >  Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> virtiofs could use security.* or system.* xattrs instead of user.*
> xattrs. Don't use user.* xattrs.

So requirement is that every layer (host, guest and nested guest), will
have a separate security.selinux label and virtiofsd should be able
to retrieve/set any of the labels depending on access.

How do we achieve that with single security.selinux label per inode on host.

Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 17:05   ` Vivek Goyal
@ 2021-09-02 17:42     ` Vivek Goyal
  2021-09-02 18:55       ` Casey Schaufler
  0 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 17:42 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> > On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> > > Hi,
> > >
> > > This is V3 of the patch. Previous versions were posted here.
> > >
> > > v2:
> > > https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > > v1:
> > > https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> > > +m/
> > >
> > > Changes since v2
> > > ----------------
> > > - Do not call inode_permission() for special files as file mode bits
> > >   on these files represent permissions to read/write from/to device
> > >   and not necessarily permission to read/write xattrs. In this case
> > >   now user.* extended xattrs can be read/written on special files
> > >   as long as caller is owner of file or has CAP_FOWNER.
> > >  
> > > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> > >   Bruce Fields)
> > >
> > > - Fixed xfstest 062. Changed it to run only on older kernels where
> > >   user extended xattrs are not allowed on symlinks/special files. Added
> > >   a new replacement test 648 which does exactly what 062. Just that
> > >   it is supposed to run on newer kernels where user extended xattrs
> > >   are allowed on symlinks and special files. Will post patch in 
> > >   same thread (Ted Ts'o).
> > >
> > > Testing
> > > -------
> > > - Ran xfstest "./check -g auto" with and without patches and did not
> > >   notice any new failures.
> > >
> > > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> > >   filesystems and it works.
> > >  
> > > Description
> > > ===========
> > >
> > > Right now we don't allow setting user.* xattrs on symlinks and special
> > > files at all. Initially I thought that real reason behind this
> > > restriction is quota limitations but from last conversation it seemed
> > > that real reason is that permission bits on symlink and special files
> > > are special and different from regular files and directories, hence
> > > this restriction is in place. (I tested with xfs user quota enabled and
> > > quota restrictions kicked in on symlink).
> > >
> > > This version of patch allows reading/writing user.* xattr on symlink and
> > > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> > 
> > This part of your project makes perfect sense. There's no good
> > security reason that you shouldn't set user.* xattrs on symlinks
> > and/or special files.
> > 
> > However, your virtiofs use case is unreasonable.
> 
> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> should make use of this mechanism or not, right?
> 
> > 
> > > Who wants to set user.* xattr on symlink/special files
> > > -----------------------------------------------------
> > > I have primarily two users at this point of time.
> > >
> > > - virtiofs daemon.
> > >
> > > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> > >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> > >   should benefit from this change as well.
> > >
> > > Why virtiofsd wants to set user.* xattr on symlink/special files
> > > ----------------------------------------------------------------
> > > In virtiofs, actual file server is virtiosd daemon running on host.
> > > There we have a mode where xattrs can be remapped to something else.
> > > For example security.selinux can be remapped to
> > > user.virtiofsd.securit.selinux on the host.
> > 
> > As I have stated before, this introduces a breach in security.
> > It allows an unprivileged process on the host to manipulate the
> > security state of the guest. This is horribly wrong. It is not
> > sufficient to claim that the breach requires misconfiguration
> > to exploit. Don't do this.
> 
> So couple of things.
> 
> - Right now whole virtiofs model is relying on the fact that host
>   unpriviliged users don't have access to shared directory. Otherwise
>   guest process can simply drop a setuid root binary in shared directory
>   and unpriviliged process can execute it and take over host system.
> 
>   So if virtiofs makes use of this mechanism, we are well with-in
>   the existing constraints. If users don't follow the constraints,
>   bad things can happen.
> 
> - I think Smalley provided a solution for your concern in other thread
>   we discussed this issue.
> 
>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> 
> 
>   "So for example if the host policy says that only virtiofsd can set
> attributes on those files, then the guest MAC labels along with all
> the other attributes are protected against tampering by any other
> process on the host."
> 
>  Apart from hiding the shared directory from unpriviliged processes,
>  we could design selinux policy in such a way that only virtiofsd
>  is allowed "setattr". That should make sure even in case of
>  misconfiguration, unprivileged process is not able to change
>  guest security xattrs stored in "user.virtiofs.security.selinux".
> 
>  I think that sounds like a very reasonable proposition.
> 
> > 
> > > This remapping is useful when SELinux is enabled in guest and virtiofs
> > > as being used as rootfs. Guest and host SELinux policy might not match
> > > and host policy might deny security.selinux xattr setting by guest
> > > onto host. Or host might have SELinux disabled and in that case to
> > > be able to set security.selinux xattr, virtiofsd will need to have
> > > CAP_SYS_ADMIN (which we are trying to avoid).
> > 
> > Adding this mapping to virtiofs provides the breach for any
> > LSM using xattrs.
> 
> I think both the points above answer this as well.
> 
> > 
> > >  Being able to remap
> > > guest security.selinux (or other xattrs) on host to something else
> > > is also better from security point of view.
> > >
> > > But when we try this, we noticed that SELinux relabeling in guest
> > > is failing on some symlinks. When I debugged a little more, I
> > > came to know that "user.*" xattrs are not allowed on symlinks
> > > or special files.
> > >
> > > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > > allow virtiofs to arbitrarily remap guests's xattrs to something
> > > else on host and that solves this SELinux issue nicely and provides
> > > two SELinux policies (host and guest) to co-exist nicely without
> > > interfering with each other.
> > 
> > virtiofs could use security.* or system.* xattrs instead of user.*
> > xattrs. Don't use user.* xattrs.
> 
> So requirement is that every layer (host, guest and nested guest), will
> have a separate security.selinux label and virtiofsd should be able
> to retrieve/set any of the labels depending on access.
> 
> How do we achieve that with single security.selinux label per inode on host.

I guess we could think of using trusted.* but that requires CAP_SYS_ADMIN
in init_user_ns. And we wanted to retain capability to run virtiofsd
inside user namespace too. Also we wanted to give minimum required
capabilities to virtiofsd to reduce the risk what if virtiofsd gets
compromised. So amount of damage it can do to system is minimum.

So guest security.selinux xattr could potentially be mapped to.

trusted.virtiofs.security.selinux

nested guest selinux xattrs could be mapped to.

trusted.virtiofs.trusted.virtiofs.security.selinux

And given reading/setting trusted.* requires CAP_SYS_ADMIN, that means
unpriviliged processes can't change these security attributes of a
file.

And trade-off is that virtiofsd process needs to be given CAP_SYS_ADMIN.

Frankly speaking, we are more concerned about the security of host
system (as opposed to something changing in file for guest). So while
using "trusted.*" is still an option, I would think that not running
virtiofsd with CAP_SYS_ADMIN probably has its advantages too. On host
if we can just hide the shared dir from unpriviliged processes then
we get best of both the worlds. Unpriviliged processes can't change
anything on the shared file at the same time, possible damage by
virtiofsd is less if it gets compromised.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
                   ` (4 preceding siblings ...)
  2021-09-02 15:50 ` [PATCH 4/1] xfstest: Add a new test to test xattr operations Vivek Goyal
@ 2021-09-02 17:52 ` Andreas Gruenbacher
  2021-09-02 18:48   ` Vivek Goyal
  2021-09-06 14:39   ` Dr. David Alan Gilbert
  5 siblings, 2 replies; 39+ messages in thread
From: Andreas Gruenbacher @ 2021-09-02 17:52 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Alexander Viro, linux-fsdevel, LKML, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner

Hi,

On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> This is V3 of the patch. Previous versions were posted here.
>
> v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
>
> Changes since v2
> ----------------
> - Do not call inode_permission() for special files as file mode bits
>   on these files represent permissions to read/write from/to device
>   and not necessarily permission to read/write xattrs. In this case
>   now user.* extended xattrs can be read/written on special files
>   as long as caller is owner of file or has CAP_FOWNER.
>
> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>   Bruce Fields)
>
> - Fixed xfstest 062. Changed it to run only on older kernels where
>   user extended xattrs are not allowed on symlinks/special files. Added
>   a new replacement test 648 which does exactly what 062. Just that
>   it is supposed to run on newer kernels where user extended xattrs
>   are allowed on symlinks and special files. Will post patch in
>   same thread (Ted Ts'o).
>
> Testing
> -------
> - Ran xfstest "./check -g auto" with and without patches and did not
>   notice any new failures.
>
> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>   filesystems and it works.
>
> Description
> ===========
>
> Right now we don't allow setting user.* xattrs on symlinks and special
> files at all. Initially I thought that real reason behind this
> restriction is quota limitations but from last conversation it seemed
> that real reason is that permission bits on symlink and special files
> are special and different from regular files and directories, hence
> this restriction is in place. (I tested with xfs user quota enabled and
> quota restrictions kicked in on symlink).
>
> This version of patch allows reading/writing user.* xattr on symlink and
> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

the idea behind user.* xattrs is that they behave similar to file
contents as far as permissions go. It follows from that that symlinks
and special files cannot have user.* xattrs. This has been the model
for many years now and applications may be expecting these semantics,
so we cannot simply change the behavior. So NACK from me.

> Who wants to set user.* xattr on symlink/special files
> -----------------------------------------------------
> I have primarily two users at this point of time.
>
> - virtiofs daemon.
>
> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>   should benefit from this change as well.
>
> Why virtiofsd wants to set user.* xattr on symlink/special files
> ----------------------------------------------------------------
> In virtiofs, actual file server is virtiosd daemon running on host.
> There we have a mode where xattrs can be remapped to something else.
> For example security.selinux can be remapped to
> user.virtiofsd.securit.selinux on the host.
>
> This remapping is useful when SELinux is enabled in guest and virtiofs
> as being used as rootfs. Guest and host SELinux policy might not match
> and host policy might deny security.selinux xattr setting by guest
> onto host. Or host might have SELinux disabled and in that case to
> be able to set security.selinux xattr, virtiofsd will need to have
> CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> guest security.selinux (or other xattrs) on host to something else
> is also better from security point of view.
>
> But when we try this, we noticed that SELinux relabeling in guest
> is failing on some symlinks. When I debugged a little more, I
> came to know that "user.*" xattrs are not allowed on symlinks
> or special files.
>
> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> allow virtiofs to arbitrarily remap guests's xattrs to something
> else on host and that solves this SELinux issue nicely and provides
> two SELinux policies (host and guest) to co-exist nicely without
> interfering with each other.

The fact that user.* xattrs don't work in this remapping scenario
should have told you that you're doing things wrong; the user.*
namespace seriously was never meant to be abused in this way.

You may be able to get away with using trusted.* xattrs which support
roughly the kind of daemon use I think you're talking about here, but
I'm not sure selinux will be happy with labels that aren't fully under
its own control. I really wonder why this wasn't obvious enough.

Thanks,
Andreas

> Thanks
> Vivek
>
> Vivek Goyal (1):
>   xattr: Allow user.* xattr on symlink and special files
>
>  fs/xattr.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> --
> 2.31.1
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 17:52 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Andreas Gruenbacher
@ 2021-09-02 18:48   ` Vivek Goyal
  2021-09-02 19:19     ` Casey Schaufler
  2021-09-06 14:39   ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 18:48 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Alexander Viro, linux-fsdevel, LKML, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner

On Thu, Sep 02, 2021 at 07:52:41PM +0200, Andreas Gruenbacher wrote:
> Hi,
> 
> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> the idea behind user.* xattrs is that they behave similar to file
> contents as far as permissions go. It follows from that that symlinks
> and special files cannot have user.* xattrs. This has been the model
> for many years now and applications may be expecting these semantics,
> so we cannot simply change the behavior. So NACK from me.

Directories with sticky bit break this general rule and don't follow
permission bit model.

man xattr says.

*****************************************************************
and access to user extended  attributes  is  re‐
       stricted  to  the  owner and to users with appropriate capabilities for
       directories with the sticky bit set
******************************************************************

So why not allow similar exceptions for symlinks and device files.

I can understand the concern about behavior change suddenly and
applications being surprised. If that's the only concern we could
think of making user opt-in for this new behavior based on a kernel
CONFIG, kernel command line or something else.


> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> >
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> The fact that user.* xattrs don't work in this remapping scenario
> should have told you that you're doing things wrong; the user.*
> namespace seriously was never meant to be abused in this way.

Guest's security label is not be parsed by host kernel. Host kernel
will have its own security label and will take decisions based on
that. In that aspect making use of "user.*" xattr seemed to make
lot of sense and we were wondering why user.* xattr is limited to
regualr files and directories only and can we change that behavior.

> 
> You may be able to get away with using trusted.* xattrs which support
> roughly the kind of daemon use I think you're talking about here, but
> I'm not sure selinux will be happy with labels that aren't fully under
> its own control. I really wonder why this wasn't obvious enough.

I guess trusted.* will do same thing. But it requires CAP_SYS_ADMIN
in init_user_ns. And that rules out running virtiofsd unpriviliged
or inside a user namespace. Also it reduces the risk posted by
virtiofsd on host filesystem due to CAP_SYS_ADMIN. That's why we
were trying to steer clear of trusted.* xattr space.

Also, trusted.* xattr space does not work with NFS.

$ setfattr -n "trusted.virtiofs" -v "foo" test.txt
setfattr: test.txt: Operation not supported

We want to be able run virtiofsd over NFS mounted dir too.

So its not that we did not consider trusted.* xattrs. We ran
into above issues.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 17:42     ` Vivek Goyal
@ 2021-09-02 18:55       ` Casey Schaufler
  2021-09-02 20:06         ` Vivek Goyal
  0 siblings, 1 reply; 39+ messages in thread
From: Casey Schaufler @ 2021-09-02 18:55 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, Casey Schaufler

On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>> Hi,
>>>>
>>>> This is V3 of the patch. Previous versions were posted here.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>> v1:
>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>> +m/
>>>>
>>>> Changes since v2
>>>> ----------------
>>>> - Do not call inode_permission() for special files as file mode bits
>>>>   on these files represent permissions to read/write from/to device
>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>   now user.* extended xattrs can be read/written on special files
>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>  
>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>   Bruce Fields)
>>>>
>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>   same thread (Ted Ts'o).
>>>>
>>>> Testing
>>>> -------
>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>   notice any new failures.
>>>>
>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>   filesystems and it works.
>>>>  
>>>> Description
>>>> ===========
>>>>
>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>> files at all. Initially I thought that real reason behind this
>>>> restriction is quota limitations but from last conversation it seemed
>>>> that real reason is that permission bits on symlink and special files
>>>> are special and different from regular files and directories, hence
>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>> quota restrictions kicked in on symlink).
>>>>
>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>> This part of your project makes perfect sense. There's no good
>>> security reason that you shouldn't set user.* xattrs on symlinks
>>> and/or special files.
>>>
>>> However, your virtiofs use case is unreasonable.
>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>> should make use of this mechanism or not, right?

I don't see a security objection. I did see that Andreas Gruenbacher
<agruenba@redhat.com> has objections to the behavior.


>>>> Who wants to set user.* xattr on symlink/special files
>>>> -----------------------------------------------------
>>>> I have primarily two users at this point of time.
>>>>
>>>> - virtiofs daemon.
>>>>
>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>   should benefit from this change as well.
>>>>
>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>> ----------------------------------------------------------------
>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>> There we have a mode where xattrs can be remapped to something else.
>>>> For example security.selinux can be remapped to
>>>> user.virtiofsd.securit.selinux on the host.
>>> As I have stated before, this introduces a breach in security.
>>> It allows an unprivileged process on the host to manipulate the
>>> security state of the guest. This is horribly wrong. It is not
>>> sufficient to claim that the breach requires misconfiguration
>>> to exploit. Don't do this.
>> So couple of things.
>>
>> - Right now whole virtiofs model is relying on the fact that host
>>   unpriviliged users don't have access to shared directory. Otherwise
>>   guest process can simply drop a setuid root binary in shared directory
>>   and unpriviliged process can execute it and take over host system.
>>
>>   So if virtiofs makes use of this mechanism, we are well with-in
>>   the existing constraints. If users don't follow the constraints,
>>   bad things can happen.
>>
>> - I think Smalley provided a solution for your concern in other thread
>>   we discussed this issue.
>>
>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>
>>
>>   "So for example if the host policy says that only virtiofsd can set
>> attributes on those files, then the guest MAC labels along with all
>> the other attributes are protected against tampering by any other
>> process on the host."

You can't count on SELinux policy to address the issue on a
system running Smack. Or any other user of system.* xattrs,
be they in the kernel or user space. You can't even count on
SELinux policy to be correct. virtiofs has to present a "safe"
situation regardless of how security.* xattrs are used and
regardless of which, if any, LSMs are configured. You can't
do that with user.* attributes.


>>
>>  Apart from hiding the shared directory from unpriviliged processes,
>>  we could design selinux policy in such a way that only virtiofsd
>>  is allowed "setattr". That should make sure even in case of
>>  misconfiguration, unprivileged process is not able to change
>>  guest security xattrs stored in "user.virtiofs.security.selinux".
>>
>>  I think that sounds like a very reasonable proposition.
>>
>>>> This remapping is useful when SELinux is enabled in guest and virtiofs
>>>> as being used as rootfs. Guest and host SELinux policy might not match
>>>> and host policy might deny security.selinux xattr setting by guest
>>>> onto host. Or host might have SELinux disabled and in that case to
>>>> be able to set security.selinux xattr, virtiofsd will need to have
>>>> CAP_SYS_ADMIN (which we are trying to avoid).
>>> Adding this mapping to virtiofs provides the breach for any
>>> LSM using xattrs.
>> I think both the points above answer this as well.
>>
>>>>  Being able to remap
>>>> guest security.selinux (or other xattrs) on host to something else
>>>> is also better from security point of view.
>>>>
>>>> But when we try this, we noticed that SELinux relabeling in guest
>>>> is failing on some symlinks. When I debugged a little more, I
>>>> came to know that "user.*" xattrs are not allowed on symlinks
>>>> or special files.
>>>>
>>>> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
>>>> allow virtiofs to arbitrarily remap guests's xattrs to something
>>>> else on host and that solves this SELinux issue nicely and provides
>>>> two SELinux policies (host and guest) to co-exist nicely without
>>>> interfering with each other.
>>> virtiofs could use security.* or system.* xattrs instead of user.*
>>> xattrs. Don't use user.* xattrs.
>> So requirement is that every layer (host, guest and nested guest), will
>> have a separate security.selinux label and virtiofsd should be able
>> to retrieve/set any of the labels depending on access.
>>
>> How do we achieve that with single security.selinux label per inode on host.
> I guess we could think of using trusted.* but that requires CAP_SYS_ADMIN
> in init_user_ns. And we wanted to retain capability to run virtiofsd
> inside user namespace too. Also we wanted to give minimum required
> capabilities to virtiofsd to reduce the risk what if virtiofsd gets
> compromised. So amount of damage it can do to system is minimum.
>
> So guest security.selinux xattr could potentially be mapped to.
>
> trusted.virtiofs.security.selinux
>
> nested guest selinux xattrs could be mapped to.
>
> trusted.virtiofs.trusted.virtiofs.security.selinux
>
> And given reading/setting trusted.* requires CAP_SYS_ADMIN, that means
> unpriviliged processes can't change these security attributes of a
> file.
>
> And trade-off is that virtiofsd process needs to be given CAP_SYS_ADMIN.
>
> Frankly speaking, we are more concerned about the security of host
> system (as opposed to something changing in file for guest). So while
> using "trusted.*" is still an option, I would think that not running
> virtiofsd with CAP_SYS_ADMIN probably has its advantages too. On host
> if we can just hide the shared dir from unpriviliged processes then
> we get best of both the worlds. Unpriviliged processes can't change
> anything on the shared file at the same time, possible damage by
> virtiofsd is less if it gets compromised.
>
> Thanks
> Vivek
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 18:48   ` Vivek Goyal
@ 2021-09-02 19:19     ` Casey Schaufler
  0 siblings, 0 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-09-02 19:19 UTC (permalink / raw)
  To: Vivek Goyal, Andreas Gruenbacher
  Cc: Alexander Viro, linux-fsdevel, LKML, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner, Casey Schaufler

On 9/2/2021 11:48 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 07:52:41PM +0200, Andreas Gruenbacher wrote:
>> Hi,
>>
>> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>> This is V3 of the patch. Previous versions were posted here.
>>>
>>> v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>> v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
>>>
>>> Changes since v2
>>> ----------------
>>> - Do not call inode_permission() for special files as file mode bits
>>>   on these files represent permissions to read/write from/to device
>>>   and not necessarily permission to read/write xattrs. In this case
>>>   now user.* extended xattrs can be read/written on special files
>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>
>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>   Bruce Fields)
>>>
>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>   a new replacement test 648 which does exactly what 062. Just that
>>>   it is supposed to run on newer kernels where user extended xattrs
>>>   are allowed on symlinks and special files. Will post patch in
>>>   same thread (Ted Ts'o).
>>>
>>> Testing
>>> -------
>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>   notice any new failures.
>>>
>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>   filesystems and it works.
>>>
>>> Description
>>> ===========
>>>
>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>> files at all. Initially I thought that real reason behind this
>>> restriction is quota limitations but from last conversation it seemed
>>> that real reason is that permission bits on symlink and special files
>>> are special and different from regular files and directories, hence
>>> this restriction is in place. (I tested with xfs user quota enabled and
>>> quota restrictions kicked in on symlink).
>>>
>>> This version of patch allows reading/writing user.* xattr on symlink and
>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>> the idea behind user.* xattrs is that they behave similar to file
>> contents as far as permissions go. It follows from that that symlinks
>> and special files cannot have user.* xattrs. This has been the model
>> for many years now and applications may be expecting these semantics,
>> so we cannot simply change the behavior. So NACK from me.
> Directories with sticky bit break this general rule and don't follow
> permission bit model.

The sticky bit is a hack. It was introduced to stave off proposed
implementations of Access Control Lists, which it did successfully
for quite some time.

> man xattr says.
>
> *****************************************************************
> and access to user extended  attributes  is  re‐
>        stricted  to  the  owner and to users with appropriate capabilities for
>        directories with the sticky bit set
> ******************************************************************
>
> So why not allow similar exceptions for symlinks and device files.

Limiting exceptions is usually a good thing. If every system mechanism
devolves into a heap of special cases it becomes very difficult to
describe your system semantics or the system security model. 

> I can understand the concern about behavior change suddenly and
> applications being surprised. If that's the only concern we could
> think of making user opt-in for this new behavior based on a kernel
> CONFIG, kernel command line or something else.

That doesn't work in the world of distros. But you knew that.


>>> Who wants to set user.* xattr on symlink/special files
>>> -----------------------------------------------------
>>> I have primarily two users at this point of time.
>>>
>>> - virtiofs daemon.
>>>
>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>   should benefit from this change as well.
>>>
>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>> ----------------------------------------------------------------
>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>> There we have a mode where xattrs can be remapped to something else.
>>> For example security.selinux can be remapped to
>>> user.virtiofsd.securit.selinux on the host.
>>>
>>> This remapping is useful when SELinux is enabled in guest and virtiofs
>>> as being used as rootfs. Guest and host SELinux policy might not match
>>> and host policy might deny security.selinux xattr setting by guest
>>> onto host. Or host might have SELinux disabled and in that case to
>>> be able to set security.selinux xattr, virtiofsd will need to have
>>> CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
>>> guest security.selinux (or other xattrs) on host to something else
>>> is also better from security point of view.
>>>
>>> But when we try this, we noticed that SELinux relabeling in guest
>>> is failing on some symlinks. When I debugged a little more, I
>>> came to know that "user.*" xattrs are not allowed on symlinks
>>> or special files.
>>>
>>> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
>>> allow virtiofs to arbitrarily remap guests's xattrs to something
>>> else on host and that solves this SELinux issue nicely and provides
>>> two SELinux policies (host and guest) to co-exist nicely without
>>> interfering with each other.
>> The fact that user.* xattrs don't work in this remapping scenario
>> should have told you that you're doing things wrong; the user.*
>> namespace seriously was never meant to be abused in this way.
> Guest's security label is not be parsed by host kernel. Host kernel
> will have its own security label and will take decisions based on
> that. In that aspect making use of "user.*" xattr seemed to make
> lot of sense

It doesn't make sense. For files, directories or anything. It's
freaking hazardous.

>  and we were wondering why user.* xattr is limited to
> regualr files and directories only and can we change that behavior.
>
>> You may be able to get away with using trusted.* xattrs which support
>> roughly the kind of daemon use I think you're talking about here, but
>> I'm not sure selinux will be happy with labels that aren't fully under
>> its own control. I really wonder why this wasn't obvious enough.
> I guess trusted.* will do same thing. But it requires CAP_SYS_ADMIN
> in init_user_ns.

Right. That's because you're doing dangerous things.

>  And that rules out running virtiofsd unpriviliged

Right. That's because you're doing dangerous things.

> or inside a user namespace. Also it reduces the risk posted by
> virtiofsd on host filesystem due to CAP_SYS_ADMIN. That's why we
> were trying to steer clear of trusted.* xattr space.

Yeah, I get it. What's wrong with admitting that what you're
trying to do is dangerous, and that you have to be careful?

> Also, trusted.* xattr space does not work with NFS.

So, fix that?

>
> $ setfattr -n "trusted.virtiofs" -v "foo" test.txt
> setfattr: test.txt: Operation not supported
>
> We want to be able run virtiofsd over NFS mounted dir too.
>
> So its not that we did not consider trusted.* xattrs. We ran
> into above issues.
>
> Thanks
> Vivek
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 18:55       ` Casey Schaufler
@ 2021-09-02 20:06         ` Vivek Goyal
  2021-09-02 22:34           ` Casey Schaufler
  0 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-02 20:06 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>> Hi,
> >>>>
> >>>> This is V3 of the patch. Previous versions were posted here.
> >>>>
> >>>> v2:
> >>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>> v1:
> >>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>> +m/
> >>>>
> >>>> Changes since v2
> >>>> ----------------
> >>>> - Do not call inode_permission() for special files as file mode bits
> >>>>   on these files represent permissions to read/write from/to device
> >>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>   now user.* extended xattrs can be read/written on special files
> >>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>  
> >>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>   Bruce Fields)
> >>>>
> >>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>   same thread (Ted Ts'o).
> >>>>
> >>>> Testing
> >>>> -------
> >>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>   notice any new failures.
> >>>>
> >>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>   filesystems and it works.
> >>>>  
> >>>> Description
> >>>> ===========
> >>>>
> >>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>> files at all. Initially I thought that real reason behind this
> >>>> restriction is quota limitations but from last conversation it seemed
> >>>> that real reason is that permission bits on symlink and special files
> >>>> are special and different from regular files and directories, hence
> >>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>> quota restrictions kicked in on symlink).
> >>>>
> >>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>> This part of your project makes perfect sense. There's no good
> >>> security reason that you shouldn't set user.* xattrs on symlinks
> >>> and/or special files.
> >>>
> >>> However, your virtiofs use case is unreasonable.
> >> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >> should make use of this mechanism or not, right?
> 
> I don't see a security objection. I did see that Andreas Gruenbacher
> <agruenba@redhat.com> has objections to the behavior.
> 
> 
> >>>> Who wants to set user.* xattr on symlink/special files
> >>>> -----------------------------------------------------
> >>>> I have primarily two users at this point of time.
> >>>>
> >>>> - virtiofs daemon.
> >>>>
> >>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>   should benefit from this change as well.
> >>>>
> >>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>> ----------------------------------------------------------------
> >>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>> There we have a mode where xattrs can be remapped to something else.
> >>>> For example security.selinux can be remapped to
> >>>> user.virtiofsd.securit.selinux on the host.
> >>> As I have stated before, this introduces a breach in security.
> >>> It allows an unprivileged process on the host to manipulate the
> >>> security state of the guest. This is horribly wrong. It is not
> >>> sufficient to claim that the breach requires misconfiguration
> >>> to exploit. Don't do this.
> >> So couple of things.
> >>
> >> - Right now whole virtiofs model is relying on the fact that host
> >>   unpriviliged users don't have access to shared directory. Otherwise
> >>   guest process can simply drop a setuid root binary in shared directory
> >>   and unpriviliged process can execute it and take over host system.
> >>
> >>   So if virtiofs makes use of this mechanism, we are well with-in
> >>   the existing constraints. If users don't follow the constraints,
> >>   bad things can happen.
> >>
> >> - I think Smalley provided a solution for your concern in other thread
> >>   we discussed this issue.
> >>
> >>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>
> >>
> >>   "So for example if the host policy says that only virtiofsd can set
> >> attributes on those files, then the guest MAC labels along with all
> >> the other attributes are protected against tampering by any other
> >> process on the host."
> 
> You can't count on SELinux policy to address the issue on a
> system running Smack.
> Or any other user of system.* xattrs,
> be they in the kernel or user space. You can't even count on
> SELinux policy to be correct. virtiofs has to present a "safe"
> situation regardless of how security.* xattrs are used and
> regardless of which, if any, LSMs are configured. You can't
> do that with user.* attributes.

Lets take a step back. Your primary concern with using user.* xattrs
by virtiofsd is that it can be modified by unprivileged users on host.
And our solution to that problem is hide shared directory from
unprivileged users.

In addition to that, LSMs on host can block setting "user.*" xattrs by
virtiofsd domain only for additional protection. If LSMs are not configured,
then hiding the directory is the solution.

So why that's not a solution and only relying on CAP_SYS_ADMIN is the
solution. I don't understand that part.

Also if directory is not hidden, unprivileged users can change file
data and other metadata. Why that's not a concern and why there is
so much of focus only security xattr. If you were to block modification
of file then you will have rely on LSMs. And if LSMs are not configured,
then we will rely on shared directory not being visible.

Can you please help me understand why hiding shared directory from
unprivileged users is not a solution (With both LSMs configured or
not configured on host). That's a requirement for virtiofs anyway. 
And if we agree on that, then I don't see why using "user.*" xattrs
for storing guest sercurity attributes is a problem.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 20:06         ` Vivek Goyal
@ 2021-09-02 22:34           ` Casey Schaufler
  2021-09-03 15:26             ` Vivek Goyal
                               ` (2 more replies)
  0 siblings, 3 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-09-02 22:34 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, Casey Schaufler

On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
>> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
>>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This is V3 of the patch. Previous versions were posted here.
>>>>>>
>>>>>> v2:
>>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>>>> v1:
>>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>>>> +m/
>>>>>>
>>>>>> Changes since v2
>>>>>> ----------------
>>>>>> - Do not call inode_permission() for special files as file mode bits
>>>>>>   on these files represent permissions to read/write from/to device
>>>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>>>   now user.* extended xattrs can be read/written on special files
>>>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>>>  
>>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>>>   Bruce Fields)
>>>>>>
>>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>>>   same thread (Ted Ts'o).
>>>>>>
>>>>>> Testing
>>>>>> -------
>>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>>>   notice any new failures.
>>>>>>
>>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>>>   filesystems and it works.
>>>>>>  
>>>>>> Description
>>>>>> ===========
>>>>>>
>>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>>>> files at all. Initially I thought that real reason behind this
>>>>>> restriction is quota limitations but from last conversation it seemed
>>>>>> that real reason is that permission bits on symlink and special files
>>>>>> are special and different from regular files and directories, hence
>>>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>>>> quota restrictions kicked in on symlink).
>>>>>>
>>>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>>>> This part of your project makes perfect sense. There's no good
>>>>> security reason that you shouldn't set user.* xattrs on symlinks
>>>>> and/or special files.
>>>>>
>>>>> However, your virtiofs use case is unreasonable.
>>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>>>> should make use of this mechanism or not, right?
>> I don't see a security objection. I did see that Andreas Gruenbacher
>> <agruenba@redhat.com> has objections to the behavior.
>>
>>
>>>>>> Who wants to set user.* xattr on symlink/special files
>>>>>> -----------------------------------------------------
>>>>>> I have primarily two users at this point of time.
>>>>>>
>>>>>> - virtiofs daemon.
>>>>>>
>>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>>>   should benefit from this change as well.
>>>>>>
>>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>>>> ----------------------------------------------------------------
>>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>>>> There we have a mode where xattrs can be remapped to something else.
>>>>>> For example security.selinux can be remapped to
>>>>>> user.virtiofsd.securit.selinux on the host.
>>>>> As I have stated before, this introduces a breach in security.
>>>>> It allows an unprivileged process on the host to manipulate the
>>>>> security state of the guest. This is horribly wrong. It is not
>>>>> sufficient to claim that the breach requires misconfiguration
>>>>> to exploit. Don't do this.
>>>> So couple of things.
>>>>
>>>> - Right now whole virtiofs model is relying on the fact that host
>>>>   unpriviliged users don't have access to shared directory. Otherwise
>>>>   guest process can simply drop a setuid root binary in shared directory
>>>>   and unpriviliged process can execute it and take over host system.
>>>>
>>>>   So if virtiofs makes use of this mechanism, we are well with-in
>>>>   the existing constraints. If users don't follow the constraints,
>>>>   bad things can happen.
>>>>
>>>> - I think Smalley provided a solution for your concern in other thread
>>>>   we discussed this issue.
>>>>
>>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>>>
>>>>
>>>>   "So for example if the host policy says that only virtiofsd can set
>>>> attributes on those files, then the guest MAC labels along with all
>>>> the other attributes are protected against tampering by any other
>>>> process on the host."
>> You can't count on SELinux policy to address the issue on a
>> system running Smack.
>> Or any other user of system.* xattrs,
>> be they in the kernel or user space. You can't even count on
>> SELinux policy to be correct. virtiofs has to present a "safe"
>> situation regardless of how security.* xattrs are used and
>> regardless of which, if any, LSMs are configured. You can't
>> do that with user.* attributes.
> Lets take a step back. Your primary concern with using user.* xattrs
> by virtiofsd is that it can be modified by unprivileged users on host.
> And our solution to that problem is hide shared directory from
> unprivileged users.

You really don't see how fragile that is, do you? How a single
errant call to rename(), chmod() or chown() on the host can expose
the entire guest to exploitation. That's not even getting into
the bag of mount() tricks.


> In addition to that, LSMs on host can block setting "user.*" xattrs by
> virtiofsd domain only for additional protection.

Try thinking outside the SELinux box briefly, if you possibly can.
An LSM that implements just Bell & LaPadula isn't going to have a
"virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
distinguish writing user xattrs from writing other file attributes
in policy. Your argument requires a fine grained policy a'la SELinux.
And an application specific SELinux policy at that.

>  If LSMs are not configured,
> then hiding the directory is the solution.

It's not a solution at all. It's wishful thinking that
some admin is going to do absolutely everything right, will
never make a mistake and will never, ever, read the mount(2)
man page.

> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> solution. I don't understand that part.

It comes back to your design, which is fundamentally flawed. You
can't store system security information in an attribute that can
be manipulated by untrusted entities. That's why we have system.*
xattrs. You want to have an attribute on the host that maps to a
security attribute on the guest. The host has to protect the attribute
on the guest with mechanisms of comparable strength as the guest's
mechanisms. Otherwise you can't trust the guest with host data.

It's a real shame that CAP_SYS_ADMIN is so scary. The capability
mechanism as implemented today won't scale to the hundreds of individual
capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
I'm not convinced that there isn't a way to accomplish what you're
trying to do without privilege, but this isn't it, and I don't know
what is. Sorry.

> Also if directory is not hidden, unprivileged users can change file
> data and other metadata.

I assumed that you've taken that into account. Are you saying that
isn't going to be done correctly either?

>  Why that's not a concern and why there is
> so much of focus only security xattr.

As with an NFS mount, the assumption is that UID 567 (or its magically
mapped equivalent) has the same access rights on both the server/host
and client/guest. I'm not worried about the mode bits because they are
presented consistently on both machines. If, on the other hand, an
attribute used to determine access is security.esprit on the guest and
user.security.esprit on the host, the unprivileged user on the host
can defeat the privilege requirements on the guest. That's why.

>  If you were to block modification
> of file then you will have rely on LSMs.

No. We're talking about the semantics of the xattr namespaces.
LSMs can further constrain access to xattrs, but the basic rules
of access to the user.* and security.* attributes are different
in any case. This is by design.

>  And if LSMs are not configured,
> then we will rely on shared directory not being visible.

LSMs are not the problem. LSMs use security.* xattrs, which is why
they come up in the discussion.

> Can you please help me understand why hiding shared directory from
> unprivileged users is not a solution

Maybe you can describe the mechanism you use to "hide" a shared directory
on the host. If the filesystem is mounted on the host it seems unlikely
that you can provide a convincing argument for sufficient protection.

>  (With both LSMs configured or
> not configured on host). That's a requirement for virtiofs anyway. 
> And if we agree on that, then I don't see why using "user.*" xattrs
> for storing guest sercurity attributes is a problem.
>
> Thanks
> Vivek
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
@ 2021-09-03  4:55   ` Dave Chinner
  2021-09-03  6:31   ` Andreas Gruenbacher
  2021-09-03  6:31   ` Zorro Lang
  2 siblings, 0 replies; 39+ messages in thread
From: Dave Chinner @ 2021-09-03  4:55 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: fstests, linux-fsdevel, linux-kernel, virtio-fs, dwalsh,
	dgilbert, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, viro

On Thu, Sep 02, 2021 at 11:47:31AM -0400, Vivek Goyal wrote:
> 
> xfstests: generic/062: Do not run on newer kernels
> 
> This test has been written with assumption that setting user.* xattrs will
> fail on symlink and special files. When newer kernels support setting
> user.* xattrs on symlink and special files, this test starts failing.
> 
> Found it hard to change test in such a way that it works on both type of
> kernels. Primary problem is 062.out file which hardcodes the output and
> output will be different on old and new kernels.
> 
> So instead, do not run this test if kernel is new and is expected to
> exhibit new behavior. Next patch will create a new test and run that
> test on new kernel.
> 
> IOW, on old kernels run 062 and on new kernels run new test.
> 
> This is a proposed patch. Will need to be fixed if corresponding
> kernel changes are merged upstream.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tests/generic/062 |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> Index: xfstests-dev/tests/generic/062
> ===================================================================
> --- xfstests-dev.orig/tests/generic/062	2021-08-31 15:51:08.160307982 -0400
> +++ xfstests-dev/tests/generic/062	2021-08-31 16:27:41.678307982 -0400
> @@ -55,6 +55,26 @@ _require_attrs
>  _require_symlinks
>  _require_mknod
>  
> +user_xattr_allowed()
> +{
> +	local kernel_version kernel_patchlevel
> +
> +	kernel_version=`uname -r | awk -F. '{print $1}'`
> +	kernel_patchlevel=`uname -r | awk -F. '{print $2}'`
> +
> +	# Kernel version 5.14 onwards allow user xattr on symlink/special files.
> +	[ $kernel_version -lt 5 ] && return 1
> +	[ $kernel_patchlevel -lt 14 ] && return 1
> +	return 0;
> +}

We don't do this because code changes get backported to random
kernels and so the kernel release is not a reliable indicator of
feature support.

Probing the functionality is the only way to reliably detect what a
kernel supports. That's what we don in all the _requires*()
functions, which is what this should all be wrapped in.

> +# Kernel version 5.14 onwards allow user xattr on symlink/special files.
> +# Do not run this test on newer kernels. Instead run the new test
> +# which has been written with the assumption that user.* xattr
> +# will succeed on symlink and special files.
> +user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead."

"run a newer test instead" is not a useful error message. Nor do you
need "skipping this test" - that's exactly what "notrun" means.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
  2021-09-03  4:55   ` Dave Chinner
@ 2021-09-03  6:31   ` Andreas Gruenbacher
  2021-09-03  6:56     ` Andreas Gruenbacher
  2021-09-03  6:31   ` Zorro Lang
  2 siblings, 1 reply; 39+ messages in thread
From: Andreas Gruenbacher @ 2021-09-03  6:31 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: fstests, linux-fsdevel, LKML, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner, Alexander Viro

On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> xfstests: generic/062: Do not run on newer kernels
>
> This test has been written with assumption that setting user.* xattrs will
> fail on symlink and special files. When newer kernels support setting
> user.* xattrs on symlink and special files, this test starts failing.

It's actually a good thing that this test case triggers for the kernel
change you're proposing; that change should never be merged. The
user.* namespace is meant for data with the same access permissions as
the file data, and it has been for many years. We may have
applications that assume the existing behavior. In addition, this
change would create backwards compatibility problems for things like
backups.

I'm not convinced that what you're actually proposing (mapping
security.selinux to a different attribute name) actually makes sense,
but that's a question for the selinux folks to decide. Mapping it to a
user.* attribute is definitely wrong though. The modified behavior
would affect anybody, not only users of selinux and/or virtiofs. If
mapping attribute names is actually the right approach, then you need
to look at trusted.* xattrs, which exist specifically for this kind of
purpose. You've noted that trusted.* xattrs aren't supported over nfs.
That's unfortunate, but not an acceptable excuse for messing up user.*
xattrs.

Thanks,
Andreas


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
  2021-09-03  4:55   ` Dave Chinner
  2021-09-03  6:31   ` Andreas Gruenbacher
@ 2021-09-03  6:31   ` Zorro Lang
  2 siblings, 0 replies; 39+ messages in thread
From: Zorro Lang @ 2021-09-03  6:31 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: fstests, linux-fsdevel, linux-kernel, virtio-fs, dwalsh,
	dgilbert, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, david, viro

On Thu, Sep 02, 2021 at 11:47:31AM -0400, Vivek Goyal wrote:
> 
> xfstests: generic/062: Do not run on newer kernels
> 
> This test has been written with assumption that setting user.* xattrs will
> fail on symlink and special files. When newer kernels support setting
> user.* xattrs on symlink and special files, this test starts failing.
> 
> Found it hard to change test in such a way that it works on both type of
> kernels. Primary problem is 062.out file which hardcodes the output and
> output will be different on old and new kernels.
> 
> So instead, do not run this test if kernel is new and is expected to
> exhibit new behavior. Next patch will create a new test and run that
> test on new kernel.
> 
> IOW, on old kernels run 062 and on new kernels run new test.
> 
> This is a proposed patch. Will need to be fixed if corresponding
> kernel changes are merged upstream.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tests/generic/062 |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> Index: xfstests-dev/tests/generic/062
> ===================================================================
> --- xfstests-dev.orig/tests/generic/062	2021-08-31 15:51:08.160307982 -0400
> +++ xfstests-dev/tests/generic/062	2021-08-31 16:27:41.678307982 -0400
> @@ -55,6 +55,26 @@ _require_attrs
>  _require_symlinks
>  _require_mknod
>  
> +user_xattr_allowed()
> +{
> +	local kernel_version kernel_patchlevel
> +
> +	kernel_version=`uname -r | awk -F. '{print $1}'`
> +	kernel_patchlevel=`uname -r | awk -F. '{print $2}'`
> +
> +	# Kernel version 5.14 onwards allow user xattr on symlink/special files.
> +	[ $kernel_version -lt 5 ] && return 1
> +	[ $kernel_patchlevel -lt 14 ] && return 1
> +	return 0;
> +}

I don't think this's a good way to judge if run or notrun a test. Many downstream
kernels always backport upstream features. I can't say what's the best way to
deal with this thing, I only can provide two optional methods:

1) Add new requre_* helpers to check if current kernel support to set xattr on
symlink and special files, then let this case only run on support/unsupport
condition.

2) Use _link_out_file() to link the .out file to different golden images (refer to
generic/050 etc), according to different feature implementation.

If anyone has a better method, feel free to talk :)

Thanks,
Zorro

> +
> +
> +# Kernel version 5.14 onwards allow user xattr on symlink/special files.
> +# Do not run this test on newer kernels. Instead run the new test
> +# which has been written with the assumption that user.* xattr
> +# will succeed on symlink and special files.
> +user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead."
> +
>  rm -f $tmp.backup1 $tmp.backup2 $seqres.full
>  
>  # real QA test starts here
> 


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03  6:31   ` Andreas Gruenbacher
@ 2021-09-03  6:56     ` Andreas Gruenbacher
  2021-09-03 14:42       ` Bruce Fields
  0 siblings, 1 reply; 39+ messages in thread
From: Andreas Gruenbacher @ 2021-09-03  6:56 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: fstests, linux-fsdevel, LKML, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner, Alexander Viro

On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > xfstests: generic/062: Do not run on newer kernels
> >
> > This test has been written with assumption that setting user.* xattrs will
> > fail on symlink and special files. When newer kernels support setting
> > user.* xattrs on symlink and special files, this test starts failing.
>
> It's actually a good thing that this test case triggers for the kernel
> change you're proposing; that change should never be merged. The
> user.* namespace is meant for data with the same access permissions as
> the file data, and it has been for many years. We may have
> applications that assume the existing behavior. In addition, this
> change would create backwards compatibility problems for things like
> backups.
>
> I'm not convinced that what you're actually proposing (mapping
> security.selinux to a different attribute name) actually makes sense,
> but that's a question for the selinux folks to decide. Mapping it to a
> user.* attribute is definitely wrong though. The modified behavior
> would affect anybody, not only users of selinux and/or virtiofs. If
> mapping attribute names is actually the right approach, then you need
> to look at trusted.* xattrs, which exist specifically for this kind of
> purpose. You've noted that trusted.* xattrs aren't supported over nfs.
> That's unfortunate, but not an acceptable excuse for messing up user.*
> xattrs.

Another possibility would be to make selinux use a different
security.* attribute for this nested selinux case. That way, the
"host" selinux would retain some control over the labels the "guest"
uses.

Thanks,
Andreas


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03  6:56     ` Andreas Gruenbacher
@ 2021-09-03 14:42       ` Bruce Fields
  2021-09-03 15:43         ` Vivek Goyal
  0 siblings, 1 reply; 39+ messages in thread
From: Bruce Fields @ 2021-09-03 14:42 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Vivek Goyal, fstests, linux-fsdevel, LKML, virtio-fs,
	Daniel Walsh, David Gilbert, Christian Brauner, Casey Schaufler,
	LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Dave Chinner,
	Alexander Viro

Well, we could also look at supporting trusted.* xattrs over NFS.  I
don't know much about them, but it looks like it wouldn't be a lot of
work to specify, especially now that we've already got user xattrs?
We'd just write a new internet draft that refers to the existing
user.* xattr draft for most of the details.

--b.

On Fri, Sep 3, 2021 at 2:56 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> > On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > xfstests: generic/062: Do not run on newer kernels
> > >
> > > This test has been written with assumption that setting user.* xattrs will
> > > fail on symlink and special files. When newer kernels support setting
> > > user.* xattrs on symlink and special files, this test starts failing.
> >
> > It's actually a good thing that this test case triggers for the kernel
> > change you're proposing; that change should never be merged. The
> > user.* namespace is meant for data with the same access permissions as
> > the file data, and it has been for many years. We may have
> > applications that assume the existing behavior. In addition, this
> > change would create backwards compatibility problems for things like
> > backups.
> >
> > I'm not convinced that what you're actually proposing (mapping
> > security.selinux to a different attribute name) actually makes sense,
> > but that's a question for the selinux folks to decide. Mapping it to a
> > user.* attribute is definitely wrong though. The modified behavior
> > would affect anybody, not only users of selinux and/or virtiofs. If
> > mapping attribute names is actually the right approach, then you need
> > to look at trusted.* xattrs, which exist specifically for this kind of
> > purpose. You've noted that trusted.* xattrs aren't supported over nfs.
> > That's unfortunate, but not an acceptable excuse for messing up user.*
> > xattrs.
>
> Another possibility would be to make selinux use a different
> security.* attribute for this nested selinux case. That way, the
> "host" selinux would retain some control over the labels the "guest"
> uses.
>
> Thanks,
> Andreas
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 22:34           ` Casey Schaufler
@ 2021-09-03 15:26             ` Vivek Goyal
  2021-09-03 18:49               ` Casey Schaufler
  2021-09-06  7:45             ` [Virtio-fs] " Sergio Lopez
  2021-09-06 14:55             ` Dr. David Alan Gilbert
  2 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-03 15:26 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david

On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> >> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> >>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is V3 of the patch. Previous versions were posted here.
> >>>>>>
> >>>>>> v2:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>>>> v1:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>>>> +m/
> >>>>>>
> >>>>>> Changes since v2
> >>>>>> ----------------
> >>>>>> - Do not call inode_permission() for special files as file mode bits
> >>>>>>   on these files represent permissions to read/write from/to device
> >>>>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>>>   now user.* extended xattrs can be read/written on special files
> >>>>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>>>  
> >>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>>>   Bruce Fields)
> >>>>>>
> >>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>>>   same thread (Ted Ts'o).
> >>>>>>
> >>>>>> Testing
> >>>>>> -------
> >>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>>>   notice any new failures.
> >>>>>>
> >>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>>>   filesystems and it works.
> >>>>>>  
> >>>>>> Description
> >>>>>> ===========
> >>>>>>
> >>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>>>> files at all. Initially I thought that real reason behind this
> >>>>>> restriction is quota limitations but from last conversation it seemed
> >>>>>> that real reason is that permission bits on symlink and special files
> >>>>>> are special and different from regular files and directories, hence
> >>>>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>>>> quota restrictions kicked in on symlink).
> >>>>>>
> >>>>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>>>> This part of your project makes perfect sense. There's no good
> >>>>> security reason that you shouldn't set user.* xattrs on symlinks
> >>>>> and/or special files.
> >>>>>
> >>>>> However, your virtiofs use case is unreasonable.
> >>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >>>> should make use of this mechanism or not, right?
> >> I don't see a security objection. I did see that Andreas Gruenbacher
> >> <agruenba@redhat.com> has objections to the behavior.
> >>
> >>
> >>>>>> Who wants to set user.* xattr on symlink/special files
> >>>>>> -----------------------------------------------------
> >>>>>> I have primarily two users at this point of time.
> >>>>>>
> >>>>>> - virtiofs daemon.
> >>>>>>
> >>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>>>   should benefit from this change as well.
> >>>>>>
> >>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>>>> ----------------------------------------------------------------
> >>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>>>> There we have a mode where xattrs can be remapped to something else.
> >>>>>> For example security.selinux can be remapped to
> >>>>>> user.virtiofsd.securit.selinux on the host.
> >>>>> As I have stated before, this introduces a breach in security.
> >>>>> It allows an unprivileged process on the host to manipulate the
> >>>>> security state of the guest. This is horribly wrong. It is not
> >>>>> sufficient to claim that the breach requires misconfiguration
> >>>>> to exploit. Don't do this.
> >>>> So couple of things.
> >>>>
> >>>> - Right now whole virtiofs model is relying on the fact that host
> >>>>   unpriviliged users don't have access to shared directory. Otherwise
> >>>>   guest process can simply drop a setuid root binary in shared directory
> >>>>   and unpriviliged process can execute it and take over host system.
> >>>>
> >>>>   So if virtiofs makes use of this mechanism, we are well with-in
> >>>>   the existing constraints. If users don't follow the constraints,
> >>>>   bad things can happen.
> >>>>
> >>>> - I think Smalley provided a solution for your concern in other thread
> >>>>   we discussed this issue.
> >>>>
> >>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>>>
> >>>>
> >>>>   "So for example if the host policy says that only virtiofsd can set
> >>>> attributes on those files, then the guest MAC labels along with all
> >>>> the other attributes are protected against tampering by any other
> >>>> process on the host."
> >> You can't count on SELinux policy to address the issue on a
> >> system running Smack.
> >> Or any other user of system.* xattrs,
> >> be they in the kernel or user space. You can't even count on
> >> SELinux policy to be correct. virtiofs has to present a "safe"
> >> situation regardless of how security.* xattrs are used and
> >> regardless of which, if any, LSMs are configured. You can't
> >> do that with user.* attributes.
> > Lets take a step back. Your primary concern with using user.* xattrs
> > by virtiofsd is that it can be modified by unprivileged users on host.
> > And our solution to that problem is hide shared directory from
> > unprivileged users.
> 
> You really don't see how fragile that is, do you?

Yes, I am not sure why we are so opposed to the idea of using 
user.* xattrs for storing the guest security.selinux xattrs by virtiofsd.
And I am trying to understand that. And this discussion should help.

With virtiofsd, we want to keep all shared directory trees in a
parent directory which has read/write/search permissions only for root
so that no unpriviliged process can get to files in shared directory
and do any of the operations.

For example, /var/lib/containers/storage setup by podman has
following permissions.

ll -d /var/lib/containers/storage/
drwx------. 10 root root 4096 Jun 18  2020 /var/lib/containers/storage/

Now I should be able to create /var/lib/containers/storage/shared1
directory and ask virtiofsd to export "share1" to guest. Unprivileged
process on host can not open any of the files in shared1 dir, hence
should not be able to modify any data/metadata associated with the
file.

If this assumption is correct, then I should be able to use "user.*"
xattrs without having to worry about unprivileged processes modifying
security labels of guest/nested-guest.

> How a single
> errant call to rename(), chmod() or chown() on the host can expose
> the entire guest to exploitation. That's not even getting into
> the bag of mount() tricks.

I am relying on unpriviliged processes not having permissions to
read/search in shared directory. And I guess I have to. Even if
I use "trusted.*" xattrs, what about file data. Unpriviliged
processes can modify file data and that's going to be equally
problematic, isn't it? So why are we so focussed only on
security label part of it.

You have mentioned that file data is not necessarily
that big a problem because UID 1000 on host and UID 1000 in guest
should be treated same. But I am not sure we can do that. In kata
container model, guest images are untrusted. So they can simply cook
up UID 1000 or UID 0 or any other UID. Now there can be use
cases where we are ready to trust guest and treat UID 1000
on host and guest in same way. But primary model I am focussing
on is that guest shared directory remains isolated from other
users on host.

> 
> 
> > In addition to that, LSMs on host can block setting "user.*" xattrs by
> > virtiofsd domain only for additional protection.
> 
> Try thinking outside the SELinux box briefly, if you possibly can.
> An LSM that implements just Bell & LaPadula isn't going to have a
> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
> distinguish writing user xattrs from writing other file attributes
> in policy. Your argument requires a fine grained policy a'la SELinux.
> And an application specific SELinux policy at that.

Ok, so does we have to have capability for every LSM to block write
to user xattr. I mean in above example, virtiofsd is relying on DAC so that
unprivileged processes can't modify user xattr labels. If we were to
use "trusted.*" xattr then we are relying on CAP_SYS_ADMIN.

IOW, it will be nice if one or more LSMs can provide mechanism fine
grained enough to block write to user xattr by unwanted application
and that provides extra level of security. But should it be mandatory?

> 
> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

I agree that its easy to make mistakes. But making mistakes is already
disastrous. So lets say and admin makes mistake and unprivileged
processes can open/read/write files in shared directory on host.
Assume I am using "trusted" xattrs to store guest's security labels.

Now file's data can be modified on host in unwated way and be very
problematic.

Guest can drop a setuid root binary in shared directory and unprivileged
process on host can run it and take over the system.

Isn't that even bigger problem. So to me making sure shared directory
is not reachable by unprivileged processes is absolute must requirement
for virtiofsd (when running as root). If we break that assumption,
we already have much bigger problems.

> 
> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> It comes back to your design, which is fundamentally flawed. You
> can't store system security information in an attribute that can
> be manipulated by untrusted entities.

You are assuming untrusted entities can have access to the shared dir. But
assumption in this model is, shared directory is not reachable by
unprivileged entities. If we break that requirement, there are much
bigger issues to deal with then just security attributes. 

> That's why we have system.*
> xattrs. You want to have an attribute on the host that maps to a
> security attribute on the guest. The host has to protect the attribute
> on the guest with mechanisms of comparable strength as the guest's
> mechanisms. Otherwise you can't trust the guest with host data.

Ok, I understand your desire that security xattrs as seen by guest kernel
should be protected by something stronger than simply user xattr.

> 
> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> mechanism as implemented today won't scale to the hundreds of individual
> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> I'm not convinced that there isn't a way to accomplish what you're
> trying to do without privilege, but this isn't it, and I don't know
> what is. Sorry.
> 
> > Also if directory is not hidden, unprivileged users can change file
> > data and other metadata.
> 
> I assumed that you've taken that into account. Are you saying that
> isn't going to be done correctly either?

I am relying on shared directory not accessible to unprivileged processes.
If that assumption is broken, I guess,  all bets are off. Until and unless
one designs SELinux (or other LSM) policy in such a way so that
only virtiofsd can read/write to these shared directories.

I think Dan Walsh has got SELinux policy written for atleast kata
containers case.

So I guess we will rely on MAC to block unwanted access if users
make a mistake? Is that the idea? I guess that's why you want
a stronger mechansim to store guest security xattrs on host. If
users make a mistake then we have a fallback path?

> 
> >  Why that's not a concern and why there is
> > so much of focus only security xattr.
> 
> As with an NFS mount, the assumption is that UID 567 (or its magically
> mapped equivalent) has the same access rights on both the server/host
> and client/guest. I'm not worried about the mode bits because they are
> presented consistently on both machines. If, on the other hand, an
> attribute used to determine access is security.esprit on the guest and
> user.security.esprit on the host, the unprivileged user on the host
> can defeat the privilege requirements on the guest. That's why.

Hmm..., so if a user id 1000 inside guest can't modify security
xattrs then same user on host should be allowed to bypass that
by modifying user xattr. I agree with that.

Just that I am relying on shared directory not being accessible
to uid 1000 on host and you don't want to rely on that because
users can easily make mistake. And that's why want to stronger
form of mechanism to store security xattrs.

> 
> >  If you were to block modification
> > of file then you will have rely on LSMs.
> 
> No. We're talking about the semantics of the xattr namespaces.
> LSMs can further constrain access to xattrs, but the basic rules
> of access to the user.* and security.* attributes are different
> in any case. This is by design.
> 
> >  And if LSMs are not configured,
> > then we will rely on shared directory not being visible.
> 
> LSMs are not the problem. LSMs use security.* xattrs, which is why
> they come up in the discussion.
> 
> > Can you please help me understand why hiding shared directory from
> > unprivileged users is not a solution
> 
> Maybe you can describe the mechanism you use to "hide" a shared directory
> on the host. If the filesystem is mounted on the host it seems unlikely
> that you can provide a convincing argument for sufficient protection.

I am relying on changing direcotry permissions to allow read/write/search
permission to root only.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03 14:42       ` Bruce Fields
@ 2021-09-03 15:43         ` Vivek Goyal
  2021-09-03 15:50           ` Bruce Fields
  0 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-03 15:43 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Andreas Gruenbacher, fstests, linux-fsdevel, LKML, virtio-fs,
	Daniel Walsh, David Gilbert, Christian Brauner, Casey Schaufler,
	LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Dave Chinner,
	Alexander Viro

On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote:
> Well, we could also look at supporting trusted.* xattrs over NFS.  I
> don't know much about them, but it looks like it wouldn't be a lot of
> work to specify, especially now that we've already got user xattrs?
> We'd just write a new internet draft that refers to the existing
> user.* xattr draft for most of the details.

Will be nice if we can support trusted.* xattrs on NFS.

Vivek

> 
> --b.
> 
> On Fri, Sep 3, 2021 at 2:56 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> >
> > On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> > > On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > xfstests: generic/062: Do not run on newer kernels
> > > >
> > > > This test has been written with assumption that setting user.* xattrs will
> > > > fail on symlink and special files. When newer kernels support setting
> > > > user.* xattrs on symlink and special files, this test starts failing.
> > >
> > > It's actually a good thing that this test case triggers for the kernel
> > > change you're proposing; that change should never be merged. The
> > > user.* namespace is meant for data with the same access permissions as
> > > the file data, and it has been for many years. We may have
> > > applications that assume the existing behavior. In addition, this
> > > change would create backwards compatibility problems for things like
> > > backups.
> > >
> > > I'm not convinced that what you're actually proposing (mapping
> > > security.selinux to a different attribute name) actually makes sense,
> > > but that's a question for the selinux folks to decide. Mapping it to a
> > > user.* attribute is definitely wrong though. The modified behavior
> > > would affect anybody, not only users of selinux and/or virtiofs. If
> > > mapping attribute names is actually the right approach, then you need
> > > to look at trusted.* xattrs, which exist specifically for this kind of
> > > purpose. You've noted that trusted.* xattrs aren't supported over nfs.
> > > That's unfortunate, but not an acceptable excuse for messing up user.*
> > > xattrs.
> >
> > Another possibility would be to make selinux use a different
> > security.* attribute for this nested selinux case. That way, the
> > "host" selinux would retain some control over the labels the "guest"
> > uses.
> >
> > Thanks,
> > Andreas
> >
> 


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03 15:43         ` Vivek Goyal
@ 2021-09-03 15:50           ` Bruce Fields
  2021-09-03 16:01             ` Casey Schaufler
  2021-09-03 16:03             ` Vivek Goyal
  0 siblings, 2 replies; 39+ messages in thread
From: Bruce Fields @ 2021-09-03 15:50 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Andreas Gruenbacher, fstests, linux-fsdevel, LKML, virtio-fs,
	Daniel Walsh, David Gilbert, Christian Brauner, Casey Schaufler,
	LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Dave Chinner,
	Alexander Viro

On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote:
> > Well, we could also look at supporting trusted.* xattrs over NFS.  I
> > don't know much about them, but it looks like it wouldn't be a lot of
> > work to specify, especially now that we've already got user xattrs?
> > We'd just write a new internet draft that refers to the existing
> > user.* xattr draft for most of the details.
>
> Will be nice if we can support trusted.* xattrs on NFS.

Maybe I should start a separate thread for that.  Who would need to be
on it to be sure we get this right?

--b.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03 15:50           ` Bruce Fields
@ 2021-09-03 16:01             ` Casey Schaufler
  2021-09-03 16:03             ` Vivek Goyal
  1 sibling, 0 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-09-03 16:01 UTC (permalink / raw)
  To: Bruce Fields, Vivek Goyal
  Cc: Andreas Gruenbacher, fstests, linux-fsdevel, LKML, virtio-fs,
	Daniel Walsh, David Gilbert, Christian Brauner, Casey Schaufler,
	LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Dave Chinner,
	Alexander Viro, Casey Schaufler

On 9/3/2021 8:50 AM, Bruce Fields wrote:
> On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote:
>>> Well, we could also look at supporting trusted.* xattrs over NFS.  I
>>> don't know much about them, but it looks like it wouldn't be a lot of
>>> work to specify, especially now that we've already got user xattrs?
>>> We'd just write a new internet draft that refers to the existing
>>> user.* xattr draft for most of the details.
>> Will be nice if we can support trusted.* xattrs on NFS.
> Maybe I should start a separate thread for that.  Who would need to be
> on it to be sure we get this right?

I would like to be included. It would probably be a good idea to
include the LSM list, linux-security-module@vger.kernel.org. I'll leave
the networking and filesystem folks to speak for themselves.

>
> --b.
>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels
  2021-09-03 15:50           ` Bruce Fields
  2021-09-03 16:01             ` Casey Schaufler
@ 2021-09-03 16:03             ` Vivek Goyal
  1 sibling, 0 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-03 16:03 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Andreas Gruenbacher, fstests, linux-fsdevel, LKML, virtio-fs,
	Daniel Walsh, David Gilbert, Christian Brauner, Casey Schaufler,
	LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Dave Chinner,
	Alexander Viro

On Fri, Sep 03, 2021 at 11:50:43AM -0400, Bruce Fields wrote:
> On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote:
> > > Well, we could also look at supporting trusted.* xattrs over NFS.  I
> > > don't know much about them, but it looks like it wouldn't be a lot of
> > > work to specify, especially now that we've already got user xattrs?
> > > We'd just write a new internet draft that refers to the existing
> > > user.* xattr draft for most of the details.
> >
> > Will be nice if we can support trusted.* xattrs on NFS.
> 
> Maybe I should start a separate thread for that.  Who would need to be
> on it to be sure we get this right?

I will like to be on cc list.

Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-03 15:26             ` Vivek Goyal
@ 2021-09-03 18:49               ` Casey Schaufler
  0 siblings, 0 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-09-03 18:49 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, dwalsh, dgilbert,
	christian.brauner, casey.schaufler, linux-security-module,
	selinux, tytso, miklos, gscrivan, bfields, stephen.smalley.work,
	agruenba, david, Casey Schaufler

On 9/3/2021 8:26 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
>> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
>>> On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
>>>> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
>>>>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>>>>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>>>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This is V3 of the patch. Previous versions were posted here.
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>>>>>> v1:
>>>>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>>>>>> +m/
>>>>>>>>
>>>>>>>> Changes since v2
>>>>>>>> ----------------
>>>>>>>> - Do not call inode_permission() for special files as file mode bits
>>>>>>>>   on these files represent permissions to read/write from/to device
>>>>>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>>>>>   now user.* extended xattrs can be read/written on special files
>>>>>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>>>>>  
>>>>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>>>>>   Bruce Fields)
>>>>>>>>
>>>>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>>>>>   same thread (Ted Ts'o).
>>>>>>>>
>>>>>>>> Testing
>>>>>>>> -------
>>>>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>>>>>   notice any new failures.
>>>>>>>>
>>>>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>>>>>   filesystems and it works.
>>>>>>>>  
>>>>>>>> Description
>>>>>>>> ===========
>>>>>>>>
>>>>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>>>>>> files at all. Initially I thought that real reason behind this
>>>>>>>> restriction is quota limitations but from last conversation it seemed
>>>>>>>> that real reason is that permission bits on symlink and special files
>>>>>>>> are special and different from regular files and directories, hence
>>>>>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>>>>>> quota restrictions kicked in on symlink).
>>>>>>>>
>>>>>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>>>>>> This part of your project makes perfect sense. There's no good
>>>>>>> security reason that you shouldn't set user.* xattrs on symlinks
>>>>>>> and/or special files.
>>>>>>>
>>>>>>> However, your virtiofs use case is unreasonable.
>>>>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>>>>>> should make use of this mechanism or not, right?
>>>> I don't see a security objection. I did see that Andreas Gruenbacher
>>>> <agruenba@redhat.com> has objections to the behavior.
>>>>
>>>>
>>>>>>>> Who wants to set user.* xattr on symlink/special files
>>>>>>>> -----------------------------------------------------
>>>>>>>> I have primarily two users at this point of time.
>>>>>>>>
>>>>>>>> - virtiofs daemon.
>>>>>>>>
>>>>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>>>>>   should benefit from this change as well.
>>>>>>>>
>>>>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>>>>>> ----------------------------------------------------------------
>>>>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>>>>>> There we have a mode where xattrs can be remapped to something else.
>>>>>>>> For example security.selinux can be remapped to
>>>>>>>> user.virtiofsd.securit.selinux on the host.
>>>>>>> As I have stated before, this introduces a breach in security.
>>>>>>> It allows an unprivileged process on the host to manipulate the
>>>>>>> security state of the guest. This is horribly wrong. It is not
>>>>>>> sufficient to claim that the breach requires misconfiguration
>>>>>>> to exploit. Don't do this.
>>>>>> So couple of things.
>>>>>>
>>>>>> - Right now whole virtiofs model is relying on the fact that host
>>>>>>   unpriviliged users don't have access to shared directory. Otherwise
>>>>>>   guest process can simply drop a setuid root binary in shared directory
>>>>>>   and unpriviliged process can execute it and take over host system.
>>>>>>
>>>>>>   So if virtiofs makes use of this mechanism, we are well with-in
>>>>>>   the existing constraints. If users don't follow the constraints,
>>>>>>   bad things can happen.
>>>>>>
>>>>>> - I think Smalley provided a solution for your concern in other thread
>>>>>>   we discussed this issue.
>>>>>>
>>>>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>>>>>
>>>>>>
>>>>>>   "So for example if the host policy says that only virtiofsd can set
>>>>>> attributes on those files, then the guest MAC labels along with all
>>>>>> the other attributes are protected against tampering by any other
>>>>>> process on the host."
>>>> You can't count on SELinux policy to address the issue on a
>>>> system running Smack.
>>>> Or any other user of system.* xattrs,
>>>> be they in the kernel or user space. You can't even count on
>>>> SELinux policy to be correct. virtiofs has to present a "safe"
>>>> situation regardless of how security.* xattrs are used and
>>>> regardless of which, if any, LSMs are configured. You can't
>>>> do that with user.* attributes.
>>> Lets take a step back. Your primary concern with using user.* xattrs
>>> by virtiofsd is that it can be modified by unprivileged users on host.
>>> And our solution to that problem is hide shared directory from
>>> unprivileged users.
>> You really don't see how fragile that is, do you?
> Yes, I am not sure why we are so opposed to the idea of using 
> user.* xattrs for storing the guest security.selinux xattrs by virtiofsd.
> And I am trying to understand that. And this discussion should help.
>
> With virtiofsd, we want to keep all shared directory trees in a
> parent directory which has read/write/search permissions only for root
> so that no unpriviliged process can get to files in shared directory
> and do any of the operations.
>
> For example, /var/lib/containers/storage setup by podman has
> following permissions.
>
> ll -d /var/lib/containers/storage/
> drwx------. 10 root root 4096 Jun 18  2020 /var/lib/containers/storage/
>
> Now I should be able to create /var/lib/containers/storage/shared1
> directory and ask virtiofsd to export "share1" to guest. Unprivileged
> process on host can not open any of the files in shared1 dir, hence
> should not be able to modify any data/metadata associated with the
> file.
>
> If this assumption is correct, then I should be able to use "user.*"
> xattrs without having to worry about unprivileged processes modifying
> security labels of guest/nested-guest.

The only attributes you can really count on to protect an object
are the attributes on the object itself. Path based protections are
not reliable.


>> How a single
>> errant call to rename(), chmod() or chown() on the host can expose
>> the entire guest to exploitation. That's not even getting into
>> the bag of mount() tricks.
> I am relying on unpriviliged processes not having permissions to
> read/search in shared directory. And I guess I have to. Even if
> I use "trusted.*" xattrs, what about file data. Unpriviliged
> processes can modify file data and that's going to be equally
> problematic, isn't it? So why are we so focussed only on
> security label part of it.

We are concentrating on your proposed clever mapping trick.
We are doing so because it won't work, and when it blows up
into a full security bruhaha someone is going to try putting
the blame on the xattr mechanism.

>
> You have mentioned that file data is not necessarily
> that big a problem because UID 1000 on host and UID 1000 in guest
> should be treated same. But I am not sure we can do that. In kata
> container model, guest images are untrusted. So they can simply cook
> up UID 1000 or UID 0 or any other UID. Now there can be use
> cases where we are ready to trust guest and treat UID 1000
> on host and guest in same way. But primary model I am focussing
> on is that guest shared directory remains isolated from other
> users on host.

I don't have the time just now to examine the rest of virtiofs.
I am quite afraid that you are making dangerous assumptions on
a number of things. It sure doesn't sound like you've thought
through all of the implications of sharing between the host and
guest.

>>> In addition to that, LSMs on host can block setting "user.*" xattrs by
>>> virtiofsd domain only for additional protection.
>> Try thinking outside the SELinux box briefly, if you possibly can.
>> An LSM that implements just Bell & LaPadula isn't going to have a
>> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
>> distinguish writing user xattrs from writing other file attributes
>> in policy. Your argument requires a fine grained policy a'la SELinux.
>> And an application specific SELinux policy at that.
> Ok, so does we have to have capability for every LSM to block write
> to user xattr. I mean in above example, virtiofsd is relying on DAC so that
> unprivileged processes can't modify user xattr labels. If we were to
> use "trusted.*" xattr then we are relying on CAP_SYS_ADMIN.
>
> IOW, it will be nice if one or more LSMs can provide mechanism fine
> grained enough to block write to user xattr by unwanted application
> and that provides extra level of security. But should it be mandatory?

No. The behavior of user xattrs is just fine the way it is.
It works as designed. 

>
>>>  If LSMs are not configured,
>>> then hiding the directory is the solution.
>> It's not a solution at all. It's wishful thinking that
>> some admin is going to do absolutely everything right, will
>> never make a mistake and will never, ever, read the mount(2)
>> man page.
> I agree that its easy to make mistakes. But making mistakes is already
> disastrous. So lets say and admin makes mistake and unprivileged
> processes can open/read/write files in shared directory on host.
> Assume I am using "trusted" xattrs to store guest's security labels.
>
> Now file's data can be modified on host in unwated way and be very
> problematic.
>
> Guest can drop a setuid root binary in shared directory and unprivileged
> process on host can run it and take over the system.
>
> Isn't that even bigger problem. So to me making sure shared directory
> is not reachable by unprivileged processes is absolute must requirement
> for virtiofsd (when running as root). If we break that assumption,
> we already have much bigger problems.

I can't help but think you probably do already have much bigger problems.

>>> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
>>> solution. I don't understand that part.
>> It comes back to your design, which is fundamentally flawed. You
>> can't store system security information in an attribute that can
>> be manipulated by untrusted entities.
> You are assuming untrusted entities can have access to the shared dir. But
> assumption in this model is, shared directory is not reachable by
> unprivileged entities. If we break that requirement, there are much
> bigger issues to deal with then just security attributes.

That's a bad assumption. You have all sorts of issues.

>
>> That's why we have system.*
>> xattrs. You want to have an attribute on the host that maps to a
>> security attribute on the guest. The host has to protect the attribute
>> on the guest with mechanisms of comparable strength as the guest's
>> mechanisms. Otherwise you can't trust the guest with host data.
> Ok, I understand your desire that security xattrs as seen by guest kernel
> should be protected by something stronger than simply user xattr.
>
>> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
>> mechanism as implemented today won't scale to the hundreds of individual
>> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
>> I'm not convinced that there isn't a way to accomplish what you're
>> trying to do without privilege, but this isn't it, and I don't know
>> what is. Sorry.
>>
>>> Also if directory is not hidden, unprivileged users can change file
>>> data and other metadata.
>> I assumed that you've taken that into account. Are you saying that
>> isn't going to be done correctly either?
> I am relying on shared directory not accessible to unprivileged processes.
> If that assumption is broken, I guess,  all bets are off. Until and unless
> one designs SELinux (or other LSM) policy in such a way so that
> only virtiofsd can read/write to these shared directories.
>
> I think Dan Walsh has got SELinux policy written for atleast kata
> containers case.
>
> So I guess we will rely on MAC to block unwanted access if users
> make a mistake? Is that the idea? I guess that's why you want
> a stronger mechansim to store guest security xattrs on host. If
> users make a mistake then we have a fallback path?
>
>>>  Why that's not a concern and why there is
>>> so much of focus only security xattr.
>> As with an NFS mount, the assumption is that UID 567 (or its magically
>> mapped equivalent) has the same access rights on both the server/host
>> and client/guest. I'm not worried about the mode bits because they are
>> presented consistently on both machines. If, on the other hand, an
>> attribute used to determine access is security.esprit on the guest and
>> user.security.esprit on the host, the unprivileged user on the host
>> can defeat the privilege requirements on the guest. That's why.
> Hmm..., so if a user id 1000 inside guest can't modify security
> xattrs then same user on host should be allowed to bypass that
> by modifying user xattr. I agree with that.
>
> Just that I am relying on shared directory not being accessible
> to uid 1000 on host and you don't want to rely on that because
> users can easily make mistake. And that's why want to stronger
> form of mechanism to store security xattrs.
>
>>>  If you were to block modification
>>> of file then you will have rely on LSMs.
>> No. We're talking about the semantics of the xattr namespaces.
>> LSMs can further constrain access to xattrs, but the basic rules
>> of access to the user.* and security.* attributes are different
>> in any case. This is by design.
>>
>>>  And if LSMs are not configured,
>>> then we will rely on shared directory not being visible.
>> LSMs are not the problem. LSMs use security.* xattrs, which is why
>> they come up in the discussion.
>>
>>> Can you please help me understand why hiding shared directory from
>>> unprivileged users is not a solution
>> Maybe you can describe the mechanism you use to "hide" a shared directory
>> on the host. If the filesystem is mounted on the host it seems unlikely
>> that you can provide a convincing argument for sufficient protection.
> I am relying on changing direcotry permissions to allow read/write/search
> permission to root only.

Then you have a real mess of problems coming your way. Sorry.

>
> Thanks
> Vivek
>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [Virtio-fs] [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 22:34           ` Casey Schaufler
  2021-09-03 15:26             ` Vivek Goyal
@ 2021-09-06  7:45             ` Sergio Lopez
  2021-09-06 14:55             ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 39+ messages in thread
From: Sergio Lopez @ 2021-09-06  7:45 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Vivek Goyal, gscrivan, tytso, agruenba, miklos, selinux,
	stephen.smalley.work, david, linux-kernel, virtio-fs,
	casey.schaufler, linux-security-module, viro, linux-fsdevel,
	bfields, christian.brauner

[-- Attachment #1: Type: text/plain, Size: 8266 bytes --]

On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> >> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> >>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is V3 of the patch. Previous versions were posted here.
> >>>>>>
> >>>>>> v2:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>>>> v1:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>>>> +m/
> >>>>>>
> >>>>>> Changes since v2
> >>>>>> ----------------
> >>>>>> - Do not call inode_permission() for special files as file mode bits
> >>>>>>   on these files represent permissions to read/write from/to device
> >>>>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>>>   now user.* extended xattrs can be read/written on special files
> >>>>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>>>  
> >>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>>>   Bruce Fields)
> >>>>>>
> >>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>>>   same thread (Ted Ts'o).
> >>>>>>
> >>>>>> Testing
> >>>>>> -------
> >>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>>>   notice any new failures.
> >>>>>>
> >>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>>>   filesystems and it works.
> >>>>>>  
> >>>>>> Description
> >>>>>> ===========
> >>>>>>
> >>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>>>> files at all. Initially I thought that real reason behind this
> >>>>>> restriction is quota limitations but from last conversation it seemed
> >>>>>> that real reason is that permission bits on symlink and special files
> >>>>>> are special and different from regular files and directories, hence
> >>>>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>>>> quota restrictions kicked in on symlink).
> >>>>>>
> >>>>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>>>> This part of your project makes perfect sense. There's no good
> >>>>> security reason that you shouldn't set user.* xattrs on symlinks
> >>>>> and/or special files.
> >>>>>
> >>>>> However, your virtiofs use case is unreasonable.
> >>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >>>> should make use of this mechanism or not, right?
> >> I don't see a security objection. I did see that Andreas Gruenbacher
> >> <agruenba@redhat.com> has objections to the behavior.
> >>
> >>
> >>>>>> Who wants to set user.* xattr on symlink/special files
> >>>>>> -----------------------------------------------------
> >>>>>> I have primarily two users at this point of time.
> >>>>>>
> >>>>>> - virtiofs daemon.
> >>>>>>
> >>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>>>   should benefit from this change as well.
> >>>>>>
> >>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>>>> ----------------------------------------------------------------
> >>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>>>> There we have a mode where xattrs can be remapped to something else.
> >>>>>> For example security.selinux can be remapped to
> >>>>>> user.virtiofsd.securit.selinux on the host.
> >>>>> As I have stated before, this introduces a breach in security.
> >>>>> It allows an unprivileged process on the host to manipulate the
> >>>>> security state of the guest. This is horribly wrong. It is not
> >>>>> sufficient to claim that the breach requires misconfiguration
> >>>>> to exploit. Don't do this.
> >>>> So couple of things.
> >>>>
> >>>> - Right now whole virtiofs model is relying on the fact that host
> >>>>   unpriviliged users don't have access to shared directory. Otherwise
> >>>>   guest process can simply drop a setuid root binary in shared directory
> >>>>   and unpriviliged process can execute it and take over host system.
> >>>>
> >>>>   So if virtiofs makes use of this mechanism, we are well with-in
> >>>>   the existing constraints. If users don't follow the constraints,
> >>>>   bad things can happen.
> >>>>
> >>>> - I think Smalley provided a solution for your concern in other thread
> >>>>   we discussed this issue.
> >>>>
> >>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>>>
> >>>>
> >>>>   "So for example if the host policy says that only virtiofsd can set
> >>>> attributes on those files, then the guest MAC labels along with all
> >>>> the other attributes are protected against tampering by any other
> >>>> process on the host."
> >> You can't count on SELinux policy to address the issue on a
> >> system running Smack.
> >> Or any other user of system.* xattrs,
> >> be they in the kernel or user space. You can't even count on
> >> SELinux policy to be correct. virtiofs has to present a "safe"
> >> situation regardless of how security.* xattrs are used and
> >> regardless of which, if any, LSMs are configured. You can't
> >> do that with user.* attributes.
> > Lets take a step back. Your primary concern with using user.* xattrs
> > by virtiofsd is that it can be modified by unprivileged users on host.
> > And our solution to that problem is hide shared directory from
> > unprivileged users.
> 
> You really don't see how fragile that is, do you? How a single
> errant call to rename(), chmod() or chown() on the host can expose
> the entire guest to exploitation. That's not even getting into
> the bag of mount() tricks.
> 
> 
> > In addition to that, LSMs on host can block setting "user.*" xattrs by
> > virtiofsd domain only for additional protection.
> 
> Try thinking outside the SELinux box briefly, if you possibly can.
> An LSM that implements just Bell & LaPadula isn't going to have a
> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
> distinguish writing user xattrs from writing other file attributes
> in policy. Your argument requires a fine grained policy a'la SELinux.
> And an application specific SELinux policy at that.
> 
> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

It's not worse than hoping that every admin is going to set up the
right permissions on a file backing the disk image of a Virtual
Machine.

And I'm no simply justifying one bad thing with another thing that's
even worse, I'm just trying to realign our expectations with the
current threat model of Virtualization in Linux. The host is protected
against the guest, but the guest is *not* implicitly protected against
the host. Everything depends on the host's admin good faith and even
better security practices.

This is the very reason why Virtualization-based Confidential
Computing technologies such as AMD SEV and Intel TDX are attracting so
much attention lately, as they aim to close the gap and also protect
guest against the host (as a necessary step to build trust on its
contents and behavior). But this is outside virtio-fs's scope.

Sergio.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 17:52 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Andreas Gruenbacher
  2021-09-02 18:48   ` Vivek Goyal
@ 2021-09-06 14:39   ` Dr. David Alan Gilbert
  2021-09-06 14:56     ` Miklos Szeredi
  1 sibling, 1 reply; 39+ messages in thread
From: Dr. David Alan Gilbert @ 2021-09-06 14:39 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Vivek Goyal, Alexander Viro, linux-fsdevel, LKML, virtio-fs,
	dwalsh, christian.brauner, casey.schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, gscrivan, Fields, Bruce,
	stephen.smalley.work, Dave Chinner

* Andreas Gruenbacher (agruenba@redhat.com) wrote:
> Hi,
> 
> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> the idea behind user.* xattrs is that they behave similar to file
> contents as far as permissions go. It follows from that that symlinks
> and special files cannot have user.* xattrs. This has been the model
> for many years now and applications may be expecting these semantics,
> so we cannot simply change the behavior. So NACK from me.
> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> >
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> The fact that user.* xattrs don't work in this remapping scenario
> should have told you that you're doing things wrong; the user.*
> namespace seriously was never meant to be abused in this way.
> 
> You may be able to get away with using trusted.* xattrs which support
> roughly the kind of daemon use I think you're talking about here, but
> I'm not sure selinux will be happy with labels that aren't fully under
> its own control. I really wonder why this wasn't obvious enough.

It was; however in our use case it wasn't an issue in general, because
the selinux instance that was setting the labels was inside an untrusted
guest, as such it's labels on the host are themselves untrusted, and
hence user. made some sense to the host - until we found out the
restrictons on user. the hard way.

The mapping code we have doesn't explicitly set user. - it's an
arbitrary remapper that can map to anything you like, trusted. whatever,
but user. feels (to us) like it's right for an untrusted guest.

IMHO the real problem here is that the user/trusted/system/security
'namespaces' are arbitrary hacks rather than a proper namespacing
mechanism that allows you to create new (nested) namespaces and associate
permissions with each one.

Each one carries with it some arbitrary baggage (trusted not working on
NFS, user. having the special rules on symlinks etc).

Then every fs or application that trips over these arbitrary limits adds
some hack to work around them in a different way to every other fs or
app that's doing the same thing; (see 9p, overlayfs, fuse-overlayfs,
crosvm etc etc all that do some level of renaming)

What we really need is a namespace where you can do anything you like,
but it's then limited by the security modules, so that I could allow
user.virtiofsd.guest1 to be able to set labels on symlinks for example.

Dave

> Thanks,
> Andreas
> 
> > Thanks
> > Vivek
> >
> > Vivek Goyal (1):
> >   xattr: Allow user.* xattr on symlink and special files
> >
> >  fs/xattr.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > --
> > 2.31.1
> >
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-02 22:34           ` Casey Schaufler
  2021-09-03 15:26             ` Vivek Goyal
  2021-09-06  7:45             ` [Virtio-fs] " Sergio Lopez
@ 2021-09-06 14:55             ` Dr. David Alan Gilbert
  2021-09-13 19:05               ` Casey Schaufler
  2 siblings, 1 reply; 39+ messages in thread
From: Dr. David Alan Gilbert @ 2021-09-06 14:55 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Vivek Goyal, viro, linux-fsdevel, linux-kernel, virtio-fs,
	dwalsh, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, david

* Casey Schaufler (casey@schaufler-ca.com) wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:

> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

That is why we run our virtiofsd with a sandbox setup and seccomp; and
frankly anything we can or could turn on we would.

> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> It comes back to your design, which is fundamentally flawed. You
> can't store system security information in an attribute that can
> be manipulated by untrusted entities. That's why we have system.*
> xattrs. You want to have an attribute on the host that maps to a
> security attribute on the guest. The host has to protect the attribute
> on the guest with mechanisms of comparable strength as the guest's
> mechanisms.

Can you just explain this line to me a bit more: 
> Otherwise you can't trust the guest with host data.

Note we're not trying to trust the guest with the host data here;
we're trying to allow the guest to store the data on the host, while
trusting the host.

> 
> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> mechanism as implemented today won't scale to the hundreds of individual
> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> I'm not convinced that there isn't a way to accomplish what you're
> trying to do without privilege, but this isn't it, and I don't know
> what is. Sorry.
> 
> > Also if directory is not hidden, unprivileged users can change file
> > data and other metadata.
> 
> I assumed that you've taken that into account. Are you saying that
> isn't going to be done correctly either?
> 
> >  Why that's not a concern and why there is
> > so much of focus only security xattr.
> 
> As with an NFS mount, the assumption is that UID 567 (or its magically
> mapped equivalent) has the same access rights on both the server/host
> and client/guest. I'm not worried about the mode bits because they are
> presented consistently on both machines. If, on the other hand, an
> attribute used to determine access is security.esprit on the guest and
> user.security.esprit on the host, the unprivileged user on the host
> can defeat the privilege requirements on the guest. That's why.

We're OK with that; remember that the host can do wth it likes to the
guest anyway - it can just go in and poke at the guests RAM if it wants
to do something evil to the guest.
We wouldn't suggest using a scheme like this once you have
encrypted/protected guest RAM for example (SEV/TDX etc)

> >  If you were to block modification
> > of file then you will have rely on LSMs.
> 
> No. We're talking about the semantics of the xattr namespaces.
> LSMs can further constrain access to xattrs, but the basic rules
> of access to the user.* and security.* attributes are different
> in any case. This is by design.

I'm happy if you can suggest somewhere else to store the guests xattr
data other than in one of the hosts xattr's - the challenge is doing
that in a non-racy way, and making sure that the xattr's never get
associated with the wrong file as seen by a guest.

> >  And if LSMs are not configured,
> > then we will rely on shared directory not being visible.
> 
> LSMs are not the problem. LSMs use security.* xattrs, which is why
> they come up in the discussion.
> 
> > Can you please help me understand why hiding shared directory from
> > unprivileged users is not a solution
> 
> Maybe you can describe the mechanism you use to "hide" a shared directory
> on the host. If the filesystem is mounted on the host it seems unlikely
> that you can provide a convincing argument for sufficient protection.

Why? What can a guests fs mounted on the host, under one of the
directories that's already typically used for container fs's do - it's
already what fileservers, and existing container systems do.

Dave



> >  (With both LSMs configured or
> > not configured on host). That's a requirement for virtiofs anyway. 
> > And if we agree on that, then I don't see why using "user.*" xattrs
> > for storing guest sercurity attributes is a problem.
> >
> > Thanks
> > Vivek
> >
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-06 14:39   ` Dr. David Alan Gilbert
@ 2021-09-06 14:56     ` Miklos Szeredi
  2021-09-07 21:40       ` Vivek Goyal
  0 siblings, 1 reply; 39+ messages in thread
From: Miklos Szeredi @ 2021-09-06 14:56 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Andreas Gruenbacher, Vivek Goyal, Alexander Viro, linux-fsdevel,
	LKML, virtio-fs-list, Daniel J Walsh, Christian Brauner,
	casey.schaufler, LSM, SElinux list, Theodore Ts'o,
	Giuseppe Scrivano, Fields, Bruce, Stephen Smalley, Dave Chinner,
	Eric W. Biederman

On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:

> IMHO the real problem here is that the user/trusted/system/security
> 'namespaces' are arbitrary hacks rather than a proper namespacing
> mechanism that allows you to create new (nested) namespaces and associate
> permissions with each one.

Indeed.

This is what Eric Biederman suggested at some point for supporting
trusted xattrs within a user namespace:

| For trusted xattrs I think it makes sense in principle.   The namespace
| would probably become something like "trusted<ns-root-uid>.".

Theory sounds simple enough.  Anyone interested in looking at the details?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-06 14:56     ` Miklos Szeredi
@ 2021-09-07 21:40       ` Vivek Goyal
  2021-09-08  7:37         ` Miklos Szeredi
  0 siblings, 1 reply; 39+ messages in thread
From: Vivek Goyal @ 2021-09-07 21:40 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Dr. David Alan Gilbert, Andreas Gruenbacher, Alexander Viro,
	linux-fsdevel, LKML, virtio-fs-list, Daniel J Walsh,
	Christian Brauner, casey.schaufler, LSM, SElinux list,
	Theodore Ts'o, Giuseppe Scrivano, Fields, Bruce,
	Stephen Smalley, Dave Chinner, Eric W. Biederman

On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
> On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> 
> > IMHO the real problem here is that the user/trusted/system/security
> > 'namespaces' are arbitrary hacks rather than a proper namespacing
> > mechanism that allows you to create new (nested) namespaces and associate
> > permissions with each one.
> 
> Indeed.
> 
> This is what Eric Biederman suggested at some point for supporting
> trusted xattrs within a user namespace:
> 
> | For trusted xattrs I think it makes sense in principle.   The namespace
> | would probably become something like "trusted<ns-root-uid>.".
> 
> Theory sounds simple enough.  Anyone interested in looking at the details?

So this namespaced trusted.* xattr domain will basically avoid the need
to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
than giving CAP_SYS_ADMIN in init_user_ns.

Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-07 21:40       ` Vivek Goyal
@ 2021-09-08  7:37         ` Miklos Szeredi
  2021-09-08 14:20           ` Eric W. Biederman
  0 siblings, 1 reply; 39+ messages in thread
From: Miklos Szeredi @ 2021-09-08  7:37 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Dr. David Alan Gilbert, Andreas Gruenbacher, Alexander Viro,
	linux-fsdevel, LKML, virtio-fs-list, Daniel J Walsh,
	Christian Brauner, Casey Schaufler, LSM, SElinux list,
	Theodore Ts'o, Giuseppe Scrivano, Fields, Bruce,
	Stephen Smalley, Dave Chinner, Eric W. Biederman

On Tue, 7 Sept 2021 at 23:40, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
> > On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> >
> > > IMHO the real problem here is that the user/trusted/system/security
> > > 'namespaces' are arbitrary hacks rather than a proper namespacing
> > > mechanism that allows you to create new (nested) namespaces and associate
> > > permissions with each one.
> >
> > Indeed.
> >
> > This is what Eric Biederman suggested at some point for supporting
> > trusted xattrs within a user namespace:
> >
> > | For trusted xattrs I think it makes sense in principle.   The namespace
> > | would probably become something like "trusted<ns-root-uid>.".
> >
> > Theory sounds simple enough.  Anyone interested in looking at the details?
>
> So this namespaced trusted.* xattr domain will basically avoid the need
> to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
> than giving CAP_SYS_ADMIN in init_user_ns.

That's the objective, yes.  I think the trick is getting filesystems
to store yet another xattr type.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-08  7:37         ` Miklos Szeredi
@ 2021-09-08 14:20           ` Eric W. Biederman
  0 siblings, 0 replies; 39+ messages in thread
From: Eric W. Biederman @ 2021-09-08 14:20 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Vivek Goyal, Dr. David Alan Gilbert, Andreas Gruenbacher,
	Alexander Viro, linux-fsdevel, LKML, virtio-fs-list,
	Daniel J Walsh, Christian Brauner, Casey Schaufler, LSM,
	SElinux list, Theodore Ts'o, Giuseppe Scrivano, Fields,
	Bruce, Stephen Smalley, Dave Chinner, Serge E. Hallyn

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Tue, 7 Sept 2021 at 23:40, Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
>> > On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
>> > <dgilbert@redhat.com> wrote:
>> >
>> > > IMHO the real problem here is that the user/trusted/system/security
>> > > 'namespaces' are arbitrary hacks rather than a proper namespacing
>> > > mechanism that allows you to create new (nested) namespaces and associate
>> > > permissions with each one.
>> >
>> > Indeed.
>> >
>> > This is what Eric Biederman suggested at some point for supporting
>> > trusted xattrs within a user namespace:
>> >
>> > | For trusted xattrs I think it makes sense in principle.   The namespace
>> > | would probably become something like "trusted<ns-root-uid>.".
>> >
>> > Theory sounds simple enough.  Anyone interested in looking at the details?
>>
>> So this namespaced trusted.* xattr domain will basically avoid the need
>> to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
>> than giving CAP_SYS_ADMIN in init_user_ns.
>
> That's the objective, yes.  I think the trick is getting filesystems
> to store yet another xattr type.

Using the uid of the root user of a user namespace is probably the best
idea we have so far for identifying a user namespace in persistent
on-disk meta-data.  We ran into a little trouble using that idea
for file capabilities.

The key problem was there are corner cases where some nested user
namespaces have the same root user id as their parent namespaces.  This
has the potential to allow privilege escalation if the creator of the
user namespace does not have sufficient capabilities.

The solution we adopted can be seen in db2e718a4798 ("capabilities:
require CAP_SETFCAP to map uid 0").

That solution is basically not allowing the creation of user namespaces
that could have problems.  I think use trusted xattrs this way the code
would need to treat CAP_SYS_ADMIN the same way it currently treats
CAP_SETFCAP.

Eric

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-06 14:55             ` Dr. David Alan Gilbert
@ 2021-09-13 19:05               ` Casey Schaufler
  2021-09-14 12:51                 ` Vivek Goyal
  0 siblings, 1 reply; 39+ messages in thread
From: Casey Schaufler @ 2021-09-13 19:05 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Vivek Goyal, viro, linux-fsdevel, linux-kernel, virtio-fs,
	dwalsh, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, david, Casey Schaufler

On 9/6/2021 7:55 AM, Dr. David Alan Gilbert wrote:
> * Casey Schaufler (casey@schaufler-ca.com) wrote:
>> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
>>>  If LSMs are not configured,
>>> then hiding the directory is the solution.
>> It's not a solution at all. It's wishful thinking that
>> some admin is going to do absolutely everything right, will
>> never make a mistake and will never, ever, read the mount(2)
>> man page.
> That is why we run our virtiofsd with a sandbox setup and seccomp; and
> frankly anything we can or could turn on we would.

That doesn't address my concern at all. Being able to create an
environment in which a feature can be used safely does not make
the feature safe.


> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> solution. I don't understand that part.

Sure you do. If you didn't, you wouldn't be so concerned about
requiring CAP_SYS_ADMIN. You're trying hard to avoid taking the
level of responsibility that running with privilege requires.
To do that, you're introducing a massive security hole, a backdoor
into the file system security attributes.

>> It comes back to your design, which is fundamentally flawed. You
>> can't store system security information in an attribute that can
>> be manipulated by untrusted entities. That's why we have system.*
>> xattrs. You want to have an attribute on the host that maps to a
>> security attribute on the guest. The host has to protect the attribute
>> on the guest with mechanisms of comparable strength as the guest's
>> mechanisms.
> Can you just explain this line to me a bit more: 
>> Otherwise you can't trust the guest with host data.
> Note we're not trying to trust the guest with the host data here;
> we're trying to allow the guest to store the data on the host, while
> trusting the host.

But you can't trust the host! You're allowing unprivileged processes
on the host to modify security state of the guest.

>> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
>> mechanism as implemented today won't scale to the hundreds of individual
>> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
>> I'm not convinced that there isn't a way to accomplish what you're
>> trying to do without privilege, but this isn't it, and I don't know
>> what is. Sorry.
>>
>>> Also if directory is not hidden, unprivileged users can change file
>>> data and other metadata.
>> I assumed that you've taken that into account. Are you saying that
>> isn't going to be done correctly either?
>>
>>>  Why that's not a concern and why there is
>>> so much of focus only security xattr.
>> As with an NFS mount, the assumption is that UID 567 (or its magically
>> mapped equivalent) has the same access rights on both the server/host
>> and client/guest. I'm not worried about the mode bits because they are
>> presented consistently on both machines. If, on the other hand, an
>> attribute used to determine access is security.esprit on the guest and
>> user.security.esprit on the host, the unprivileged user on the host
>> can defeat the privilege requirements on the guest. That's why.
> We're OK with that;

I understand that. I  am  not  OK  with  that.

>  remember that the host can do wth it likes to the
> guest anyway

We're not talking about "the host", we're talking about an
unprivileged user on the host.

>  - it can just go in and poke at the guests RAM if it wants
> to do something evil to the guest.
> We wouldn't suggest using a scheme like this once you have
> encrypted/protected guest RAM for example (SEV/TDX etc)
>
>>>  If you were to block modification
>>> of file then you will have rely on LSMs.
>> No. We're talking about the semantics of the xattr namespaces.
>> LSMs can further constrain access to xattrs, but the basic rules
>> of access to the user.* and security.* attributes are different
>> in any case. This is by design.
> I'm happy if you can suggest somewhere else to store the guests xattr
> data other than in one of the hosts xattr's - the challenge is doing
> that in a non-racy way, and making sure that the xattr's never get
> associated with the wrong file as seen by a guest.

I'm sorry, but I've got a bunch of other stuff on my plate.
I've already suggested implementing xattr namespaces a'la user
namespaces, but I understand that is beyond the scope of your
current needs, and has its own set of dragons.

>>>  And if LSMs are not configured,
>>> then we will rely on shared directory not being visible.
>> LSMs are not the problem. LSMs use security.* xattrs, which is why
>> they come up in the discussion.
>>
>>> Can you please help me understand why hiding shared directory from
>>> unprivileged users is not a solution
>> Maybe you can describe the mechanism you use to "hide" a shared directory
>> on the host. If the filesystem is mounted on the host it seems unlikely
>> that you can provide a convincing argument for sufficient protection.
> Why?

Because 99-44/100% of admins out there aren't as skilled at "hiding"
data as you are. Many (I almost said "most". I'm still not sure which.)
of them don't even know how to use mode bits correctly.

>  What can a guests fs mounted on the host, under one of the
> directories that's already typically used for container fs's do - it's
> already what fileservers, and existing container systems do.
>
> Dave
>
>
>
>>>  (With both LSMs configured or
>>> not configured on host). That's a requirement for virtiofs anyway. 
>>> And if we agree on that, then I don't see why using "user.*" xattrs
>>> for storing guest sercurity attributes is a problem.
>>>
>>> Thanks
>>> Vivek
>>>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-13 19:05               ` Casey Schaufler
@ 2021-09-14 12:51                 ` Vivek Goyal
  2021-09-14 13:56                   ` Casey Schaufler
  2021-09-14 13:59                   ` Bruce Fields
  0 siblings, 2 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-14 12:51 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Dr. David Alan Gilbert, viro, linux-fsdevel, linux-kernel,
	virtio-fs, dwalsh, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, david

On Mon, Sep 13, 2021 at 12:05:07PM -0700, Casey Schaufler wrote:
> On 9/6/2021 7:55 AM, Dr. David Alan Gilbert wrote:
> > * Casey Schaufler (casey@schaufler-ca.com) wrote:
> >> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> >>>  If LSMs are not configured,
> >>> then hiding the directory is the solution.
> >> It's not a solution at all. It's wishful thinking that
> >> some admin is going to do absolutely everything right, will
> >> never make a mistake and will never, ever, read the mount(2)
> >> man page.
> > That is why we run our virtiofsd with a sandbox setup and seccomp; and
> > frankly anything we can or could turn on we would.
> 
> That doesn't address my concern at all. Being able to create an
> environment in which a feature can be used safely does not make
> the feature safe.

That's the requirement of virtiofs shared directory. It is a shared
directory, potentially being used by untrusted guest. So it should
not be accessible to unprivileged entities on host.

Same is the requirement for regular containers and that's why
podman (and possibly other container managers), make top level
storage directory only readable and searchable by root, so that
unpriveleged entities on host can not access container root filesystem
data.

I think similar requirements are there for idmapped mounts. One
will shift container images into user namespaces. And that means,
through idmapped mounts one can write files which will actually
show up as owned by root in original directory. And that original
directory should not be accessible to unprivileged users otherwise
user found an easy way to drop root owned files on system and
exectute these.

So more and more use cases are there which are relying on directory
not being accessible to unprivileged users.

I understand it is not going to be easy to get the configurations
right all the time. But orchestration tools can be helpful. container
managers can make sure kata container rootfs are not accessible.
libvirt can probably warn if shared directory is accessible to
unprivileged users. And all that will help in getting configuration
right.

> 
> 
> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> Sure you do. If you didn't, you wouldn't be so concerned about
> requiring CAP_SYS_ADMIN. You're trying hard to avoid taking the
> level of responsibility that running with privilege requires.

Running with minimal capabilities is always desired. So we want
to do away with CAP_SYS_ADMIN. And this reduces our risk in
case virtiofsd gets compromised.

I thought we had similar reasons that we did not want setuid
root binaries and wanted to give them limited set of capabilites
depending on what they are doing.

> To do that, you're introducing a massive security hole, a backdoor
> into the file system security attributes.

Shared directory is not accessible to unprivileged entities. If
configuration is not right, then it is user's problem and we
need to fix that.

> 
> >> It comes back to your design, which is fundamentally flawed. You
> >> can't store system security information in an attribute that can
> >> be manipulated by untrusted entities. That's why we have system.*
> >> xattrs. You want to have an attribute on the host that maps to a
> >> security attribute on the guest. The host has to protect the attribute
> >> on the guest with mechanisms of comparable strength as the guest's
> >> mechanisms.
> > Can you just explain this line to me a bit more: 
> >> Otherwise you can't trust the guest with host data.
> > Note we're not trying to trust the guest with the host data here;
> > we're trying to allow the guest to store the data on the host, while
> > trusting the host.
> 
> But you can't trust the host! You're allowing unprivileged processes
> on the host to modify security state of the guest.

No we are not. Shared directory should not be accessible to
unpriviliged entities on host.

> 
> >> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> >> mechanism as implemented today won't scale to the hundreds of individual
> >> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> >> I'm not convinced that there isn't a way to accomplish what you're
> >> trying to do without privilege, but this isn't it, and I don't know
> >> what is. Sorry.
> >>
> >>> Also if directory is not hidden, unprivileged users can change file
> >>> data and other metadata.
> >> I assumed that you've taken that into account. Are you saying that
> >> isn't going to be done correctly either?
> >>
> >>>  Why that's not a concern and why there is
> >>> so much of focus only security xattr.
> >> As with an NFS mount, the assumption is that UID 567 (or its magically
> >> mapped equivalent) has the same access rights on both the server/host
> >> and client/guest. I'm not worried about the mode bits because they are
> >> presented consistently on both machines. If, on the other hand, an
> >> attribute used to determine access is security.esprit on the guest and
> >> user.security.esprit on the host, the unprivileged user on the host
> >> can defeat the privilege requirements on the guest. That's why.
> > We're OK with that;
> 
> I understand that. I  am  not  OK  with  that.

Unprivileged entities can't modify "user.viritiofs.security.selinux"
as shared directory is not accessible to them.

Sorry, I have to repeat this so many times because you are 
completely ignoring this requirement saying users will get it
wrong. Sure if they do get it wrong, they need to fix it.  But
that does not mean we start giving CAP_SYS_ADMIN to daemon. 

If shared direcotry is accessible to unprivileged entities, game
is already over (setuid root binaries). Being able to modify
security attributes of a file seems like such a minor concern
in comparison.

> 
> >  remember that the host can do wth it likes to the
> > guest anyway
> 
> We're not talking about "the host", we're talking about an
> unprivileged user on the host.
> 
> >  - it can just go in and poke at the guests RAM if it wants
> > to do something evil to the guest.
> > We wouldn't suggest using a scheme like this once you have
> > encrypted/protected guest RAM for example (SEV/TDX etc)
> >
> >>>  If you were to block modification
> >>> of file then you will have rely on LSMs.
> >> No. We're talking about the semantics of the xattr namespaces.
> >> LSMs can further constrain access to xattrs, but the basic rules
> >> of access to the user.* and security.* attributes are different
> >> in any case. This is by design.
> > I'm happy if you can suggest somewhere else to store the guests xattr
> > data other than in one of the hosts xattr's - the challenge is doing
> > that in a non-racy way, and making sure that the xattr's never get
> > associated with the wrong file as seen by a guest.
> 
> I'm sorry, but I've got a bunch of other stuff on my plate.
> I've already suggested implementing xattr namespaces a'la user
> namespaces, but I understand that is beyond the scope of your
> current needs, and has its own set of dragons.
> 
> >>>  And if LSMs are not configured,
> >>> then we will rely on shared directory not being visible.
> >> LSMs are not the problem. LSMs use security.* xattrs, which is why
> >> they come up in the discussion.
> >>
> >>> Can you please help me understand why hiding shared directory from
> >>> unprivileged users is not a solution
> >> Maybe you can describe the mechanism you use to "hide" a shared directory
> >> on the host. If the filesystem is mounted on the host it seems unlikely
> >> that you can provide a convincing argument for sufficient protection.
> > Why?
> 
> Because 99-44/100% of admins out there aren't as skilled at "hiding"
> data as you are. Many (I almost said "most". I'm still not sure which.)
> of them don't even know how to use mode bits correctly.

We need to rely on orchestration tools to this by default.
podman already does that for containers. We probably need to
add something to libvirt too and warn users.

Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-14 12:51                 ` Vivek Goyal
@ 2021-09-14 13:56                   ` Casey Schaufler
  2021-09-14 13:59                   ` Bruce Fields
  1 sibling, 0 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-09-14 13:56 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Dr. David Alan Gilbert, viro, linux-fsdevel, linux-kernel,
	virtio-fs, dwalsh, christian.brauner, casey.schaufler,
	linux-security-module, selinux, tytso, miklos, gscrivan, bfields,
	stephen.smalley.work, agruenba, david, Casey Schaufler

On 9/14/2021 5:51 AM, Vivek Goyal wrote:
> Sorry, I have to repeat this so many times because you are 
> completely ignoring this requirement saying users will get it
> wrong. 

And I keep repeating myself because you are still wrong.
Configuration won't fix the problem. Tools won't fix the
problem. Your scheme is broken. Don't do it.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-14 12:51                 ` Vivek Goyal
  2021-09-14 13:56                   ` Casey Schaufler
@ 2021-09-14 13:59                   ` Bruce Fields
  2021-09-14 14:32                     ` Vivek Goyal
  1 sibling, 1 reply; 39+ messages in thread
From: Bruce Fields @ 2021-09-14 13:59 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Casey Schaufler, Dr. David Alan Gilbert, Alexander Viro,
	linux-fsdevel, LKML, virtio-fs, Daniel Walsh, Christian Brauner,
	Casey Schaufler, LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Andreas Gruenbacher,
	Dave Chinner

On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> Same is the requirement for regular containers and that's why
> podman (and possibly other container managers), make top level
> storage directory only readable and searchable by root, so that
> unpriveleged entities on host can not access container root filesystem
> data.

Note--if that directory is on NFS, making it readable and searchable
by root is very weak protection, since it's often possible for an
attacker to guess filehandles and access objects without the need for
directory lookups.

--b.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-14 13:59                   ` Bruce Fields
@ 2021-09-14 14:32                     ` Vivek Goyal
  2021-09-14 15:01                       ` Bruce Fields
  2021-09-15 16:33                       ` Dr. Greg
  0 siblings, 2 replies; 39+ messages in thread
From: Vivek Goyal @ 2021-09-14 14:32 UTC (permalink / raw)
  To: Bruce Fields
  Cc: Casey Schaufler, Dr. David Alan Gilbert, Alexander Viro,
	linux-fsdevel, LKML, virtio-fs, Daniel Walsh, Christian Brauner,
	Casey Schaufler, LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Andreas Gruenbacher,
	Dave Chinner

On Tue, Sep 14, 2021 at 09:59:19AM -0400, Bruce Fields wrote:
> On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > Same is the requirement for regular containers and that's why
> > podman (and possibly other container managers), make top level
> > storage directory only readable and searchable by root, so that
> > unpriveleged entities on host can not access container root filesystem
> > data.
> 
> Note--if that directory is on NFS, making it readable and searchable
> by root is very weak protection, since it's often possible for an
> attacker to guess filehandles and access objects without the need for
> directory lookups.

open_by_handle_at() requires CAP_DAC_READ_SEARCH. And if you have
CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
should be able to read/search through all directories, IIUC.

So how does one make sure that shared directory on host is not
accessible to unprivileged entities. If making directory accessible
to root only is weaker security, what are the options for stronger
security.

Vivek


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-14 14:32                     ` Vivek Goyal
@ 2021-09-14 15:01                       ` Bruce Fields
  2021-09-15 16:33                       ` Dr. Greg
  1 sibling, 0 replies; 39+ messages in thread
From: Bruce Fields @ 2021-09-14 15:01 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Casey Schaufler, Dr. David Alan Gilbert, Alexander Viro,
	linux-fsdevel, LKML, virtio-fs, Daniel Walsh, Christian Brauner,
	Casey Schaufler, LSM, selinux, Theodore Ts'o, Miklos Szeredi,
	Giuseppe Scrivano, stephen.smalley.work, Andreas Gruenbacher,
	Dave Chinner

On Tue, Sep 14, 2021 at 10:32 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> open_by_handle_at() requires CAP_DAC_READ_SEARCH.

Or some sort of access to the network.  If you can send rpc requests
to the nfs server that appear to be from someone with access to the
export, you can guess filehandles that allow access to objects under
that directory.  You'll need access to particular objects, but you
won't need read or lookup access to the directory.

You can prevent that if you set things up right, but these
filehandle-issues are poorly understood, and people often forget to
take them into account.

--b.

> And if you have
> CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
> should be able to read/search through all directories, IIUC.
>
> So how does one make sure that shared directory on host is not
> accessible to unprivileged entities. If making directory accessible
> to root only is weaker security, what are the options for stronger
> security.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v3 0/1] Relax restrictions on user.* xattr
  2021-09-14 14:32                     ` Vivek Goyal
  2021-09-14 15:01                       ` Bruce Fields
@ 2021-09-15 16:33                       ` Dr. Greg
  1 sibling, 0 replies; 39+ messages in thread
From: Dr. Greg @ 2021-09-15 16:33 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Bruce Fields, Casey Schaufler, Dr. David Alan Gilbert,
	Alexander Viro, linux-fsdevel, LKML, virtio-fs, Daniel Walsh,
	Christian Brauner, Casey Schaufler, LSM, selinux,
	Theodore Ts'o, Miklos Szeredi, Giuseppe Scrivano,
	stephen.smalley.work, Andreas Gruenbacher, Dave Chinner

On Tue, Sep 14, 2021 at 10:32:13AM -0400, Vivek Goyal wrote:

Good morning, I hope the day is going well for everyone.

> On Tue, Sep 14, 2021 at 09:59:19AM -0400, Bruce Fields wrote:
> > On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > Same is the requirement for regular containers and that's why
> > > podman (and possibly other container managers), make top level
> > > storage directory only readable and searchable by root, so that
> > > unpriveleged entities on host can not access container root filesystem
> > > data.
> > 
> > Note--if that directory is on NFS, making it readable and searchable
> > by root is very weak protection, since it's often possible for an
> > attacker to guess filehandles and access objects without the need for
> > directory lookups.

> open_by_handle_at() requires CAP_DAC_READ_SEARCH. And if you have
> CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
> should be able to read/search through all directories, IIUC.
>
> So how does one make sure that shared directory on host is not
> accessible to unprivileged entities. If making directory accessible
> to root only is weaker security, what are the options for stronger
> security.

I've been watching this thread, with some interest, given what we have
been working on with respect to providing a new security framework
that merges IMA and LSM and external security co-processor technology.

Some observations based on those experiences and this thread.

Casey is an expert on MAC and capability based security systems,
unfortunately for our industry, particularly bog standard system
administrators, a rarefied set of skills.  It may be helpful to
consider his concerns and position on the issues involved in the
framework of the number of systems that have, and blog posts that
recommend, setting 'selinux=0' on the kernel command-line.

I believe the best summary of his position on this issue, is the
notion that placing security labels, even in transitive form in user
accessible attributes, subordinates the security of the guest system,
regardless of the MAC policy it implements, to the DAC based policy on
the host system.

Given that, there are no legitimate security guarantees that are
inferrable based on the guest MAC policy.

A legitimate pundit, could and probably should question, in the face
of container filesystems and virtual machine images, whether any type
of inferrable security guarantees are possible, but that is a question
and argument for another day.

I didn't see any mention of EVM brought up in these discussions, which
may provide some options to improve the security integrity state of
the guest.

The 800 pound gorilla in the corner in all of this, is that inferrable
security guarantees in guests require a certifiable chain of trust
from the creator of the object to the kernel context that is making
the security gating decisions on the object.  A hard to implement and
prove concept in bare metal trusted systems, let alone the myriad of
edge cases lurking in namespaced and virtual environments.

Which, in a nod to the other corner of the ring, may simply mean, with
our current state of deployable technology, you pay your money and
take your chances in these virtual environments.  Which would in turn
support the notion of a minimum security, ie. DAC, based effort.

> Vivek

Have a good remainder of the week.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 N. 19th Ave.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: dg@enjellic.com
------------------------------------------------------------------------------
"This place is so screwed up.  It's just like the Titanic, only
 we don't even have a band playing.
                                -- Terrance George Wieland
                                   Resurrection.

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2021-09-15 17:00 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 15:22 [PATCH v3 0/1] Relax restrictions on user.* xattr Vivek Goyal
2021-09-02 15:22 ` [PATCH v3 1/1] xattr: Allow user.* xattr on symlink and special files Vivek Goyal
2021-09-02 15:38 ` [PATCH 2/1] man-pages: xattr.7: Update text for user extended xattr behavior change Vivek Goyal
2021-09-02 15:43 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Casey Schaufler
2021-09-02 17:05   ` Vivek Goyal
2021-09-02 17:42     ` Vivek Goyal
2021-09-02 18:55       ` Casey Schaufler
2021-09-02 20:06         ` Vivek Goyal
2021-09-02 22:34           ` Casey Schaufler
2021-09-03 15:26             ` Vivek Goyal
2021-09-03 18:49               ` Casey Schaufler
2021-09-06  7:45             ` [Virtio-fs] " Sergio Lopez
2021-09-06 14:55             ` Dr. David Alan Gilbert
2021-09-13 19:05               ` Casey Schaufler
2021-09-14 12:51                 ` Vivek Goyal
2021-09-14 13:56                   ` Casey Schaufler
2021-09-14 13:59                   ` Bruce Fields
2021-09-14 14:32                     ` Vivek Goyal
2021-09-14 15:01                       ` Bruce Fields
2021-09-15 16:33                       ` Dr. Greg
2021-09-02 15:47 ` [PATCH 3/1] xfstests: generic/062: Do not run on newer kernels Vivek Goyal
2021-09-03  4:55   ` Dave Chinner
2021-09-03  6:31   ` Andreas Gruenbacher
2021-09-03  6:56     ` Andreas Gruenbacher
2021-09-03 14:42       ` Bruce Fields
2021-09-03 15:43         ` Vivek Goyal
2021-09-03 15:50           ` Bruce Fields
2021-09-03 16:01             ` Casey Schaufler
2021-09-03 16:03             ` Vivek Goyal
2021-09-03  6:31   ` Zorro Lang
2021-09-02 15:50 ` [PATCH 4/1] xfstest: Add a new test to test xattr operations Vivek Goyal
2021-09-02 17:52 ` [PATCH v3 0/1] Relax restrictions on user.* xattr Andreas Gruenbacher
2021-09-02 18:48   ` Vivek Goyal
2021-09-02 19:19     ` Casey Schaufler
2021-09-06 14:39   ` Dr. David Alan Gilbert
2021-09-06 14:56     ` Miklos Szeredi
2021-09-07 21:40       ` Vivek Goyal
2021-09-08  7:37         ` Miklos Szeredi
2021-09-08 14:20           ` Eric W. Biederman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).