[PATCH 3/4] KEYS: CA link restriction

[Stefan Berger]

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 at linux.ibm.com> 
>>> wrote:
>>>
>>>
>>>
>>> On 3/9/22 13:13, Eric Snowberg wrote:
>>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb at linux.ibm.com> 
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar at 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 at 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.