Re: Trusted kernel patchset for Secure Boot lockdown

[Kees Cook]

On Fri, Mar 14, 2014 at 5:51 AM, Matthew Garrett
<matthew.garrett@xxxxxxxxxx> wrote:
> 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.

And this ignores the fact that being able to query the measured state
is potentially valuable to additional security policies (e.g. module
loading, firmware loading). Hiding the measured logic in capabilities
makes no sense -- they're orthogonal restrictions.

>>
>> 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?

The command line problem here is a total red herring. If you've got a
measured kernel, you have a measured command line. (If not, you don't
have a measured kernel.) Dealing with the command line has nothing to
do with enforcing the ring0/uid0 boundary which is what this patch
series does.

>> > 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.

Right. As mentioned, we've been over all of this before. We cannot
change the meaning of capabilities (nor expand their coverage to
existing interfaces) without breaking userspace. Since we cannot break
userspace, we must create a different policy system that covers this
new thing we want to do and keeps the new policy distinctly separate.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/