* [PATCH] ovl: update S_ISGID when setting posix ACLs @ 2016-10-26 17:10 Amir Goldstein 2016-10-26 18:30 ` [PATCH v2] " Amir Goldstein 0 siblings, 1 reply; 7+ messages in thread From: Amir Goldstein @ 2016-10-26 17:10 UTC (permalink / raw) To: Miklos Szeredi Cc: Vivek Goyal, Andreas Gruenbacher, linux-unionfs, linux-fsdevel Since operations on upper are performed using mounter's credentials, we need to call posix_acl_update_mode() with user credentials on overlay inode to possibly copy-up and clear setgid bit, before setting extended attributes on upper inode. This change fixes xfstest generic/375, which failed to clear the setgid bit in the following test case over overlayfs: touch $testfile chown 100:100 $testfile chmod 2755 $testfile _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/super.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 30263a5..16cc844 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -522,6 +522,22 @@ static unsigned int ovl_split_lowerdirs(char *str) return ctr; } +static int +ovl_set_mode(struct dentry *dentry, umode_t mode) +{ + struct iattr iattr; + int err; + + if (mode == d_inode(dentry)->i_mode) + return 0; + + iattr.ia_valid = ATTR_MODE | ATTR_CTIME; + iattr.ia_mode = mode; + iattr.ia_ctime = current_time(d_inode(dentry)); + + return ovl_setattr(dentry, &iattr); +} + static int __maybe_unused ovl_posix_acl_xattr_get(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, @@ -560,6 +576,18 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, if (!inode_owner_or_capable(inode)) goto out_acl_release; + if (handler->flags == ACL_TYPE_ACCESS) { + umode_t mode; + struct posix_acl *newacl = acl; + + err = posix_acl_update_mode(inode, &mode, &newacl); + if (err) + goto out_acl_release; + err = ovl_set_mode(dentry, mode); + if (err) + goto out_acl_release; + } + posix_acl_release(acl); err = ovl_xattr_set(dentry, handler->name, value, size, flags); -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-26 17:10 [PATCH] ovl: update S_ISGID when setting posix ACLs Amir Goldstein @ 2016-10-26 18:30 ` Amir Goldstein 2016-10-26 18:52 ` Andreas Gruenbacher 2016-10-27 22:37 ` Vivek Goyal 0 siblings, 2 replies; 7+ messages in thread From: Amir Goldstein @ 2016-10-26 18:30 UTC (permalink / raw) To: Miklos Szeredi Cc: Vivek Goyal, Andreas Gruenbacher, linux-unionfs, linux-fsdevel Since operations on upper are performed using mounter's credentials, we need to call posix_acl_update_mode() with current credentials on overlay inode to possibly copy-up and clear setgid bit, before setting posix ACLs on upper inode. Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to avoid compiler warning for implicit declaration of function 'posix_acl_update_mode' on build without that config option. This change fixes xfstest generic/375, which failed to clear the setgid bit in the following test case over overlayfs: touch $testfile chown 100:100 $testfile chmod 2755 $testfile _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/super.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 30263a5..7071790 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -523,6 +523,22 @@ static unsigned int ovl_split_lowerdirs(char *str) } static int __maybe_unused +ovl_set_mode(struct dentry *dentry, umode_t mode) +{ + struct iattr iattr; + + if (mode == d_inode(dentry)->i_mode) + return 0; + + iattr.ia_valid = ATTR_MODE | ATTR_CTIME; + iattr.ia_mode = mode; + iattr.ia_ctime = current_time(d_inode(dentry)); + + return ovl_setattr(dentry, &iattr); +} + +#ifdef CONFIG_FS_POSIX_ACL +static int __maybe_unused ovl_posix_acl_xattr_get(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, const char *name, void *buffer, size_t size) @@ -560,6 +576,18 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, if (!inode_owner_or_capable(inode)) goto out_acl_release; + if (handler->flags == ACL_TYPE_ACCESS) { + umode_t mode; + struct posix_acl *newacl = acl; + + err = posix_acl_update_mode(inode, &mode, &newacl); + if (err) + goto out_acl_release; + err = ovl_set_mode(dentry, mode); + if (err) + goto out_acl_release; + } + posix_acl_release(acl); err = ovl_xattr_set(dentry, handler->name, value, size, flags); @@ -572,6 +600,7 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, posix_acl_release(acl); return err; } +#endif static int ovl_own_xattr_get(const struct xattr_handler *handler, struct dentry *dentry, struct inode *inode, @@ -603,6 +632,7 @@ static int ovl_other_xattr_set(const struct xattr_handler *handler, return ovl_xattr_set(dentry, name, value, size, flags); } +#ifdef CONFIG_FS_POSIX_ACL static const struct xattr_handler __maybe_unused ovl_posix_acl_access_xattr_handler = { .name = XATTR_NAME_POSIX_ACL_ACCESS, @@ -618,6 +648,7 @@ ovl_posix_acl_default_xattr_handler = { .get = ovl_posix_acl_xattr_get, .set = ovl_posix_acl_xattr_set, }; +#endif static const struct xattr_handler ovl_own_xattr_handler = { .prefix = OVL_XATTR_PREFIX, -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-26 18:30 ` [PATCH v2] " Amir Goldstein @ 2016-10-26 18:52 ` Andreas Gruenbacher 2016-10-27 22:37 ` Vivek Goyal 1 sibling, 0 replies; 7+ messages in thread From: Andreas Gruenbacher @ 2016-10-26 18:52 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Vivek Goyal, linux-unionfs, linux-fsdevel Amir, On Wed, Oct 26, 2016 at 8:30 PM, Amir Goldstein <amir73il@gmail.com> wrote: > Since operations on upper are performed using mounter's credentials, > we need to call posix_acl_update_mode() with current credentials on > overlay inode to possibly copy-up and clear setgid bit, before setting > posix ACLs on upper inode. I'll let Miklos decide if this is the right approach. > Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to > avoid compiler warning for implicit declaration of function > 'posix_acl_update_mode' on build without that config option. If you do that, make sure to remove the __maybe_unused declarations in that file well: they were added to silence the compiler when CONFIG_FS_POSIX_ACL isn't set. Or else add a dummy posix_acl_update_mode function to include/linux/posix_acl.h. Thanks, Andreas ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-26 18:30 ` [PATCH v2] " Amir Goldstein 2016-10-26 18:52 ` Andreas Gruenbacher @ 2016-10-27 22:37 ` Vivek Goyal 2016-10-28 4:47 ` Amir Goldstein 1 sibling, 1 reply; 7+ messages in thread From: Vivek Goyal @ 2016-10-27 22:37 UTC (permalink / raw) To: Amir Goldstein Cc: Miklos Szeredi, Andreas Gruenbacher, linux-unionfs, linux-fsdevel On Wed, Oct 26, 2016 at 09:30:16PM +0300, Amir Goldstein wrote: > Since operations on upper are performed using mounter's credentials, > we need to call posix_acl_update_mode() with current credentials on > overlay inode to possibly copy-up and clear setgid bit, before setting > posix ACLs on upper inode. > > Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to > avoid compiler warning for implicit declaration of function > 'posix_acl_update_mode' on build without that config option. > > This change fixes xfstest generic/375, which failed to clear the > setgid bit in the following test case over overlayfs: > > touch $testfile > chown 100:100 $testfile > chmod 2755 $testfile > _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/super.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 30263a5..7071790 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -523,6 +523,22 @@ static unsigned int ovl_split_lowerdirs(char *str) > } > > static int __maybe_unused > +ovl_set_mode(struct dentry *dentry, umode_t mode) > +{ > + struct iattr iattr; > + > + if (mode == d_inode(dentry)->i_mode) > + return 0; > + > + iattr.ia_valid = ATTR_MODE | ATTR_CTIME; > + iattr.ia_mode = mode; > + iattr.ia_ctime = current_time(d_inode(dentry)); > + > + return ovl_setattr(dentry, &iattr); > +} > + > +#ifdef CONFIG_FS_POSIX_ACL > +static int __maybe_unused > ovl_posix_acl_xattr_get(const struct xattr_handler *handler, > struct dentry *dentry, struct inode *inode, > const char *name, void *buffer, size_t size) > @@ -560,6 +576,18 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, > if (!inode_owner_or_capable(inode)) > goto out_acl_release; > > + if (handler->flags == ACL_TYPE_ACCESS) { > + umode_t mode; > + struct posix_acl *newacl = acl; > + > + err = posix_acl_update_mode(inode, &mode, &newacl); > + if (err) > + goto out_acl_release; > + err = ovl_set_mode(dentry, mode); > + if (err) > + goto out_acl_release; > + } > + > posix_acl_release(acl); > > err = ovl_xattr_set(dentry, handler->name, value, size, flags); > @@ -572,6 +600,7 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, Hi Amir, What happens if ovl_set_mode() is successful but ovl_xattr_set() fails. We will leave file with S_ISGID clear? Vivek > posix_acl_release(acl); > return err; > } > +#endif > > static int ovl_own_xattr_get(const struct xattr_handler *handler, > struct dentry *dentry, struct inode *inode, > @@ -603,6 +632,7 @@ static int ovl_other_xattr_set(const struct xattr_handler *handler, > return ovl_xattr_set(dentry, name, value, size, flags); > } > > +#ifdef CONFIG_FS_POSIX_ACL > static const struct xattr_handler __maybe_unused > ovl_posix_acl_access_xattr_handler = { > .name = XATTR_NAME_POSIX_ACL_ACCESS, > @@ -618,6 +648,7 @@ ovl_posix_acl_default_xattr_handler = { > .get = ovl_posix_acl_xattr_get, > .set = ovl_posix_acl_xattr_set, > }; > +#endif > > static const struct xattr_handler ovl_own_xattr_handler = { > .prefix = OVL_XATTR_PREFIX, > -- > 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-27 22:37 ` Vivek Goyal @ 2016-10-28 4:47 ` Amir Goldstein 2016-10-28 12:54 ` Miklos Szeredi 0 siblings, 1 reply; 7+ messages in thread From: Amir Goldstein @ 2016-10-28 4:47 UTC (permalink / raw) To: Vivek Goyal Cc: Miklos Szeredi, Andreas Gruenbacher, linux-unionfs, linux-fsdevel On Fri, Oct 28, 2016 at 1:37 AM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Wed, Oct 26, 2016 at 09:30:16PM +0300, Amir Goldstein wrote: >> Since operations on upper are performed using mounter's credentials, >> we need to call posix_acl_update_mode() with current credentials on >> overlay inode to possibly copy-up and clear setgid bit, before setting >> posix ACLs on upper inode. >> >> Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to >> avoid compiler warning for implicit declaration of function >> 'posix_acl_update_mode' on build without that config option. >> >> This change fixes xfstest generic/375, which failed to clear the >> setgid bit in the following test case over overlayfs: >> >> touch $testfile >> chown 100:100 $testfile >> chmod 2755 $testfile >> _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/overlayfs/super.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c >> index 30263a5..7071790 100644 >> --- a/fs/overlayfs/super.c >> +++ b/fs/overlayfs/super.c >> @@ -523,6 +523,22 @@ static unsigned int ovl_split_lowerdirs(char *str) >> } >> >> static int __maybe_unused >> +ovl_set_mode(struct dentry *dentry, umode_t mode) >> +{ >> + struct iattr iattr; >> + >> + if (mode == d_inode(dentry)->i_mode) >> + return 0; >> + >> + iattr.ia_valid = ATTR_MODE | ATTR_CTIME; >> + iattr.ia_mode = mode; >> + iattr.ia_ctime = current_time(d_inode(dentry)); >> + >> + return ovl_setattr(dentry, &iattr); >> +} >> + >> +#ifdef CONFIG_FS_POSIX_ACL >> +static int __maybe_unused >> ovl_posix_acl_xattr_get(const struct xattr_handler *handler, >> struct dentry *dentry, struct inode *inode, >> const char *name, void *buffer, size_t size) >> @@ -560,6 +576,18 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, >> if (!inode_owner_or_capable(inode)) >> goto out_acl_release; >> >> + if (handler->flags == ACL_TYPE_ACCESS) { >> + umode_t mode; >> + struct posix_acl *newacl = acl; >> + >> + err = posix_acl_update_mode(inode, &mode, &newacl); >> + if (err) >> + goto out_acl_release; >> + err = ovl_set_mode(dentry, mode); >> + if (err) >> + goto out_acl_release; >> + } >> + >> posix_acl_release(acl); >> >> err = ovl_xattr_set(dentry, handler->name, value, size, flags); >> @@ -572,6 +600,7 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler, > > Hi Amir, > > What happens if ovl_set_mode() is successful but ovl_xattr_set() fails. We > will leave file with S_ISGID clear? > Yes, we will, and that is the same thing that both xfs and ext4 do. We already tested that current user has sufficient credentials to perform the operation (and some other validity checks). IMO, if user has the right to perform an operation with the knowledge that this operation will clear S_ISGID and user executes this operation, then at least from security point of view, the outcome of clearing the bit even though the operation itself failed makes sense. Thanks, Amir. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-28 4:47 ` Amir Goldstein @ 2016-10-28 12:54 ` Miklos Szeredi 2016-10-28 19:22 ` Amir Goldstein 0 siblings, 1 reply; 7+ messages in thread From: Miklos Szeredi @ 2016-10-28 12:54 UTC (permalink / raw) To: Amir Goldstein Cc: Vivek Goyal, Andreas Gruenbacher, linux-unionfs, linux-fsdevel On Fri, Oct 28, 2016 at 07:47:12AM +0300, Amir Goldstein wrote: > On Fri, Oct 28, 2016 at 1:37 AM, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, Oct 26, 2016 at 09:30:16PM +0300, Amir Goldstein wrote: > >> Since operations on upper are performed using mounter's credentials, > >> we need to call posix_acl_update_mode() with current credentials on > >> overlay inode to possibly copy-up and clear setgid bit, before setting > >> posix ACLs on upper inode. > >> > >> Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to > >> avoid compiler warning for implicit declaration of function > >> 'posix_acl_update_mode' on build without that config option. > >> > >> This change fixes xfstest generic/375, which failed to clear the > >> setgid bit in the following test case over overlayfs: > >> > >> touch $testfile > >> chown 100:100 $testfile > >> chmod 2755 $testfile > >> _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile Instead of calculating and setting the equivalent mode in overlayfs code (as well as in the upper layer later), how about just clearing the sgid bit when necessary? Untested patch follows. Thanks, Miklos --- fs/overlayfs/super.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -562,6 +562,21 @@ ovl_posix_acl_xattr_set(const struct xat posix_acl_release(acl); + /* + * Check if sgid bit needs to be cleared (actual setacl operation will + * be done with mounter's capabilities and so that won't do it for us). + */ + if (unlikely(inode->i_mode & S_ISGID) && + handler->flags == ACL_TYPE_ACCESS && + !in_group_p(inode->i_gid) && + !capable_wrt_inode_uidgid(inode, CAP_FSETID)) { + struct iattr iattr = { .ia_valid = ATTR_KILL_SGID }; + + err = ovl_setattr(dentry, &iattr); + if (err) + return err; + } + err = ovl_xattr_set(dentry, handler->name, value, size, flags); if (!err) ovl_copyattr(ovl_inode_real(inode, NULL), inode); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: update S_ISGID when setting posix ACLs 2016-10-28 12:54 ` Miklos Szeredi @ 2016-10-28 19:22 ` Amir Goldstein 0 siblings, 0 replies; 7+ messages in thread From: Amir Goldstein @ 2016-10-28 19:22 UTC (permalink / raw) To: Miklos Szeredi Cc: Vivek Goyal, Andreas Gruenbacher, linux-unionfs, linux-fsdevel On Fri, Oct 28, 2016 at 3:54 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Fri, Oct 28, 2016 at 07:47:12AM +0300, Amir Goldstein wrote: >> On Fri, Oct 28, 2016 at 1:37 AM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > On Wed, Oct 26, 2016 at 09:30:16PM +0300, Amir Goldstein wrote: >> >> Since operations on upper are performed using mounter's credentials, >> >> we need to call posix_acl_update_mode() with current credentials on >> >> overlay inode to possibly copy-up and clear setgid bit, before setting >> >> posix ACLs on upper inode. >> >> >> >> Also wrap posix acl handlers with #ifdef CONFIG_FS_POSIX_ACL to >> >> avoid compiler warning for implicit declaration of function >> >> 'posix_acl_update_mode' on build without that config option. >> >> >> >> This change fixes xfstest generic/375, which failed to clear the >> >> setgid bit in the following test case over overlayfs: >> >> >> >> touch $testfile >> >> chown 100:100 $testfile >> >> chmod 2755 $testfile >> >> _runas -u 100 -g 101 -- setfacl -m u::rwx,g::rwx,o::rwx $testfile > > Instead of calculating and setting the equivalent mode in overlayfs code (as > well as in the upper layer later), how about just clearing the sgid bit when > necessary? > > Untested patch follows. > Tested your patch. It fixes generic/375 and passes xfs/pjd/union tests. You may re-cycle my commit message (relevant parts) and either keep my Signed-off-by or add Tested-by me Thanks, Amir. > Thanks, > Miklos > > --- > fs/overlayfs/super.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -562,6 +562,21 @@ ovl_posix_acl_xattr_set(const struct xat > > posix_acl_release(acl); > > + /* > + * Check if sgid bit needs to be cleared (actual setacl operation will > + * be done with mounter's capabilities and so that won't do it for us). > + */ > + if (unlikely(inode->i_mode & S_ISGID) && > + handler->flags == ACL_TYPE_ACCESS && > + !in_group_p(inode->i_gid) && > + !capable_wrt_inode_uidgid(inode, CAP_FSETID)) { > + struct iattr iattr = { .ia_valid = ATTR_KILL_SGID }; > + > + err = ovl_setattr(dentry, &iattr); > + if (err) > + return err; > + } > + > err = ovl_xattr_set(dentry, handler->name, value, size, flags); > if (!err) > ovl_copyattr(ovl_inode_real(inode, NULL), inode); ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-28 19:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-26 17:10 [PATCH] ovl: update S_ISGID when setting posix ACLs Amir Goldstein 2016-10-26 18:30 ` [PATCH v2] " Amir Goldstein 2016-10-26 18:52 ` Andreas Gruenbacher 2016-10-27 22:37 ` Vivek Goyal 2016-10-28 4:47 ` Amir Goldstein 2016-10-28 12:54 ` Miklos Szeredi 2016-10-28 19:22 ` Amir Goldstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).