diff mbox series

[3/4] KEYS: CA link restriction

Message ID 20220301173651.3435350-4-eric.snowberg@oracle.com (mailing list archive)
State New, archived
Headers show
Series Add CA enforcement in the machine keyring | expand

Commit Message

Eric Snowberg March 1, 2022, 5:36 p.m. UTC
Add a new link restriction.  Restrict the addition of keys in a keyring
based on the key to be added being a CA.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       | 15 +++++++++++
 2 files changed, 58 insertions(+)

Comments

Stefan Berger March 4, 2022, 3:28 p.m. UTC | #1
On 3/1/22 12:36, Eric Snowberg wrote:
> Add a new link restriction.  Restrict the addition of keys in a keyring
> based on the key to be added being a CA.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>   crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
>   include/crypto/public_key.h       | 15 +++++++++++
>   2 files changed, 58 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 6b1ac5f5896a..49bb2ea7f609 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>   	return ret;
>   }
>   
> +/**
> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @trust_keyring: Unused.
> + *
> + * Check if the new certificate is a CA. If it is a CA, then mark the new
> + * certificate as being ok to link.

CA = root CA here, right?


> + *
> + * Returns 0 if the new certificate was accepted, -ENOKEY if the
> + * certificate is not a CA. -ENOPKG if the signature uses unsupported
> + * crypto, or some other error if there is a matching certificate but
> + * the signature check cannot be performed.
> + */
> +int restrict_link_by_ca(struct key *dest_keyring,
> +			const struct key_type *type,
> +			const union key_payload *payload,
> +			struct key *trust_keyring)
This function needs to correspond to the key_restrict_link_func_t and 
therefore has 4 parameter. Call the unused 'trust_keyring' 'unused' instead?
> +{
> +	const struct public_key_signature *sig;
> +	const struct public_key *pkey;
> +
> +	if (type != &key_type_asymmetric)
> +		return -EOPNOTSUPP;
> +
> +	sig = payload->data[asym_auth];
> +	if (!sig)
> +		return -ENOPKG;
> +
> +	if (!sig->auth_ids[0] && !sig->auth_ids[1])
> +		return -ENOKEY;
> +
> +	pkey = payload->data[asym_crypto];
> +	if (!pkey)
> +		return -ENOPKG;
> +
> +	if (!pkey->key_is_ca)
> +		return -ENOKEY;
> +
> +	return public_key_verify_signature(pkey, sig);
> +}
> +

Comparing this to 'restrict_link_by_signature'... looks good.


>   static bool match_either_id(const struct asymmetric_key_id **pair,
>   			    const struct asymmetric_key_id *single)
>   {
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index 0521241764b7..5eadb182a400 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -72,6 +72,21 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
>   						 const union key_payload *payload,
>   						 struct key *trusted);
>   
> +#if IS_REACHABLE(CONFIG_ASYMMETRIC_KEY_TYPE)
> +extern int restrict_link_by_ca(struct key *dest_keyring,
> +			       const struct key_type *type,
> +			       const union key_payload *payload,
> +			       struct key *trust_keyring);
> +#else
> +static inline int restrict_link_by_ca(struct key *dest_keyring,
> +				      const struct key_type *type,
> +				      const union key_payload *payload,
> +				      struct key *trust_keyring)
> +{
> +	return 0;
> +}
> +#endif
> +
>   extern int query_asymmetric_key(const struct kernel_pkey_params *,
>   				struct kernel_pkey_query *);
>
Eric Snowberg March 7, 2022, 6:06 p.m. UTC | #2
> On Mar 4, 2022, at 8:28 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
> 
> 
> On 3/1/22 12:36, Eric Snowberg wrote:
>> Add a new link restriction.  Restrict the addition of keys in a keyring
>> based on the key to be added being a CA.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>>  crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
>>  include/crypto/public_key.h       | 15 +++++++++++
>>  2 files changed, 58 insertions(+)
>> 
>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>> index 6b1ac5f5896a..49bb2ea7f609 100644
>> --- a/crypto/asymmetric_keys/restrict.c
>> +++ b/crypto/asymmetric_keys/restrict.c
>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>  	return ret;
>>  }
>>  +/**
>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>> + * @dest_keyring: Keyring being linked to.
>> + * @type: The type of key being added.
>> + * @payload: The payload of the new key.
>> + * @trust_keyring: Unused.
>> + *
>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>> + * certificate as being ok to link.
> 
> CA = root CA here, right?

Yes, I’ll update the comment

>> + *
>> + * Returns 0 if the new certificate was accepted, -ENOKEY if the
>> + * certificate is not a CA. -ENOPKG if the signature uses unsupported
>> + * crypto, or some other error if there is a matching certificate but
>> + * the signature check cannot be performed.
>> + */
>> +int restrict_link_by_ca(struct key *dest_keyring,
>> +			const struct key_type *type,
>> +			const union key_payload *payload,
>> +			struct key *trust_keyring)
> This function needs to correspond to the key_restrict_link_func_t and therefore has 4 parameter. Call the unused 'trust_keyring' 'unused' instead?

