Re: [patch] file capabilities: Add no_file_caps switch

[Serge E. Hallyn]

Quoting Andreas Gruenbacher (agruen@xxxxxxx):
> On Wednesday, 27 August 2008 18:04:39 Serge E. Hallyn wrote:
> > Quoting Andreas Gruenbacher (agruen@xxxxxxx):
> > > On Wednesday, 27 August 2008 15:52:06 Serge E. Hallyn wrote:
> > > > Quoting Andreas Gruenbacher (agruen@xxxxxxx):
> > > > > Hello,
> > > > >
> > > > > here is a patch allowing to disable file capabilities via a kernel
> > > > > command line option (once compiled in with
> > > > > CONFIG_SECURITY_FILE_CAPABILITIES).
> > > > >
> > > > > We would like to ship our next round of products with file
> > > > > capabilities compiled in, yet we feel that too many system utilities
> > > > > are still file capabilitiy unaware, and so we would like to turn them
> > > > > off by default initially. File capabilities can be used to grant
> > > > > privileges to binaries which otherwise look "harmless", which is a
> > > > > security risk until utilities like rpm have learned how to install
> > > > > and verify capabilities, etc.
> > > > >
> > > > > Any objections?
> > > >
> > > > Hi Andreas,
> > > >
> > > > No objections in general - if it makes you more comfortable shipping
> > > > kernels with CONFIG_SECURITY_FILE_CAPABILITIES=y then it's worthwhile.
> > > > However, can you elaborate on your concerns?
> > >
> > > We don't have the time left for developing the few missing pieces and
> > > properly integrating file capabilities into our products (use in various
> > > packages, support in rpm, system management, manuals, release notes), and
> > > so I would like to have a way to turn them off by default for now.
> > >
> > > > In particular, if as you say above the concern is really just that a
> > > > file might have capabilities accidentally (or maliciously) enabled,
> > > > then we should be able to just check for file_caps_enabled() at
> > > > get_file_caps(), refusing to fill in the file capabilities.
> > >
> > > My main concern is accidental granting of capabilities because of admin
> > > unawareness / lack of tool support. This could be taken care of by not
> > > loading the capabilities from disk.
> > >
> > > > The other changes which you are canceling out confuscate the code but
> > > > actually make no difference.
> > >
> > > Well, the other difference is that with
> > > CONFIG_SECURITY_FILE_CAPABILITIES=y you currently lose the ability to
> > > pass on capabilities to other processes. Do you have good arguments why
> > > this feature is unnecessary?
> >
> > Yes, mainly that you don't actually have that ability anyway, because
> > when CONFIG_SECURITY_FILE_CAPABILITIES=n, then CAP_SETPCAP is not in the
> > system-wide capability bounding set, and without CAP_SETPCAP you cannot
> > pass capabilities to another process.
> >
> > You can do it if you have a custom initrd that adds CAP_SETPCAP to the
> > bounding set early enough, but it has to be done by pid=1.  As far as I
> > have seen there are 0 users of the feature.
> 
> Alright, this should suffice and we won't have to care about this case then.
> 
> What remains is a way to disable the loading of capabilities from the kernel 
> command line, but this is a rather trivial patch. Would you like to write 
> that? Shall i send a patch?

The below patch seems to be working for me.  Please take a look.

> > If, however, you really do have such users, then we must go with a
> > version of your patch.  We may then want to consider altogether
> > replacing the CONFIG_SECURITY_FILE_CAPABILITIES boolean with a default
> > value for file_caps_enabled.  That may actually end up cleaner than
> > the current code by removing all of the #ifdefs.
> 
> Most ifdefs would go away by adding a file_caps_enabled variable / #define in 
> capability.h and using that. I would suggest to make this on-by-default as 
> the common case will eventually be on, and that way, we won't have to carry 
> the kernel command-line option code forever.
> 
> > (Also note that if you have such users, you'll want to ask David
> > Howells not to push the patch he has floated removing the ability to
> > pass caps to another task altogether when
> > CONFIG_SECURITY_FILE_CAPABILITIES=n :)
> 
> I was actually about to ask for making this behavior change pertinent instead 
> of having it depend on CONFIG_SECURITY_FILE_CAPABILITIES :)

Ok, cool.  In that case - David I was a little surprised to find that
patch not applied in Linus' tree.  You sent it quite some time ago,
didn't you?  Were you planning on trying again to push it soon?

In any case, how about the following patch?

If it is ok with everyone, then I'll rewrite the intro and send it
out separately.

thanks,
-serge