From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932259AbdJUTsY (ORCPT ); Sat, 21 Oct 2017 15:48:24 -0400 Received: from mail-it0-f68.google.com ([209.85.214.68]:45530 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932231AbdJUTsW (ORCPT ); Sat, 21 Oct 2017 15:48:22 -0400 X-Google-Smtp-Source: ABhQp+Qvu1qQxNVaJD75Y6tCE93nwqS458kez0MtHf6inl7DjxqVjnpUO3/cDLn1K+iVV+q1xiOllg== From: Andreas Dilger Message-Id: <94E1EFF2-281B-4ACD-98FF-D194F4F35EDB@dilger.ca> Content-Type: multipart/signed; boundary="Apple-Mail=_DC3C9E18-32BB-443F-BA5B-7A62606BB17C"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] fs: fix xattr permission checking error Date: Sat, 21 Oct 2017 13:48:16 -0600 In-Reply-To: <20171021133947.19935-1-nicolas@belouin.fr> Cc: Dave Kleikamp , linux-fsdevel , Linux Kernel Mailing List , jfs-discussion@lists.sourceforge.net To: Nicolas Belouin References: <20171021133947.19935-1-nicolas@belouin.fr> X-Mailer: Apple Mail (2.3273) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Apple-Mail=_DC3C9E18-32BB-443F-BA5B-7A62606BB17C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii On Oct 21, 2017, at 7:39 AM, Nicolas Belouin wrote: >=20 > Fix an issue making trusted xattr world readable and other > cap_sys_admin only >=20 > Signed-off-by: Nicolas Belouin > --- > fs/hfsplus/xattr.c | 2 +- > fs/jfs/xattr.c | 5 ++--- > 2 files changed, 3 insertions(+), 4 deletions(-) >=20 > 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; >=20 > - 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() =3D=3D 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) !=3D 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 --Apple-Mail=_DC3C9E18-32BB-443F-BA5B-7A62606BB17C Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- Comment: GPGTools - http://gpgtools.org iD8DBQFZ66SBpIg59Q01vtYRAlcFAJ4zq6RCZOcsX4Wari5b9Os071zoQACgyKXQ JPrp16p/c3jBd89CJhtxQck= =/All -----END PGP SIGNATURE----- --Apple-Mail=_DC3C9E18-32BB-443F-BA5B-7A62606BB17C--