* [PATCH] fs: fix xattr permission checking error @ 2017-10-21 13:39 Nicolas Belouin 2017-10-21 19:48 ` Andreas Dilger 2017-10-22 5:39 ` Theodore Ts'o 0 siblings, 2 replies; 4+ messages in thread From: Nicolas Belouin @ 2017-10-21 13:39 UTC (permalink / raw) To: Dave Kleikamp, linux-fsdevel, linux-kernel, jfs-discussion Cc: Nicolas Belouin Fix an issue making trusted xattr world readable and other cap_sys_admin only Signed-off-by: Nicolas Belouin <nicolas@belouin.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); } 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)); } ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size) -- 2.14.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fs: fix xattr permission checking error 2017-10-21 13:39 [PATCH] fs: fix xattr permission checking error Nicolas Belouin @ 2017-10-21 19:48 ` Andreas Dilger 2017-10-21 20:00 ` Nicolas Belouin 2017-10-22 5:39 ` Theodore Ts'o 1 sibling, 1 reply; 4+ messages in thread From: Andreas Dilger @ 2017-10-21 19:48 UTC (permalink / raw) To: Nicolas Belouin Cc: Dave Kleikamp, linux-fsdevel, Linux Kernel Mailing List, jfs-discussion [-- Attachment #1: Type: text/plain, Size: 2300 bytes --] On Oct 21, 2017, at 7:39 AM, Nicolas Belouin <nicolas@belouin.fr> wrote: > > Fix an issue making trusted xattr world readable and other > cap_sys_admin only > > Signed-off-by: Nicolas Belouin <nicolas@belouin.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 [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fs: fix xattr permission checking error 2017-10-21 19:48 ` Andreas Dilger @ 2017-10-21 20:00 ` Nicolas Belouin 0 siblings, 0 replies; 4+ messages in thread From: Nicolas Belouin @ 2017-10-21 20:00 UTC (permalink / raw) To: Andreas Dilger Cc: Dave Kleikamp, linux-fsdevel, Linux Kernel Mailing List, jfs-discussion Indeed, I was mistaken here. Thanks for the review. I will do the "strncmp() ==0" in another patch as it is particularly unclear in this context. On October 21, 2017 9:48:16 PM GMT+02:00, Andreas Dilger <adilger@dilger.ca> wrote: >On Oct 21, 2017, at 7:39 AM, Nicolas Belouin <nicolas@belouin.fr> >wrote: >> >> Fix an issue making trusted xattr world readable and other >> cap_sys_admin only >> >> Signed-off-by: Nicolas Belouin <nicolas@belouin.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 Nicolas ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fs: fix xattr permission checking error 2017-10-21 13:39 [PATCH] fs: fix xattr permission checking error Nicolas Belouin 2017-10-21 19:48 ` Andreas Dilger @ 2017-10-22 5:39 ` Theodore Ts'o 1 sibling, 0 replies; 4+ messages in thread From: Theodore Ts'o @ 2017-10-22 5:39 UTC (permalink / raw) To: Nicolas Belouin Cc: Dave Kleikamp, linux-fsdevel, linux-kernel, jfs-discussion On Sat, Oct 21, 2017 at 03:39:47PM +0200, Nicolas Belouin wrote: > Fix an issue making trusted xattr world readable and other > cap_sys_admin only > NACK. It is *documented* that trusted xattrs are only supposed to be readable by root. - Ted ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-22 5:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-21 13:39 [PATCH] fs: fix xattr permission checking error Nicolas Belouin 2017-10-21 19:48 ` Andreas Dilger 2017-10-21 20:00 ` Nicolas Belouin 2017-10-22 5:39 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®