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

[Mimi Zohar]

On Tue, 2016-01-12 at 10:08 +0000, David Howells wrote:
> Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > The IMA MOK and blacklist are restricted to "public_key_restrict_link".
> > Does this only allow keys signed by keys on the respective keyring or
> > also by the system keyring? 
> 
> As my patches stand, the following are implemented:
> 
>  (1) public_key_restrict_link() restricts to asymmetric keys that are signed
>      by a CA in the specified keyring.  It returns -ENOKEY if no matching key
>      is found rather than -EKEYREJECTED, however, so you can call it several
>      times for different keyrings.  -EKEYREJECTED is only returned if a
>      signature check fails.  This is used by the following two functions.

Ok, so it is restricted it to CAs.  Combining it with an option to limit
it to the builtin CA keys based on the builtin flag would be nice.
 
>  (2) restrict_link_by_system_trusted() restricts to asymmetric keys that are
>      signed by a CA in the system keyring.  This ignores the keyring argument
>      it is given.

>      Note that the system_trusted_keyring is then no longer exported because
>      verify_pkcs7_signature() is also in certs/system_keyring.c and uses that
>      by default if NULL is passed.

Ok

>  (3) restrict_link_by_ima_mok() restricts to asymmetric keys signed by a CA in
>      either .system_keyring or .ima_mok.

> So the trusted keyrings are then restricted as follows:
> 
>  (1) .system_keyring uses restrict_link_by_system_trusted() - though it lacks
>      any sort of write permission, so it's currently moot.  It could just as
>      well be replaced with a function that just returns -EPERM.

Why not retain the current semantics of the system keyring of not being
writable and create a new keyring for new feature(s)?

>  (2) .ima_mok should be using restrict_link_by_system_trusted(), but I failed
>      to update this when I split the public_key_restrict_link() function.
>      I've updated this in my patch.  This would then be correct according to
>      Petko's commit log:
> 
> 	To successfully import a key into .ima_mok it must be signed by a
> 	key which CA is in .system keyring.
> 
>      However, from what Petko says, this is wrong and it should instead be
>      using restrict_link_by_ima_mok().

The name "restrict_link_by_ima_mok()" doesn't reflect that it is either
the system keyring or the IMA MOK keyring.

>  (3) .ima_blacklist should be using restrict_link_by_system_trusted() also.
>      I've no idea whether additions to this should be permitted by keys in
>      .ima_mok also.

Petko/Mark, please correct me if I'm wrong.  It would be the same as the
IMA MOK keyring.

>  (4) .ima uses restrict_link_by_ima_mok(), as per:
> 
> 	On turn any key that needs to go in .ima keyring must be signed by CA
> 	in either .system or .ima_mok keyrings.

Currently, if IMA MOK is not enabled, then the keyring isn't created and
shouldn't be referenced.  The default should be
restrict_link_by_system_trusted.

>  (5) .evm is not restricted by my patches.  This is a mistake on my part - but
>      I'm not sure what the restriction actually needs to be as it's not
>      mentioned in Petko's commit message.  Presumably it needs the same as
>      .ima.

Currently, security.evm xattrs are not portable across systems.  On
first access, security.evm xattrs containing file signatures are
converted to an HMAC.   This probably will change in the future, but for
now .evm should be restrict_link_by_system_trusted as well.

David, thank you for the detailed explanation.

Mimi