Re: [PATCH] certs: Add the ability to add only CA certificates to the secondary trusted keyring

[Sergey Shtylyov]

On 9/6/23 2:32 PM, Denis Glazkov wrote:

> When building a chain of trust for IMA certificates issued from

   It's a shame I forgot what IMA stands for... and even Google doesn't
give any suitable value...

> intermediate certificates using a secondary trusted keying, there
> is no way to restrict the addition of IMA certificates to trusted
> certificates, since any certificate signed by an built-in or
> secondary trusted certificate can be added to the secondary
> trusted keying.
> 
> With root privileges, an attacker can load a certificate intended
> for IMA into the trusted certificates and sign the kernel modules
> with the corresponding private key. This allows an attacker to
> load untrusted modules into kernel space.
> 
> This patch adds the configuration that once enabled, only
> certificates that meet the following requirements can be added
> to the secondary trusted keying:
> 
> 1. The certificate is a CA.

   Oh, and I also forgot what CA stands for... :-/

> 2. The certificate must be used for verifying a CA's signatures.
> 3. The certificate must not be used for digital signatures.
> 
> Signed-off-by: Denis Glazkov <d.glazkov@xxxxxx>
[...]

> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..8d45c19ba92e 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -90,6 +90,10 @@ int restrict_link_by_builtin_and_secondary_trusted(
>  	const union key_payload *payload,
>  	struct key *restrict_key)
>  {
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING_FOR_CA_CERTIFICATES_ONLY
> +	struct public_key *pub;
> +#endif

   Mhm, why this is not a part of the *if* block in the next hunk?
You don't use this variable outside that block...

[...]
> @@ -99,6 +103,23 @@ int restrict_link_by_builtin_and_secondary_trusted(
>  		/* Allow the builtin keyring to be added to the secondary */
>  		return 0;
>  
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING_FOR_CA_CERTIFICATES_ONLY
> +	if (dest_keyring == secondary_trusted_keys) {
> +		if (type != &key_type_asymmetric)
> +			return -EOPNOTSUPP;
> +
> +		pub = payload->data[asym_crypto];

   I'm not seeing this index declared in Linus' repo...

> +		if (!pub)
> +			return -ENOPKG;
> +		if (!test_bit(KEY_EFLAG_CA, &pub->key_eflags))
> +			return -EPERM;
> +		if (!test_bit(KEY_EFLAG_KEYCERTSIGN, &pub->key_eflags))
> +			return -EPERM;
> +		if (test_bit(KEY_EFLAG_DIGITALSIG, &pub->key_eflags))
> +			return -EPERM;
> +	}
> +#endif
> +
>  	return restrict_link_by_signature(dest_keyring, type, payload,
>  					  secondary_trusted_keys);
>  }

MBR, Sergey