Messages in this thread | | | Date | Mon, 14 Mar 2022 08:00:33 -0400 | Subject | Re: [PATCH 3/4] KEYS: CA link restriction | From | 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@linux.ibm.com> >>> wrote: >>> >>> >>> >>> On 3/9/22 13:13, Eric Snowberg wrote: >>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> >>>>> 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 >>>>> >>>> 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.
| |