lkml.org 
[lkml]   [2022]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 3/4] KEYS: CA link restriction
From


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.

\
 
 \ /
  Last update: 2022-03-14 14:35    [W:0.085 / U:3.528 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site