Re: [PATCH] X.509: Partially revert patch to add validation against IMA MOK keyring

[Petko Manolov]

On 16-01-12 01:38:13, David Howells wrote:
> Petko Manolov <petkan@xxxxxxxxxxxx> wrote:
> 
> > > I would like to counter Mimi's NAK:
> > > 
> > >  (1) Commit 41c89b64d7184a780f12f2cccdabe65cb2408893 doesn't do what it
> > >      says.  Given the change I want to revert, this bit of the description:
> > > 
> > > 	To successfully import a key into .ima_mok it must be signed by a
> > > 	key which CA is in .system keyring.
> > > 
> > >      is *not* true.  A key in the .ima_mok keyring will *also* allow a key
> > 
> > This is correct, but is also the desired result.
> 
> Not really.  What if you want a trusted keyring that is not governed by the 
> .ima_mok keyring?  You can't have it.

There is no need for .ima_mok if here is .mok, which should be system wide 
keyring.  I'm trying to say that once we have .mok (you're more than welcome to 
suggest better name) we'll get rid of .ima_mok.

> > Assume that you have multi tenant machine where the manufacturer signs the
> > owner's/tenant's key and those also need to sign other sub-tenant keys.  One
> > can't put them on the system keyring
> 
> Why not?  We can enable user-write on the system keyring.  Are you saying that 
> there should be multiple .ima_mok keyrings?  If so, your change is wrong.

Nope, no multiple MOK keyrings.  One, read-write trusted keyring should be 
enough.

> > so they end up in .ima_mok.
> > 
> > >      into the .ima_mok keyring.  Thus the .ima_mok keyring is redundant and
> > >      should be merged into the .system keyring.
> > 
> > I share Mimi's opinion that .system keyring must be static and ultimately 
> > trusted.
> 
> I don't necessarily share the opinion that it must remain static.  If you can 
> change the .ima_mok keyring, then it's as good as being able to change the 
> .system keyring by your change that I want to revert.

I still see value in immutable system keyring.  Being able to reboot to a known 
state is only one of the reasons.  The other is the ultimate trust one should 
have in .system...

> The one thing I grant that enabling the .system keyring will allow is deletion 
> of trusted keys - and once you've deleted them, you can't necessarily get them 
> back without rebooting.

Can't we incorporate this functionality in .blacklist and avoid rebooting.

> > Since .ima_mok is a dynamic keyring, merging them will break the semantics.
> 
> So what you're saying is that .ima_mok is just a staging area for changes that 
> would otherwise go in .system?  In which case, why is it IMA-related at all?  
> Why isn't it called ".mok" or ".system_overrides" or something like that and 
> in certs/system_keyring.c?

.ima_mok was designed at a time when i did not see it as a system-level trusted 
keyring.  It later occurred that it should be moved out of the IMA subsystem as 
there are potentially other users.

> Remember: IMA is optional.  We want to be able to disable it.  So if you want 
> this feature, it really needs to be separated from IMA.

Moving it out, or being enabled if IMA (or other sybsystem) is selected, will do 
the trick

> And why shouldn't we change these semantics?
> 
> And why can't .system be a dynamic keyring?

Because this makes me uneasy.  What are we saving?  A few pages of memory?..

> > >  (2) You can use KEYCTL_LINK to link trusted keys between trusted keyrings
> > >      if the key being linked grants permission.  Add a new key to one open
> > >      keyring and you can then link it across to another.
> > > 
> > >      Keyrings need to guard against *link* as per my recently posted
> > >      patches.
> > 
> > I'd rather rely on a certificate being properly signed in order to land in a 
> > particular keyring, rather than being linked based on permissions model.
> 
> Then the patches I posted *are* necessary.  Currently KEYCTL_LINK will let you 
> link a "trusted" key between trusted-only keyrings if the Link permission is 
> set because trustedness is currently evaluated only once - at key preparse 
> time - because we currently throw away all the metadata you need to do further 
> checks.

I don't mind linking in general as long as the permission check is supplementary 
to the keys CA hierarchy verification.

> The patches I posted validate the certificate signature on addition of a 
> *link* into a keyring (add_key(), if it doesn't update a key, creates a key 
> and then does a link operation).  The gatekeeper function is settable 
> per-keyring.

Splendid.  I've no issue with this.

> > >  (3) In the current model, the trusted-only keyring and trusted-key concept
> > >      ought really to apply only to the .system keyring as the concept of
> > >      'trust' is boolean in this implementation.
> > 
> > The .system keyring should be read-only, IOW static.  Only keys present at
> > build time should go there.
> 
> Why?  You haven't given a reason why .ima_mok shouldn't be integrated into 
> .system and that opened up.  Because you prefer it the way you've done it 
> isn't necessarily a good reason.

Err...  I've stated my motive numerous times.  Please let me know if i miss the 
point somewhere.

> > Everything else goes to the machine owner keyring (MOK) or whatever the
> > name.  MOK should be read-write and (maybe) hold only second level CAs,
> > signed by CA in the system keyring.  I introduced .ima_mok just because my
> > work had limited scope at the time and i consider the name as misleading.
> 
> What would you call it then, if not .ima_mok?  Given its current name, it 
> seems that it should relate to the UEFI BIOS data if such is available.

Nope, the name is indeed misleading.  I don't insist on MOK in the name as long 
as the semantics is preserved.  I also do not want to involve UEFI-embedded 
certificates.  Just because they are in my machine's NV memory does not mean i 
trust them.  I'd rather trust the kernel and certificates that i've made.

There are companies that make their own hardware and build all of their 
software.  UEFI may or may not be present.  Again, think of big machines.

> > The way i see kernel's keyrings:
> > 
> >                            /---> .ima
> >             /----> MOK ---<
> > .system ---<               \---> .evm
> >             \----> BL       \---> .whatever need to be "trusted"
> > 
> > The graph could be a lot more complex, but to wrap your head around the idea
> > think of big ass machine with years of uptime and multiple simultaneous
> > users, all pre-installed files IMA signed, ability to add other IMA signed
> > packages on the fly.  The machine must be FIPS certifiable, etc.  A terabit
> > switching machine should be able to do that and there are real users for
> > this scenario out there.
> 
> Given you seem to have one .system ring and one MOK ring, why do you need
> separate rings?

If .system is RO, then we need something that is both second level CA hierarchy 
and is RW.

> > The black-list keyring is equally important so one can revoke CAs if need
> > be.  On the fly.
> 
> I've no objection to a blacklist.  I can see that you might want it to be a 
> separate keyring to have the no-removal clause in place.  I would consider 
> making a blacklist key type and have it contain a bundle of key IDs to reduce 
> resource consumption, but that's an implementation detail (the match function 
> can check against all the IDs in a bundle).

I think blacklist keyring is important.  Being write-only is also important.  
Once this situation is resolved i'll have to post a patch that makes use of 
.blacklist as we already have users for this scenario.

> However, the blacklist also should be separated from the IMA subsystem and 
> moved to system_keyring.c.  I can probably backport the code we use in Fedora 
> for this.

I totally agree with you here.  If the certificate used to sign certain kernel 
modules is revoked we would very much like to forbid loading those modules.


		Petko