Messages in this thread | | | Date | Wed, 9 Mar 2022 12:17:06 -0500 | Subject | Re: [PATCH 3/4] KEYS: CA link restriction | From | Stefan Berger <> |
| |
On 3/9/22 12:12, Stefan Berger 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 > > > Stefan
Sorry for the mess in the response. The first version is the good one :-)
| |