Quoting Seth Arnold ([EMAIL PROTECTED]):
> On Fri, Jul 27, 2007 at 12:31:11PM -0500, Serge E. Hallyn wrote:
> > When you
> >
> > setfcaps -c cap_net_admin=p -e /bin/ping
> > cp /bin/sh /bin/ping
> >
> > then /bin/ping should lose its file capabilities. This patch probably
> > will need to be cleaned up, but seems to work as it should.
>
> Example is bad -- cp will unlink /bin/ping and put a new inode in it's
> place. dd would convey what happens better :)
You'd think.
But no. Try it.
> > }
> > EXPORT_SYMBOL(inode_setattr);
> >
> > +int file_has_capabilities(struct dentry *dentry);
> > +int remove_file_capabilities(struct dentry *dentry);
> > +
> > int notify_change(struct dentry * dentry, struct iattr * attr)
> > {
> > struct inode *inode = dentry->d_inode;
> > @@ -116,6 +119,15 @@ int notify_change(struct dentry * dentry, struct iattr
> > * attr)
> > attr->ia_atime = now;
> > if (!(ia_valid & ATTR_MTIME_SET))
> > attr->ia_mtime = now;
> > + if (ia_valid & ATTR_KILL_CAP) {
> > + attr->ia_valid &= ~ATTR_KILL_CAP;
> > + ia_valid &= ~ATTR_KILL_CAP;
> > + if (file_has_capabilities(dentry)) {
> > + error = remove_file_capabilities(dentry);
> > + if (error)
> > + return error;
> > + }
> > + }
>
> If the caps can't be removed for whatever reason, then any suid bits
> stick around. Is this something to actually worry about?
suid bits stick around, and we return an error so that IIUC the
operation causing this is cancelled.
> > +int remove_file_capabilities(struct dentry *dentry);
> > int remove_suid(struct dentry *dentry)
> > {
> > int kill = should_remove_suid(dentry);
> > + int ret = 0;
> >
> > if (unlikely(kill))
> > - return __remove_suid(dentry, kill);
> > + ret = __remove_suid(dentry, kill);
> > + if (ret)
> > + return ret;
> >
> > - return 0;
> > + if (should_remove_cap(dentry))
> > + ret = remove_file_capabilities(dentry);
> > +
> > + return ret;
> > }
>
> Again, I'm worried about removing caps but not suid bits. Should I be
> worried?
Well you should definately be worried that *something* subtle in this
patch is likely to be messed up :) But I don't think this is it.
In fact the remaining problem (in both these locations) might be that if
there is an error removing the suid bit or cap xattrs (since they are
done in inverse order), the operation is left partially complete. That
can't be right. I'll look into it.
thanks,
-serge
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
