| Message ID | ddf1fb9b5001e633e0022dee7fecb0ef431e851f.1512041070.git.dongsu@kinvolk.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: > From: Seth Forshee <seth.forshee@canonical.com> > > Expand the check in should_remove_suid() to keep privileges for I realize this description came from Seth, but reading it now, 'Expand' seems wrong. Expanding a check brings to my mind making it stricter, not looser. How about 'Relax the check' ? > CAP_FSETID in s_user_ns rather than init_user_ns. > > Patch v4 is available: https://patchwork.kernel.org/patch/8944621/ > > --EWB Changed from ns_capable(sb->s_user_ns, ) to capable_wrt_inode_uidgid Why exactly? This is wrong, because capable_wrt_inode_uidgid() does a check against current_user_ns, not the inode->i_sb->s_user_ns > > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Serge Hallyn <serge@hallyn.com> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Dongsu Park <dongsu@kinvolk.io> > --- > fs/inode.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index fd401028..6459a437 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1749,7 +1749,8 @@ EXPORT_SYMBOL(touch_atime); > */ > int should_remove_suid(struct dentry *dentry) > { > - umode_t mode = d_inode(dentry)->i_mode; > + struct inode *inode = d_inode(dentry); > + umode_t mode = inode->i_mode; > int kill = 0; > > /* suid always must be killed */ > @@ -1763,7 +1764,8 @@ int should_remove_suid(struct dentry *dentry) > if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > kill |= ATTR_KILL_SGID; > > - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) > + if (unlikely(kill && !capable_wrt_inode_uidgid(inode, CAP_FSETID) && > + S_ISREG(mode))) > return kill; > > return 0; > -- > 2.13.6
Hi, On Sat, Dec 23, 2017 at 4:26 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: >> From: Seth Forshee <seth.forshee@canonical.com> >> >> Expand the check in should_remove_suid() to keep privileges for > > I realize this description came from Seth, but reading it now, > 'Expand' seems wrong. Expanding a check brings to my mind making > it stricter, not looser. How about 'Relax the check' ? Makes sense. Will do. >> CAP_FSETID in s_user_ns rather than init_user_ns. >> >> Patch v4 is available: https://patchwork.kernel.org/patch/8944621/ >> >> --EWB Changed from ns_capable(sb->s_user_ns, ) to capable_wrt_inode_uidgid > > Why exactly? > > This is wrong, because capable_wrt_inode_uidgid() does a check > against current_user_ns, not the inode->i_sb->s_user_ns Ah. I see. I suppose it was changed probably for the privileged_wrt_inode_uidgid() called by capable_wrt_inode_uidgid(). But as you pointed out, that checks against current_user_ns, which is wrong. I would just create another wrapper like capable_userns_wrt_inode_uidgid(), which takes an additional parameter of (struct user_namespace *), to be able to check for both ns_capable() and privileged_wrt_inode_uidgid(). Thanks, Dongsu >> Cc: linux-fsdevel@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> >> Cc: Serge Hallyn <serge@hallyn.com> >> Signed-off-by: Seth Forshee <seth.forshee@canonical.com> >> Signed-off-by: Dongsu Park <dongsu@kinvolk.io> >> --- >> fs/inode.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index fd401028..6459a437 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -1749,7 +1749,8 @@ EXPORT_SYMBOL(touch_atime); >> */ >> int should_remove_suid(struct dentry *dentry) >> { >> - umode_t mode = d_inode(dentry)->i_mode; >> + struct inode *inode = d_inode(dentry); >> + umode_t mode = inode->i_mode; >> int kill = 0; >> >> /* suid always must be killed */ >> @@ -1763,7 +1764,8 @@ int should_remove_suid(struct dentry *dentry) >> if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) >> kill |= ATTR_KILL_SGID; >> >> - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) >> + if (unlikely(kill && !capable_wrt_inode_uidgid(inode, CAP_FSETID) && >> + S_ISREG(mode))) >> return kill; >> >> return 0; >> -- >> 2.13.6
On Sat, Dec 23, 2017 at 1:38 PM, Dongsu Park <dongsu@kinvolk.io> wrote: > Hi, > > On Sat, Dec 23, 2017 at 4:26 AM, Serge E. Hallyn <serge@hallyn.com> wrote: >> On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: >>> From: Seth Forshee <seth.forshee@canonical.com> >>> >>> Expand the check in should_remove_suid() to keep privileges for >> >> I realize this description came from Seth, but reading it now, >> 'Expand' seems wrong. Expanding a check brings to my mind making >> it stricter, not looser. How about 'Relax the check' ? > > Makes sense. Will do. > >>> CAP_FSETID in s_user_ns rather than init_user_ns. >>> >>> Patch v4 is available: https://patchwork.kernel.org/patch/8944621/ >>> >>> --EWB Changed from ns_capable(sb->s_user_ns, ) to capable_wrt_inode_uidgid >> >> Why exactly? >> >> This is wrong, because capable_wrt_inode_uidgid() does a check >> against current_user_ns, not the inode->i_sb->s_user_ns I'm thoroughly confused. s_user_ns is supposed to be about the usernamespace the filesystem perceives to be in, right? How does that come into play when checking permissions to do something? Thanks, Miklos
diff --git a/fs/inode.c b/fs/inode.c index fd401028..6459a437 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1749,7 +1749,8 @@ EXPORT_SYMBOL(touch_atime); */ int should_remove_suid(struct dentry *dentry) { - umode_t mode = d_inode(dentry)->i_mode; + struct inode *inode = d_inode(dentry); + umode_t mode = inode->i_mode; int kill = 0; /* suid always must be killed */ @@ -1763,7 +1764,8 @@ int should_remove_suid(struct dentry *dentry) if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) kill |= ATTR_KILL_SGID; - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) + if (unlikely(kill && !capable_wrt_inode_uidgid(inode, CAP_FSETID) && + S_ISREG(mode))) return kill; return 0;