[RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[James Morris]

I noticed that there are differences in the behavior of listxattr(2) for 
xattrs in the trusted namespace.

Some filesystems, such as ext[234], require CAP_SYS_ADMIN for this, i.e. 
trusted xattr names are hidden from unprivileged users.

I audited the kernel for users of the trusted xattr namespace, and found 
the following filesystems not checking for CAP_SYS_ADMIN:

  - jffs2
  - ocfs2
  - btrfs
  - xfs

I've created patches for jffs2 (tested) and ocfs2 (not tested) to add the 
check -- see following emails. btrfs and xfs have custom listxattr 
operations and will need a bit more work to fix.

I'm not sure what the initial intention was for the behavior, although 
given that several major filesystems are have been fielded with the 
CAP_SYS_ADMIN check, it seems most prudent to make this the standard 
behavior for all filesystems, in case any users are depending on it.

Thoughts?


- James
-- 
James Morris
<jmorris@namei.org>

[PATCH 1/2] jffs2: ensure trusted xattrs are not returned to unprivileged users via listxattr

[James Morris]

Ensure that trusted xattrs are not returned to unprivileged users
via listxattr, in keeping with several other implmentations, such
as ext3.

Signed-off-by: James Morris <jmorris@namei.org>
---
 fs/jffs2/xattr_trusted.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/jffs2/xattr_trusted.c b/fs/jffs2/xattr_trusted.c
index 3e5a5e3..405121c 100644
--- a/fs/jffs2/xattr_trusted.c
+++ b/fs/jffs2/xattr_trusted.c
@@ -39,6 +39,9 @@ static size_t jffs2_trusted_listxattr(struct dentry *dentry, char *list,
 {
 	size_t retlen = XATTR_TRUSTED_PREFIX_LEN + name_len + 1;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return 0;
+
 	if (list && retlen<=list_size) {
 		strcpy(list, XATTR_TRUSTED_PREFIX);
 		strcpy(list + XATTR_TRUSTED_PREFIX_LEN, name);
-- 
1.6.3.3

[PATCH 2/2] ocfs2: ensure trusted xattrs are not returned to unprivileged users via listxattr

[James Morris]

Ensure that trusted xattrs are not returned to unprivileged users
via listxattr, in keeping with several other implmentations, such
as ext3.

Signed-off-by: James Morris <jmorris@namei.org>
---
 fs/ocfs2/xattr.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 8fc6fb0..da98448 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -7077,6 +7077,9 @@ static size_t ocfs2_xattr_trusted_list(struct dentry *dentry, char *list,
 	const size_t prefix_len = XATTR_TRUSTED_PREFIX_LEN;
 	const size_t total_len = prefix_len + name_len + 1;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return 0;
+
 	if (list && total_len <= list_size) {
 		memcpy(list, XATTR_TRUSTED_PREFIX, prefix_len);
 		memcpy(list + prefix_len, name, name_len);
-- 
1.6.3.3

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Andreas Dilger]

On 2010-03-02, at 01:01, James Morris wrote:
> I noticed that there are differences in the behavior of listxattr(2)  
> for
> xattrs in the trusted namespace.
>
> Some filesystems, such as ext[234], require CAP_SYS_ADMIN for this,  
> i.e.
> trusted xattr names are hidden from unprivileged users.

This matches my understanding of the trusted.* namespace.  It is  
settable by the kernel and root (CAP_SYS_ADMIN) but not regular users.

> I'm not sure what the initial intention was for the behavior, although
> given that several major filesystems are have been fielded with the
> CAP_SYS_ADMIN check, it seems most prudent to make this the standard
> behavior for all filesystems, in case any users are depending on it.


Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH 2/2] ocfs2: ensure trusted xattrs are not returned to unprivileged users via listxattr

[Joel Becker]

On Tue, Mar 02, 2010 at 07:02:22PM +1100, James Morris wrote:
> Ensure that trusted xattrs are not returned to unprivileged users
> via listxattr, in keeping with several other implmentations, such
> as ext3.
> 
> Signed-off-by: James Morris <jmorris@namei.org>

If this is the standard expectation, why not lift it up into the vfs?

Acked-by: Joel Becker <joel.becker@oracle.com>

-- 

"The nearest approach to immortality on Earth is a government
 bureau."
	- James F. Byrnes

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH 2/2] ocfs2: ensure trusted xattrs are not returned to unprivileged users via listxattr

[James Morris]

On Tue, 2 Mar 2010, Joel Becker wrote:

> On Tue, Mar 02, 2010 at 07:02:22PM +1100, James Morris wrote:
> > Ensure that trusted xattrs are not returned to unprivileged users
> > via listxattr, in keeping with several other implmentations, such
> > as ext3.
> > 
> > Signed-off-by: James Morris <jmorris@namei.org>
> 
> If this is the standard expectation, why not lift it up into the vfs?

The VFS doesn't know what's in the listxattr list.  When using generic 
xattr handlers for synthetic xattrs, it's easy to determine the xattr 
namespace (each namespace has a handler), although on-disk xattrs need to 
be read from disk to determine if they're in the trusted namespace.

The VFS could possibly modify the listxattr results after the fact, 
although it's very ugly, and it would not be able to correctly handle size 
probes, where the VFS only sees a total size value from the FS.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2] ocfs2: ensure trusted xattrs are not returned to unprivileged users via listxattr

[Serge E. Hallyn]

Quoting Joel Becker (Joel.Becker@oracle.com):
> On Tue, Mar 02, 2010 at 07:02:22PM +1100, James Morris wrote:
> > Ensure that trusted xattrs are not returned to unprivileged users
> > via listxattr, in keeping with several other implmentations, such
> > as ext3.
> > 
> > Signed-off-by: James Morris <jmorris@namei.org>
> 
> If this is the standard expectation, why not lift it up into the vfs?

I wonder why xattr_permission() isn't called from vfs_listxattr()
in fs/xattr.c?  It sure looks like it was done on purpose...

> Acked-by: Joel Becker <joel.becker@oracle.com>
> 
> -- 
> 
> "The nearest approach to immortality on Earth is a government
>  bureau."
> 	- James F. Byrnes
> 
> Joel Becker
> Principal Software Developer
> Oracle
> E-mail: joel.becker@oracle.com
> Phone: (650) 506-8127
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Stephen Smalley]

On Tue, 2010-03-02 at 19:01 +1100, James Morris wrote:
> I noticed that there are differences in the behavior of listxattr(2) for 
> xattrs in the trusted namespace.
> 
> Some filesystems, such as ext[234], require CAP_SYS_ADMIN for this, i.e. 
> trusted xattr names are hidden from unprivileged users.
> 
> I audited the kernel for users of the trusted xattr namespace, and found 
> the following filesystems not checking for CAP_SYS_ADMIN:
> 
>   - jffs2
>   - ocfs2
>   - btrfs
>   - xfs
> 
> I've created patches for jffs2 (tested) and ocfs2 (not tested) to add the 
> check -- see following emails. btrfs and xfs have custom listxattr 
> operations and will need a bit more work to fix.
> 
> I'm not sure what the initial intention was for the behavior, although 
> given that several major filesystems are have been fielded with the 
> CAP_SYS_ADMIN check, it seems most prudent to make this the standard 
> behavior for all filesystems, in case any users are depending on it.
> 
> Thoughts?

Should it be using has_capability_noaudit() rather than capable() so
that merely calling listxattr() on a file that happens to have trusted
xattrs does not set PF_SUPERPRIV on the task and does not trigger an
audit message?

-- 
Stephen Smalley
National Security Agency

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Christoph Hellwig]

