linux-kernel - Re: [PATCH] fs: fix xattr permission checking error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 Oct 2017 13:48:16 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Nicolas Belouin <nicolas@...ouin.fr>
Cc:     Dave Kleikamp <shaggy@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        jfs-discussion@...ts.sourceforge.net
Subject: Re: [PATCH] fs: fix xattr permission checking error

On Oct 21, 2017, at 7:39 AM, Nicolas Belouin <nicolas@...ouin.fr> wrote:
> 
> Fix an issue making trusted xattr world readable and other
> cap_sys_admin only
> 
> Signed-off-by: Nicolas Belouin <nicolas@...ouin.fr>
> ---
> fs/hfsplus/xattr.c | 2 +-
> fs/jfs/xattr.c     | 5 ++---
> 2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index d37bb88dc746..ae03a19196ef 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -604,7 +604,7 @@ static inline int can_list(const char *xattr_name)
> 	if (!xattr_name)
> 		return 0;
> 
> -	return strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
> +	return !strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
> 			XATTR_TRUSTED_PREFIX_LEN) ||
> 				capable(CAP_SYS_ADMIN);

I don't think this is correct.  This means "you can list the xattr if
it IS 'trusted.*', OR if you have sysadmin privilege", so non-trusted
xattrs could not be listed by regular users.

As can be seen by this defect, the use of "strncmp()" with an explicit
boolean return code is confusing and subject to errors, in particular
"strncmp()" returning 0 (false) means the strings MATCH.  My preference
is to explicitly check "strncmp() == 0" for the match, as this is more
clear to the reader that strncmp() has a non-standard return convention.

To my reading, the original logic is correct, which is "you can list
the xattr if it is not 'trusted.*' OR if you have sysadmin privilege",
but it could be improved like:

	return strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
		       XATTR_TRUSTED_PREFIX_LEN) != 0 ||
		capable(CAP_SYS_ADMIN);

> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
> index c60f3d32ee91..1c46573a96ed 100644
> --- a/fs/jfs/xattr.c
> +++ b/fs/jfs/xattr.c
> @@ -858,9 +858,8 @@ ssize_t __jfs_getxattr(struct inode *inode, const char *name, void *data,
>  */
> static inline int can_list(struct jfs_ea *ea)
> {
> -	return (strncmp(ea->name, XATTR_TRUSTED_PREFIX,
> -			    XATTR_TRUSTED_PREFIX_LEN) ||
> -		capable(CAP_SYS_ADMIN));
> +	return (!strncmp(ea->name, XATTR_TRUSTED_PREFIX,
> +			 XATTR_TRUSTED_PREFIX_LEN) || capable(CAP_SYS_ADMIN));
> }

I think the original code is also correct here, and your patch is adding
a bug.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)