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