Date:   Wed, 9 Mar 2022 18:13:50 +0000
From:   Eric Snowberg <eric.snowberg@...cle.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>
CC:     Mimi Zohar <zohar@...ux.ibm.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        David Howells <dhowells@...hat.com>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jmorris@...ei.org" <jmorris@...ei.org>,
        "serge@...lyn.com" <serge@...lyn.com>,
        "nayna@...ux.ibm.com" <nayna@...ux.ibm.com>,
        "mic@...ux.microsoft.com" <mic@...ux.microsoft.com>,
        Konrad Wilk <konrad.wilk@...cle.com>,
        "keyrings@...r.kernel.org" <keyrings@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH 3/4] KEYS: CA link restriction



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

Powered by blists - more mailing lists