linux-kernel - Re: [PATCH 3/4] KEYS: CA link restriction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Mar 2022 18:02:26 +0000
From:   Eric Snowberg <eric.snowberg@...cle.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>,
        Stefan Berger <stefanb@...ux.ibm.com>
CC:     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 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.

> Agreed, as long as the other two criteria are also met: CA and keyUsage
> should be required and limited to keyCertSign.

I have added the key_is_ca in the public_key header.  I can look at adding the usage
too. Before doing this I would like to understand the "limited to" above.  Many CA keys 
that have keyCertSign set, also have digitalSignature set for key usage.  For 
example:

http://cacerts.digicert.com/DigiCertEVCodeSigningCA-SHA2.crt

Are you saying we would want to exclude a CA like the one above, since it as the 
digitalSignature usage set too?