and I’ll change the name in the next round.
Mimi Zohar March 7, 2022, 11:01 p.m. UTC | #3
On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
> 
> >> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> >> index 6b1ac5f5896a..49bb2ea7f609 100644
> >> --- a/crypto/asymmetric_keys/restrict.c
> >> +++ b/crypto/asymmetric_keys/restrict.c
> >> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
> >>  	return ret;
> >>  }
> >>  +/**
> >> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
> >> + * @dest_keyring: Keyring being linked to.
> >> + * @type: The type of key being added.
> >> + * @payload: The payload of the new key.
> >> + * @trust_keyring: Unused.
> >> + *
> >> + * Check if the new certificate is a CA. If it is a CA, then mark the new
> >> + * certificate as being ok to link.
> > 
> > CA = root CA here, right?
> 
> Yes, I’ll update the comment

Updating the comment is not enough.  There's an existing function named
"x509_check_for_self_signed()" which determines whether the certificate
is self-signed.

thanks,

Mimi
Eric Snowberg March 7, 2022, 11:38 p.m. UTC | #4
> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>> 
>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>> 	return ret;
>>>> }
>>>> +/**
>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>> + * @dest_keyring: Keyring being linked to.
>>>> + * @type: The type of key being added.
>>>> + * @payload: The payload of the new key.
>>>> + * @trust_keyring: Unused.
>>>> + *
>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>> + * certificate as being ok to link.
>>> 
>>> CA = root CA here, right?
>> 
>> Yes, I’ll update the comment
> 
> Updating the comment is not enough.  There's an existing function named
> "x509_check_for_self_signed()" which determines whether the certificate
> is self-signed.

Originally I tried using that function.  However when the restrict link code is called,
all the necessary x509 information is no longer available.   The code in 
restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.  
After verifying the cert has the CA flag set, the call to public_key_verify_signature
validates the cert is self signed.
Stefan Berger March 8, 2022, 2:31 a.m. UTC | #5
On 3/7/22 18:38, Eric Snowberg wrote:
> 
> 
>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>
>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>> 	return ret;
>>>>> }
>>>>> +/**
>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>> + * @dest_keyring: Keyring being linked to.
>>>>> + * @type: The type of key being added.
>>>>> + * @payload: The payload of the new key.
>>>>> + * @trust_keyring: Unused.
>>>>> + *
>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>> + * certificate as being ok to link.
>>>>
>>>> CA = root CA here, right?
>>>
>>> Yes, I’ll update the comment
>>
>> Updating the comment is not enough.  There's an existing function named
>> "x509_check_for_self_signed()" which determines whether the certificate
>> is self-signed.
> 
> Originally I tried using that function.  However when the restrict link code is called,
> all the necessary x509 information is no longer available.   The code in
> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
> After verifying the cert has the CA flag set, the call to public_key_verify_signature
> validates the cert is self signed.
> 
Isn't x509_cert_parse() being called as part of parsing the certificate? 
If so, it seems to check for a self-signed certificate every time. You 
could add something like the following to x509_check_for_self_signed(cert):
pub->x509_self_signed = cert->self_signed = true;

This could then reduce the function in 3/4 to something like:

return payload->data[asym_crypto]->x509_self_signed;


    Stefan
Mimi Zohar March 8, 2022, 12:45 p.m. UTC | #6
On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
> 
> On 3/7/22 18:38, Eric Snowberg wrote:
> > 
> > 
> >> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>
> >> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
> >>>
> >>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> >>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
> >>>>> --- a/crypto/asymmetric_keys/restrict.c
> >>>>> +++ b/crypto/asymmetric_keys/restrict.c
> >>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
> >>>>> 	return ret;
> >>>>> }
> >>>>> +/**
> >>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
> >>>>> + * @dest_keyring: Keyring being linked to.
> >>>>> + * @type: The type of key being added.
> >>>>> + * @payload: The payload of the new key.
> >>>>> + * @trust_keyring: Unused.
> >>>>> + *
> >>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
> >>>>> + * certificate as being ok to link.
> >>>>
> >>>> CA = root CA here, right?
> >>>
> >>> Yes, I’ll update the comment
> >>
> >> Updating the comment is not enough.  There's an existing function named
> >> "x509_check_for_self_signed()" which determines whether the certificate
> >> is self-signed.
> > 
> > Originally I tried using that function.  However when the restrict link code is called,
> > all the necessary x509 information is no longer available.   The code in
> > restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
> > After verifying the cert has the CA flag set, the call to public_key_verify_signature
> > validates the cert is self signed.
> > 
> Isn't x509_cert_parse() being called as part of parsing the certificate? 
> If so, it seems to check for a self-signed certificate every time. You 
> could add something like the following to x509_check_for_self_signed(cert):
> pub->x509_self_signed = cert->self_signed = true;
> 
> This could then reduce the function in 3/4 to something like:
> 
> return payload->data[asym_crypto]->x509_self_signed;

Agreed, as long as the other two criteria are also met: CA and keyUsage
should be required and limited to keyCertSign.

thanks,

Mimi
Stefan Berger March 8, 2022, 1:56 p.m. UTC | #7
On 3/8/22 07:45, Mimi Zohar wrote:
> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>
>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>
>>>
>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>
>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>
>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>> 	return ret;
>>>>>>> }
>>>>>>> +/**
>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>> + * @type: The type of key being added.
>>>>>>> + * @payload: The payload of the new key.
>>>>>>> + * @trust_keyring: Unused.
>>>>>>> + *
>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>> + * certificate as being ok to link.
>>>>>>
>>>>>> CA = root CA here, right?
>>>>>
>>>>> Yes, I’ll update the comment
>>>>
>>>> Updating the comment is not enough.  There's an existing function named
>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>> is self-signed.
>>>
>>> Originally I tried using that function.  However when the restrict link code is called,
>>> all the necessary x509 information is no longer available.   The code in
>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>> validates the cert is self signed.
>>>
>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>> If so, it seems to check for a self-signed certificate every time. You
>> could add something like the following to x509_check_for_self_signed(cert):
>> pub->x509_self_signed = cert->self_signed = true;
>>
>> This could then reduce the function in 3/4 to something like:
>>
>> return payload->data[asym_crypto]->x509_self_signed;
> 
> Agreed, as long as the other two criteria are also met: CA and keyUsage
> should be required and limited to keyCertSign.

right, it's not as easy as the return statement above...

> 
> thanks,
> 
> Mimi
>
Eric Snowberg March 8, 2022, 6:02 p.m. UTC | #8
> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>> 
>> On 3/7/22 18:38, Eric Snowberg wrote:
>>> 
>>> 
>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>> 
>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>> 
>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>> 	return ret;
>>>>>>> }
>>>>>>> +/**
>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>> + * @type: The type of key being added.
>>>>>>> + * @payload: The payload of the new key.
>>>>>>> + * @trust_keyring: Unused.
>>>>>>> + *
>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>> + * certificate as being ok to link.
>>>>>> 
>>>>>> CA = root CA here, right?
>>>>> 
>>>>> Yes, I’ll update the comment
>>>> 
>>>> Updating the comment is not enough.  There's an existing function named
>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>> is self-signed.
>>> 
>>> Originally I tried using that function.  However when the restrict link code is called,
>>> all the necessary x509 information is no longer available.   The code in
>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>> validates the cert is self signed.
>>> 
>> Isn't x509_cert_parse() being called as part of parsing the certificate? 
>> If so, it seems to check for a self-signed certificate every time. You 
>> could add something like the following to x509_check_for_self_signed(cert):
>> pub->x509_self_signed = cert->self_signed = true;
>> 
>> This could then reduce the function in 3/4 to something like:
>> 
>> return payload->data[asym_crypto]->x509_self_signed;

When I was studying the restriction code, before writing this patch, it looked like
it was written from the standpoint to be as generic as possible.  All code contained 
within it works on either a public_key_signature or a public_key.  I had assumed it
was written this way to be used with different asymmetrical key types now and in
the future. I called the public_key_verify_signature function instead of interrogating
the x509 payload to keep in line with what I thought was the original design. Let me
know if I should be carrying x509 code in here to make the change above.

> Agreed, as long as the other two criteria are also met: CA and keyUsage
> should be required and limited to keyCertSign.

I have added the key_is_ca in the public_key header.  I can look at adding the usage
too. Before doing this I would like to understand the "limited to" above.  Many CA keys 
that have keyCertSign set, also have digitalSignature set for key usage.  For 
example:

http://cacerts.digicert.com/DigiCertEVCodeSigningCA-SHA2.crt

Are you saying we would want to exclude a CA like the one above, since it as the 
digitalSignature usage set too?
Stefan Berger March 9, 2022, 5:12 p.m. UTC | #9
On 3/8/22 13:02, Eric Snowberg wrote:
> 
> 
>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>
>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>
>>>>
>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>
>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>
>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>>> 	return ret;
>>>>>>>> }
>>>>>>>> +/**
>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>> + * @type: The type of key being added.
>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>> + *
>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>>> + * certificate as being ok to link.
>>>>>>>
>>>>>>> CA = root CA here, right?
>>>>>>
>>>>>> Yes, I’ll update the comment
>>>>>
>>>>> Updating the comment is not enough.  There's an existing function named
>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>> is self-signed.
>>>>
>>>> Originally I tried using that function.  However when the restrict link code is called,
>>>> all the necessary x509 information is no longer available.   The code in
>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>> validates the cert is self signed.
>>>>
>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>> If so, it seems to check for a self-signed certificate every time. You
>>> could add something like the following to x509_check_for_self_signed(cert):
>>> pub->x509_self_signed = cert->self_signed = true;
>>>
>>> This could then reduce the function in 3/4 to something like:
>>>
>>> return payload->data[asym_crypto]->x509_self_signed;
> 
> When I was studying the restriction code, before writing this patch, it looked like
> it was written from the standpoint to be as generic as possible.  All code contained
> within it works on either a public_key_signature or a public_key.  I had assumed it
> was written this way to be used with different asymmetrical key types now and in
> the future. I called the public_key_verify_signature function instead of interrogating
> the x509 payload to keep in line with what I thought was the original design. Let me
> know if I should be carrying x509 code in here to make the change above.

It does not seem right if there were two functions trying to determine 
whether an x509 cert is self-signed. The existing is invoked as part of 
loading a key onto the machine keyring from what I can see. It has 
access to more data about the cert and therefore can do stronger tests, 
yours doesn't have access to the data. So I guess I would remember in a 
boolean in the public key structure that the x509 cert it comes from was 
self signed following the existing test. Key in your function may be 
that that payload->data[] array is guaranteed to be from the x509 cert 
as set in x509_key_preparse().

https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236

    StefanIt does not seem right if there were two functions trying to 
determine whether an x509 cert is self-signed. The existing is invoked 
as part of loading a key onto the machine keyring from what I can see. 
It has access to more data about the cert and therefore can do stronger 
tests, yours doesn't have access to the data. So I guess I would 
remember in a boolean in the public key structure that the x509 cert it 
comes from was self signed following the existing test. Key in your 
function may be that that payload->data[] array is guaranteed to be from 
the x509 cert as set in x509_key_preparse().

https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236

    Stefan
Stefan Berger March 9, 2022, 5:17 p.m. UTC | #10
On 3/9/22 12:12, Stefan Berger wrote:
> 
> 
> On 3/8/22 13:02, Eric Snowberg wrote:
>>
>>
>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>
>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>
>>>>>
>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>
>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c 
>>>>>>>>> b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key 
>>>>>>>>> *dest_keyring,
>>>>>>>>>     return ret;
>>>>>>>>> }
>>>>>>>>> +/**
>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>> + *
>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then 
>>>>>>>>> mark the new
>>>>>>>>> + * certificate as being ok to link.
>>>>>>>>
>>>>>>>> CA = root CA here, right?
>>>>>>>
>>>>>>> Yes, I’ll update the comment
>>>>>>
>>>>>> Updating the comment is not enough.  There's an existing function 
>>>>>> named
>>>>>> "x509_check_for_self_signed()" which determines whether the 
>>>>>> certificate
>>>>>> is self-signed.
>>>>>
>>>>> Originally I tried using that function.  However when the restrict 
>>>>> link code is called,
>>>>> all the necessary x509 information is no longer available.   The 
>>>>> code in
>>>>> restrict_link_by_ca is basically doing the equivalent to 
>>>>> x509_check_for_self_signed.
>>>>> After verifying the cert has the CA flag set, the call to 
>>>>> public_key_verify_signature
>>>>> validates the cert is self signed.
>>>>>
>>>> Isn't x509_cert_parse() being called as part of parsing the 
>>>> certificate?
>>>> If so, it seems to check for a self-signed certificate every time. You
>>>> could add something like the following to 
>>>> x509_check_for_self_signed(cert):
>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>
>>>> This could then reduce the function in 3/4 to something like:
>>>>
>>>> return payload->data[asym_crypto]->x509_self_signed;
>>
>> When I was studying the restriction code, before writing this patch, 
>> it looked like
>> it was written from the standpoint to be as generic as possible.  All 
>> code contained
>> within it works on either a public_key_signature or a public_key.  I 
>> had assumed it
>> was written this way to be used with different asymmetrical key types 
>> now and in
>> the future. I called the public_key_verify_signature function instead 
>> of interrogating
>> the x509 payload to keep in line with what I thought was the original 
>> design. Let me
>> know if I should be carrying x509 code in here to make the change above.
> 
> It does not seem right if there were two functions trying to determine 
> whether an x509 cert is self-signed. The existing is invoked as part of 
> loading a key onto the machine keyring from what I can see. It has 
> access to more data about the cert and therefore can do stronger tests, 
> yours doesn't have access to the data. So I guess I would remember in a 
> boolean in the public key structure that the x509 cert it comes from was 
> self signed following the existing test. Key in your function may be 
> that that payload->data[] array is guaranteed to be from the x509 cert 
> as set in x509_key_preparse().
> 
> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236 
> 
> 
>     Stefan

Sorry for the mess in the response. The first version is the good one :-)
Mimi Zohar March 9, 2022, 5:33 p.m. UTC | #11
On Tue, 2022-03-08 at 18:02 +0000, Eric Snowberg wrote:

> > On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:

> > Agreed, as long as the other two criteria are also met: CA and keyUsage
> > should be required and limited to keyCertSign.
> 
> I have added the key_is_ca in the public_key header.  I can look at adding the usage
> too. Before doing this I would like to understand the "limited to" above.  Many CA keys 
> that have keyCertSign set, also have digitalSignature set for key usage.  For 
> example:
> 
> http://cacerts.digicert.com/DigiCertEVCodeSigningCA-SHA2.crt
> 
> Are you saying we would want to exclude a CA like the one above, since it as the 
> digitalSignature usage set too?  

Yes, the "machine" keyring is defining a new root of trust to support
allowing end-users the ability "to add their own keys and sign modules
they trust".  There should be a clear distinction between  keys used
for certificate signing from those used for code signing.  Certificate
signing keys should be added to the .machine keyring.  Code signing
keys should be added to the IMA keyring.

thanks,

Mimi
Eric Snowberg March 9, 2022, 6:13 p.m. UTC | #12
> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
> 
> 
> 
> On 3/8/22 13:02, Eric Snowberg wrote:
>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>> 
>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>> 
>>>>> 
>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>> 
>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>> 
>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>>>> 	return ret;
>>>>>>>>> }
>>>>>>>>> +/**
>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>> + *
>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>>>> + * certificate as being ok to link.
>>>>>>>> 
>>>>>>>> CA = root CA here, right?
>>>>>>> 
>>>>>>> Yes, I’ll update the comment
>>>>>> 
>>>>>> Updating the comment is not enough.  There's an existing function named
>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>> is self-signed.
>>>>> 
>>>>> Originally I tried using that function.  However when the restrict link code is called,
>>>>> all the necessary x509 information is no longer available.   The code in
>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>> validates the cert is self signed.
>>>>> 
>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>> If so, it seems to check for a self-signed certificate every time. You
>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>> pub->x509_self_signed = cert->self_signed = true;
>>>> 
>>>> This could then reduce the function in 3/4 to something like:
>>>> 
>>>> return payload->data[asym_crypto]->x509_self_signed;
>> When I was studying the restriction code, before writing this patch, it looked like
>> it was written from the standpoint to be as generic as possible.  All code contained
>> within it works on either a public_key_signature or a public_key.  I had assumed it
>> was written this way to be used with different asymmetrical key types now and in
>> the future. I called the public_key_verify_signature function instead of interrogating
>> the x509 payload to keep in line with what I thought was the original design. Let me
>> know if I should be carrying x509 code in here to make the change above.
> 
> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
> 
> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236

I could add another bool to the public key structure to designate if the key was self signed, 
but this seems to go against what the kernel document states. "Asymmetric / Public-key 
Cryptography Key Type” [1] states:

"The “asymmetric” key type is designed to be a container for the keys used in public-key
cryptography, without imposing any particular restrictions on the form or mechanism of 
the cryptography or form of the key.

The asymmetric key is given a subtype that defines what sort of data is associated with 
the key and provides operations to describe and destroy it. However, no requirement is 
made that the key data actually be stored in the key."

Now every public key type would need to fill in the information on whether the key is self 
signed or not.  Instead of going through the public_key_verify_signature function currently 
used in this patch.

https://docs.kernel.org/crypto/asymmetric-keys.html
Stefan Berger March 9, 2022, 7:02 p.m. UTC | #13
On 3/9/22 13:13, Eric Snowberg wrote:
> 
> 
>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>
>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>
>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>
>>>>>>
>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>
>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>>>>> 	return ret;
>>>>>>>>>> }
>>>>>>>>>> +/**
>>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>>> + *
>>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>>>>> + * certificate as being ok to link.
>>>>>>>>>
>>>>>>>>> CA = root CA here, right?
>>>>>>>>
>>>>>>>> Yes, I’ll update the comment
>>>>>>>
>>>>>>> Updating the comment is not enough.  There's an existing function named
>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>> is self-signed.
>>>>>>
>>>>>> Originally I tried using that function.  However when the restrict link code is called,
>>>>>> all the necessary x509 information is no longer available.   The code in
>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>> validates the cert is self signed.
>>>>>>
>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>
>>>>> This could then reduce the function in 3/4 to something like:
>>>>>
>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>> When I was studying the restriction code, before writing this patch, it looked like
>>> it was written from the standpoint to be as generic as possible.  All code contained
>>> within it works on either a public_key_signature or a public_key.  I had assumed it
>>> was written this way to be used with different asymmetrical key types now and in
>>> the future. I called the public_key_verify_signature function instead of interrogating
>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>> know if I should be carrying x509 code in here to make the change above.
>>
>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>
>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
> 
> I could add another bool to the public key structure to designate if the key was self signed,
> but this seems to go against what the kernel document states. "Asymmetric / Public-key
> Cryptography Key Type” [1] states:
> 
> "The “asymmetric” key type is designed to be a container for the keys used in public-key
> cryptography, without imposing any particular restrictions on the form or mechanism of
> the cryptography or form of the key.
> 
> The asymmetric key is given a subtype that defines what sort of data is associated with
> the key and provides operations to describe and destroy it. However, no requirement is
> made that the key data actually be stored in the key."
> 
> Now every public key type would need to fill in the information on whether the key is self
> signed or not.  Instead of going through the public_key_verify_signature function currently
> used in this patch.

Every public key extracted from a x509 certificate would have to set 
this field to true if the public key originates from a self-signed x509 
cert. Is this different from this code here where now every public key 
would have to set the key_is_ca field?

+		if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+			ctx->cert->pub->key_is_ca = true;

The extension I would have suggested looked similar:

cert->pub->x509_self_sign = cert->self_signed = true

[ to be put here: 
https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 
]



> 
> https://docs.kernel.org/crypto/asymmetric-keys.html
>
Eric Snowberg March 11, 2022, 6:44 p.m. UTC | #14
> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
> 
> 
> 
> On 3/9/22 13:13, Eric Snowberg wrote:
>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>> 
>>> 
>>> 
>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>> 
>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>> 
>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>> 
>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>>>>>> 	return ret;
>>>>>>>>>>> }
>>>>>>>>>>> +/**
>>>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>>>> + *
>>>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>>>>>> + * certificate as being ok to link.
>>>>>>>>>> 
>>>>>>>>>> CA = root CA here, right?
>>>>>>>>> 
>>>>>>>>> Yes, I’ll update the comment
>>>>>>>> 
>>>>>>>> Updating the comment is not enough.  There's an existing function named
>>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>>> is self-signed.
>>>>>>> 
>>>>>>> Originally I tried using that function.  However when the restrict link code is called,
>>>>>>> all the necessary x509 information is no longer available.   The code in
>>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>>> validates the cert is self signed.
>>>>>>> 
>>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>> 
>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>> 
>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>> When I was studying the restriction code, before writing this patch, it looked like
>>>> it was written from the standpoint to be as generic as possible.  All code contained
>>>> within it works on either a public_key_signature or a public_key.  I had assumed it
>>>> was written this way to be used with different asymmetrical key types now and in
>>>> the future. I called the public_key_verify_signature function instead of interrogating
>>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>>> know if I should be carrying x509 code in here to make the change above.
>>> 
>>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>> 
>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>> I could add another bool to the public key structure to designate if the key was self signed,
>> but this seems to go against what the kernel document states. "Asymmetric / Public-key
>> Cryptography Key Type” [1] states:
>> "The “asymmetric” key type is designed to be a container for the keys used in public-key
>> cryptography, without imposing any particular restrictions on the form or mechanism of
>> the cryptography or form of the key.
>> The asymmetric key is given a subtype that defines what sort of data is associated with
>> the key and provides operations to describe and destroy it. However, no requirement is
>> made that the key data actually be stored in the key."
>> Now every public key type would need to fill in the information on whether the key is self
>> signed or not.  Instead of going through the public_key_verify_signature function currently
>> used in this patch.
> 
> Every public key extracted from a x509 certificate would have to set this field to true if the public key originates from a self-signed x509 cert. Is this different from this code here where now every public key would have to set the key_is_ca field?

The information to determine if the key is a CA can not be derived without help from 
the specific key type.  Up to this point, no one has needed it.

> 
> +		if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +			ctx->cert->pub->key_is_ca = true;
> 
> The extension I would have suggested looked similar:
> 
> cert->pub->x509_self_sign = cert->self_signed = true
> 
> [ to be put here: https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 ]

The information to determine if a key is self signed can be derived without help 
from the specific key type.  This can be achieved without modification to a generic
public header file.  Adding a field to the public header would need to either be 
x509 specific or generic for all key types.  Adding a x509 specific field seems to
go against the goal outlined in the kernel documentation.  Adding a generic
self_signed field impacts all key types, now each needs to be modified to fill in 
the new field.
Stefan Berger March 11, 2022, 8:23 p.m. UTC | #15
On 3/11/22 13:44, Eric Snowberg wrote:
> 
> 
>> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 3/9/22 13:13, Eric Snowberg wrote:
>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>>>
>>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>>
>>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>>>
>>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct key *dest_keyring,
>>>>>>>>>>>> 	return ret;
>>>>>>>>>>>> }
>>>>>>>>>>>> +/**
>>>>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
>>>>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>>>>> + *
>>>>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, then mark the new
>>>>>>>>>>>> + * certificate as being ok to link.
>>>>>>>>>>>
>>>>>>>>>>> CA = root CA here, right?
>>>>>>>>>>
>>>>>>>>>> Yes, I’ll update the comment
>>>>>>>>>
>>>>>>>>> Updating the comment is not enough.  There's an existing function named
>>>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>>>> is self-signed.
>>>>>>>>
>>>>>>>> Originally I tried using that function.  However when the restrict link code is called,
>>>>>>>> all the necessary x509 information is no longer available.   The code in
>>>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>>>> validates the cert is self signed.
>>>>>>>>
>>>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>>>
>>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>>>
>>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>>> When I was studying the restriction code, before writing this patch, it looked like
>>>>> it was written from the standpoint to be as generic as possible.  All code contained
>>>>> within it works on either a public_key_signature or a public_key.  I had assumed it
>>>>> was written this way to be used with different asymmetrical key types now and in
>>>>> the future. I called the public_key_verify_signature function instead of interrogating
>>>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>>>> know if I should be carrying x509 code in here to make the change above.
>>>>
>>>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>>>
>>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>>> I could add another bool to the public key structure to designate if the key was self signed,
>>> but this seems to go against what the kernel document states. "Asymmetric / Public-key
>>> Cryptography Key Type” [1] states:
>>> "The “asymmetric” key type is designed to be a container for the keys used in public-key
>>> cryptography, without imposing any particular restrictions on the form or mechanism of
>>> the cryptography or form of the key.
>>> The asymmetric key is given a subtype that defines what sort of data is associated with
>>> the key and provides operations to describe and destroy it. However, no requirement is
>>> made that the key data actually be stored in the key."
>>> Now every public key type would need to fill in the information on whether the key is self
>>> signed or not.  Instead of going through the public_key_verify_signature function currently
>>> used in this patch.
>>
>> Every public key extracted from a x509 certificate would have to set this field to true if the public key originates from a self-signed x509 cert. Is this different from this code here where now every public key would have to set the key_is_ca field?
> 
> The information to determine if the key is a CA can not be derived without help from
> the specific key type.  Up to this point, no one has needed it.
> 
>>
>> +		if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>> +			ctx->cert->pub->key_is_ca = true;
>>
>> The extension I would have suggested looked similar:
>>
>> cert->pub->x509_self_sign = cert->self_signed = true
>>
>> [ to be put here: https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 ]
> 
> The information to determine if a key is self signed can be derived without help
> from the specific key type.  This can be achieved without modification to a generic
> public header file.  Adding a field to the public header would need to either be
> x509 specific or generic for all key types.  Adding a x509 specific field seems to
> go against the goal outlined in the kernel documentation.  Adding a generic
> self_signed field impacts all key types, now each needs to be modified to fill in
> the new field.
> 

If we now called the generic field cert_self_signed we could let it 
indicate whether the certificate the key was extracted from was 
self-self signed. The next question then is how many different types of 
certificates does the key subsystem support besides x509 so we know 
where to set this field if necessary? I don't know of any other...  x509 
seems to be the only type of certificate associated with struct public_key.
What seems to be the case is that pkcs7 also runs the x509 cert parser 
to extract an x509 certificate, thus the flag will be set down this call 
path as well.

https://elixir.bootlin.com/linux/latest/source/crypto/asymmetric_keys/pkcs7_parser.c#L408

Further, the public_key struct is only used in a few places and only in 
the crypto/asymmetric_keys directory filled in. Its usage in pkcs8 seems 
not relevant for certs, so leaving cert_self_signed there uninitialized 
seems just right. The code in public_key.c seems to not deal with 
certificates. So what's left is the x509_cert_parser.c and the function 
x509_cert_parse() allocates it and then calls 
x509_check_for_self_signed(cert), which can set the flag.

It looks to me giving it a generic name and only ever setting it to true 
iin x509_check_for_self_sign(cert) should work.
Stefan Berger March 14, 2022, noon UTC | #16
On 3/11/22 15:23, Stefan Berger wrote:
> 
> 
> On 3/11/22 13:44, Eric Snowberg wrote:
>>
>>
>>> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com> 
>>> wrote:
>>>
>>>
>>>
>>> On 3/9/22 13:13, Eric Snowberg wrote:
>>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> 
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>>>>
>>>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> 
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c 
>>>>>>>>>>>>> b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> @@ -108,6 +108,49 @@ int restrict_link_by_signature(struct 
>>>>>>>>>>>>> key *dest_keyring,
>>>>>>>>>>>>>     return ret;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> +/**
>>>>>>>>>>>>> + * restrict_link_by_ca - Restrict additions to a ring of 
>>>>>>>>>>>>> CA keys
>>>>>>>>>>>>> + * @dest_keyring: Keyring being linked to.
>>>>>>>>>>>>> + * @type: The type of key being added.
>>>>>>>>>>>>> + * @payload: The payload of the new key.
>>>>>>>>>>>>> + * @trust_keyring: Unused.
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * Check if the new certificate is a CA. If it is a CA, 
>>>>>>>>>>>>> then mark the new
>>>>>>>>>>>>> + * certificate as being ok to link.
>>>>>>>>>>>>
>>>>>>>>>>>> CA = root CA here, right?
>>>>>>>>>>>
>>>>>>>>>>> Yes, I’ll update the comment
>>>>>>>>>>
>>>>>>>>>> Updating the comment is not enough.  There's an existing 
>>>>>>>>>> function named
>>>>>>>>>> "x509_check_for_self_signed()" which determines whether the 
>>>>>>>>>> certificate
>>>>>>>>>> is self-signed.
>>>>>>>>>
>>>>>>>>> Originally I tried using that function.  However when the 
>>>>>>>>> restrict link code is called,
>>>>>>>>> all the necessary x509 information is no longer available.   
>>>>>>>>> The code in
>>>>>>>>> restrict_link_by_ca is basically doing the equivalent to 
>>>>>>>>> x509_check_for_self_signed.
>>>>>>>>> After verifying the cert has the CA flag set, the call to 
>>>>>>>>> public_key_verify_signature
>>>>>>>>> validates the cert is self signed.
>>>>>>>>>
>>>>>>>> Isn't x509_cert_parse() being called as part of parsing the 
>>>>>>>> certificate?
>>>>>>>> If so, it seems to check for a self-signed certificate every 
>>>>>>>> time. You
>>>>>>>> could add something like the following to 
>>>>>>>> x509_check_for_self_signed(cert):
>>>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>>>>
>>>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>>>>
>>>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>>>> When I was studying the restriction code, before writing this 
>>>>>> patch, it looked like
>>>>>> it was written from the standpoint to be as generic as possible.  
>>>>>> All code contained
>>>>>> within it works on either a public_key_signature or a public_key.  
>>>>>> I had assumed it
>>>>>> was written this way to be used with different asymmetrical key 
>>>>>> types now and in
>>>>>> the future. I called the public_key_verify_signature function 
>>>>>> instead of interrogating
>>>>>> the x509 payload to keep in line with what I thought was the 
>>>>>> original design. Let me
>>>>>> know if I should be carrying x509 code in here to make the change 
>>>>>> above.
>>>>>
>>>>> It does not seem right if there were two functions trying to 
>>>>> determine whether an x509 cert is self-signed. The existing is 
>>>>> invoked as part of loading a key onto the machine keyring from what 
>>>>> I can see. It has access to more data about the cert and therefore 
>>>>> can do stronger tests, yours doesn't have access to the data. So I 
>>>>> guess I would remember in a boolean in the public key structure 
>>>>> that the x509 cert it comes from was self signed following the 
>>>>> existing test. Key in your function may be that that 
>>>>> payload->data[] array is guaranteed to be from the x509 cert as set 
>>>>> in x509_key_preparse().
>>>>>
>>>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236 
>>>>>
>>>> I could add another bool to the public key structure to designate if 
>>>> the key was self signed,
>>>> but this seems to go against what the kernel document states. 
>>>> "Asymmetric / Public-key
>>>> Cryptography Key Type” [1] states:
>>>> "The “asymmetric” key type is designed to be a container for the 
>>>> keys used in public-key
>>>> cryptography, without imposing any particular restrictions on the 
>>>> form or mechanism of
>>>> the cryptography or form of the key.
>>>> The asymmetric key is given a subtype that defines what sort of data 
>>>> is associated with
>>>> the key and provides operations to describe and destroy it. However, 
>>>> no requirement is
>>>> made that the key data actually be stored in the key."
>>>> Now every public key type would need to fill in the information on 
>>>> whether the key is self
>>>> signed or not.  Instead of going through the 
>>>> public_key_verify_signature function currently
>>>> used in this patch.
>>>
>>> Every public key extracted from a x509 certificate would have to set 
>>> this field to true if the public key originates from a self-signed 
>>> x509 cert. Is this different from this code here where now every 
>>> public key would have to set the key_is_ca field?
>>
>> The information to determine if the key is a CA can not be derived 
>> without help from
>> the specific key type.  Up to this point, no one has needed it.
>>
>>>
>>> +        if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>>> +            ctx->cert->pub->key_is_ca = true;
>>>
>>> The extension I would have suggested looked similar:
>>>
>>> cert->pub->x509_self_sign = cert->self_signed = true
>>>
>>> [ to be put here: 
>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 
>>> ]
>>
>> The information to determine if a key is self signed can be derived 
>> without help
>> from the specific key type.  This can be achieved without modification 
>> to a generic
>> public header file.  Adding a field to the public header would need to 
>> either be
>> x509 specific or generic for all key types.  Adding a x509 specific 
>> field seems to
>> go against the goal outlined in the kernel documentation.  Adding a 
>> generic
>> self_signed field impacts all key types, now each needs to be modified 
>> to fill in
>> the new field.
>>
> 
> If we now called the generic field cert_self_signed we could let it 
> indicate whether the certificate the key was extracted from was 
> self-self signed. The next question then is how many different types of 
> certificates does the key subsystem support besides x509 so we know 
> where to set this field if necessary? I don't know of any other...  x509 
> seems to be the only type of certificate associated with struct public_key.
> What seems to be the case is that pkcs7 also runs the x509 cert parser 
> to extract an x509 certificate, thus the flag will be set down this call 
> path as well.
> 
> https://elixir.bootlin.com/linux/latest/source/crypto/asymmetric_keys/pkcs7_parser.c#L408 
> 
> 
> Further, the public_key struct is only used in a few places and only in 
> the crypto/asymmetric_keys directory filled in. Its usage in pkcs8 seems 
> not relevant for certs, so leaving cert_self_signed there uninitialized 
> seems just right. The code in public_key.c seems to not deal with 
> certificates. So what's left is the x509_cert_parser.c and the function 
> x509_cert_parse() allocates it and then calls 
> x509_check_for_self_signed(cert), which can set the flag.
> 
> It looks to me giving it a generic name and only ever setting it to true 
> iin x509_check_for_self_sign(cert) should work.

Otherwise maybe we could introduce

struct cert_info {
         bool is_ca;
         bool self_sign;
}

And use it like this:
+		if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+			ctx->cert->cert_info->is_ca = true;

New index in the data array:

	prep->payload.data[asym_subtype] = &public_key_subtype;
	prep->payload.data[asym_key_ids] = kids;
	prep->payload.data[asym_crypto] = cert->pub;
	prep->payload.data[asym_auth] = cert->sig;
	prep->payload.data[asym_cert_info] = cert->cert_info;

There are a few more places where this new array index would need to be 
set to NULL.
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..49bb2ea7f609 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,49 @@  int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+/**
+ * restrict_link_by_ca - Restrict additions to a ring of CA keys
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @trust_keyring: Unused.
+ *
+ * Check if the new certificate is a CA. If it is a CA, then mark the new
+ * certificate as being ok to link.
+ *
+ * Returns 0 if the new certificate was accepted, -ENOKEY if the
+ * certificate is not a CA. -ENOPKG if the signature uses unsupported
+ * crypto, or some other error if there is a matching certificate but
+ * the signature check cannot be performed.
+ */
+int restrict_link_by_ca(struct key *dest_keyring,
+			const struct key_type *type,
+			const union key_payload *payload,
+			struct key *trust_keyring)
+{
+	const struct public_key_signature *sig;
+	const struct public_key *pkey;
+
+	if (type != &key_type_asymmetric)
+		return -EOPNOTSUPP;
+
+	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
+
+	if (!sig->auth_ids[0] && !sig->auth_ids[1])
+		return -ENOKEY;
+
+	pkey = payload->data[asym_crypto];
+	if (!pkey)
+		return -ENOPKG;
+
+	if (!pkey->key_is_ca)
+		return -ENOKEY;
+
+	return public_key_verify_signature(pkey, sig);
+}
+
 static bool match_either_id(const struct asymmetric_key_id **pair,
 			    const struct asymmetric_key_id *single)
 {
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 0521241764b7..5eadb182a400 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -72,6 +72,21 @@  extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_KEY_TYPE)
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+#else
+static inline int restrict_link_by_ca(struct key *dest_keyring,
+				      const struct key_type *type,
+				      const union key_payload *payload,
+				      struct key *trust_keyring)
+{
+	return 0;
+}
+#endif
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);