On Tue, Mar 02, 2010 at 07:01:05PM +1100, James Morris wrote:
> I noticed that there are differences in the behavior of listxattr(2) for 
> xattrs in the trusted namespace.
> 
> Some filesystems, such as ext[234], require CAP_SYS_ADMIN for this, i.e. 
> trusted xattr names are hidden from unprivileged users.
> 
> I audited the kernel for users of the trusted xattr namespace, and found 
> the following filesystems not checking for CAP_SYS_ADMIN:
> 
>   - jffs2
>   - ocfs2
>   - btrfs
>   - xfs
> 
> I've created patches for jffs2 (tested) and ocfs2 (not tested) to add the 
> check -- see following emails. btrfs and xfs have custom listxattr 
> operations and will need a bit more work to fix.

I think the behaviour of the above filesystems is correct.  There is no
requirement for privilegues to see the existence of these attributes.
We also don't hide entries that aren't readable from readdir output.

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Andreas Gruenbacher]

On Wednesday 03 March 2010 15:09:41 Christoph Hellwig wrote:
> On Tue, Mar 02, 2010 at 07:01:05PM +1100, James Morris wrote:
> > I noticed that there are differences in the behavior of listxattr(2) for 
> > xattrs in the trusted namespace.
> > 
> > Some filesystems, such as ext[234], require CAP_SYS_ADMIN for this, i.e. 
> > trusted xattr names are hidden from unprivileged users.
> > 
> > I audited the kernel for users of the trusted xattr namespace, and found 
> > the following filesystems not checking for CAP_SYS_ADMIN:
> > 
> >   - jffs2
> >   - ocfs2
> >   - btrfs
> >   - xfs
> > 
> > I've created patches for jffs2 (tested) and ocfs2 (not tested) to add the 
> > check -- see following emails. btrfs and xfs have custom listxattr 
> > operations and will need a bit more work to fix.
> 
> I think the behaviour of the above filesystems is correct.  There is no
> requirement for privilegues to see the existence of these attributes.
> We also don't hide entries that aren't readable from readdir output.

