lkml.org 
[lkml]   [2022]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/4] KEYS: CA link restriction
Date


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

\
 
 \ /
  Last update: 2022-03-11 19:45    [W:2.096 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site