Date:	Fri, 14 Mar 2014 12:51:58 +0000
From:	Matthew Garrett <matthew.garrett@...ula.com>
To:	"gnomes@...rguk.ukuu.org.uk" <gnomes@...rguk.ukuu.org.uk>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"jmorris@...ei.org" <jmorris@...ei.org>,
	"keescook@...omium.org" <keescook@...omium.org>,
	"linux-security-module@...r.kernel.org" 
	<linux-security-module@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"hpa@...or.com" <hpa@...or.com>,
	"jwboyer@...oraproject.org" <jwboyer@...oraproject.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: Re: Trusted kernel patchset for Secure Boot lockdown

On Fri, 2014-03-14 at 12:22 +0000, One Thousand Gnomes wrote:
> > Have you actually looked at these patches? I've looked at every case of
> > RAWIO in the kernel. For cases that are hardware specific and tied to
> > fairly old hardware, I've ignored them. For cases which provide an
> 
> Yes I have - and it's not exactly localised: it modifies stuff all over
> the tree when it shouldn't need to. It's a security policy. If it leaks
> policy all over the kernel then the implementation model is *wrong*.

But you keep talking about MSRs despite there being a patch that limits
access to MSRs. If you have specific examples of privilege escalations
that are possible even with these patches then please, mention them.

> > >default behaviour for capable(x) in fact be
> > >
> > >        capable(x & ~secure_forbidden)
> > >
> > >for a measured kernel and add a 
> > >
> > >        capable_always()
> > >
> > >for the cases you want to not break.
> > 
> > We could do that, but now the behaviour of the patchset is far less
> > obvious. capable(CAP_SYS_RAWIO) now means something different to every
> > other use of capable(), and we still need get_trusted_kernel() calls for
> > cases where the checks have nothing to do with processes and so
> > capabilities can't be used. 
> 
> You don't need get_measured_kernel() calls because thats hidden within
> capable/capable_always. And if the interaction is a complicated as you
> imply that again says to me the model is wrong. Complicated multi-way
> security interactions lead to complicated exploits leveraging the
> disconnects.

How is capable() going to be any use when deciding whether or not to
interpret some kernel command line options?

> > We can do this without unnecessarily breaking any userspace. We just
> > can't do it by fiddling with capabilities.
> 
> It should still be a security model nto spreading measured_kernel()
> checks about. Now if that means
> 
> 	capable(CAP_SYS_RAWIO)
> 
> should become security-> interface hooks that pass something meaningful
> to the security layer then I'd rather we did the job properly in the first
> place and put the policy in one spot not all over the kernel.

We still need a mechanism to differentiate between cases where
CAP_SYS_RAWIO should be forbidden and cases where it should be
permitted, which means that we need to add additional policy. We can
modify capable(), but that means that capable() no longer does what you
expect it to - it's not a transparent interface, and that's likely to
result in people accidentally misusing it.

> The question then becomes what do we need to pass beyond "CAP_SYS_RAWIO"
> so the policy can be in the right place - even for example imposed by
> SELinux rules.

It needs to be possible for this policy to be imposed without userspace
being involved, so using selinux would mean baking it into the kernel
(along with an additional implementation for apparmor, and presumably
one for tomoyo as well).

> 
> > > I don't think we need to break any userspace for "normal" mode to do
> > > this. Userspace in measured mode is going to change anyway. It already
> > > has just for things like module signing.
> > 
> > This has been discussed at length. Nobody who's actually spent time
> > working on the problem wants to use capabilities. CAP_SYS_RAWIO is not
> > semantically identical to the trusted kernel bit. Trying to make them
> > semantically identical will break existing userspace.
> 
> I never said it was. I said that getting rid of CAP_SYS_RAWIO and then
> dealing with *just* the exceptions to this is a lot cleaner, and likely
> to be more secure.

And will give us the choice between complicating a simple interface or
breaking userspace. If the maintainer believes that's a better approach
then I can rewrite all of this again, but so far you seem to be in a
minority on this front. 

> In addition several of the cases that you point out could be fixed by
> replacing the CAP_SYS_RAWIO using stuff with a proper interface should
> probably just be *fixed* to provide that.

...thus breaking userspace outside this use case. I mean, sure, I'm
absolutely fine with deleting /dev/mem entirely. I don't see Linus
agreeing.

-- 
Matthew Garrett <matthew.garrett@...ula.com>

Powered by blists - more mailing lists