Message-ID: <00B288C0-5011-4706-B393-481910489FFE@oracle.com>
Date:   Fri, 11 Mar 2022 18:44:31 +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 12:02 PM, Stefan Berger <stefanb@...ux.ibm.com> wrote:
> 
> 
> 
> On 3/9/22 13:13, Eric Snowberg wrote:
>>> 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.
> 
> 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.

Powered by blists - more mailing lists