[RFC][PATCH 0/2] Correct behavior for listxattr and 'trusted' xattrs
James Morris
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
<jmo...@namei.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
James Morris
via listxattr, in keeping with several other implmentations, such
as ext3.
Signed-off-by: James Morris <jmo...@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
James Morris
via listxattr, in keeping with several other implmentations, such
as ext3.
Signed-off-by: James Morris <jmo...@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
Andreas Dilger
> 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.
Joel Becker
> 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 <jmo...@namei.org>
If this is the standard expectation, why not lift it up into the vfs?
Acked-by: Joel Becker <joel....@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....@oracle.com
Phone: (650) 506-8127
James Morris
> 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 <jmo...@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
<jmo...@namei.org>
Serge E. Hallyn
> 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 <jmo...@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....@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....@oracle.com
> Phone: (650) 506-8127
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
Stephen Smalley
> 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
Christoph Hellwig
> 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.
Andreas Gruenbacher
> 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
Christoph Hellwig
> 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?
Andreas Gruenbacher
> 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
James Morris
> 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
<jmo...@namei.org>