The original idea was that regular processes will never have access to 
trusted.* attributes anyway, and so there is little point in listing such 
attributes in the first place.

This is different from user.* attributes which a particular process may or may 
not have access to depending on file permissions.  Checking those permissions 
in listxattr() would have significant overheads, and race with permission 
changes, possibly leading to weird results.  (In contrast, processes don't 
usually listxattr() with privileges and then getxattr() without privileges, or 
vice versa.)

Andreas

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Christoph Hellwig]

On Tue, Mar 02, 2010 at 07:01:05PM +1100, James Morris wrote:
> I audited the kernel for users of the trusted xattr namespace, and found 
> the following filesystems not checking for CAP_SYS_ADMIN:
> 
>   - jffs2
>   - ocfs2
>   - btrfs
>   - xfs

Now that everyone felt the consensus is that we need the check I look
into adding it into XFS, but it seems like we already have that check
in xfs_xattr_put_listent:

	/*
         * Only show root namespace entries if we are actually allowed to
	 * see them.
	 */
	if ((flags & XFS_ATTR_ROOT) && !capable(CAP_SYS_ADMIN))
		return 0;

Can you send me the testcases where XFs shows trusted attributes?

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[Andreas Gruenbacher]

On Wednesday 03 March 2010 14:02:58 Stephen Smalley wrote:
> Should it be using has_capability_noaudit() rather than capable() so
> that merely calling listxattr() on a file that happens to have trusted
> xattrs does not set PF_SUPERPRIV on the task and does not trigger an
> audit message?

Yes, makes sense.  A version of has_capability_noaudit() without an explicit 
task parameter, like security_capable(), would be better still.

Thanks,
Andreas

Re: [RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs

[James Morris]

On Wed, 10 Mar 2010, Christoph Hellwig wrote:

> Now that everyone felt the consensus is that we need the check I look
> into adding it into XFS, but it seems like we already have that check
> in xfs_xattr_put_listent:
> 
> 	/*
>          * Only show root namespace entries if we are actually allowed to
> 	 * see them.
> 	 */
> 	if ((flags & XFS_ATTR_ROOT) && !capable(CAP_SYS_ADMIN))
> 		return 0;
> 

Looks like I didn't understand what the XFS code was doing.


- James
-- 
James Morris
<jmorris@namei.org>