On 3/11/22 15:23, Stefan Berger wrote:

On 3/11/22 13:44, Eric Snowberg wrote:

quoted

quoted

On Mar 9, 2022, at 12:02 PM, Stefan Berger [off-list ref] 
wrote:



On 3/9/22 13:13, Eric Snowberg wrote:

quoted

quoted

On Mar 9, 2022, at 10:12 AM, Stefan Berger [off-list ref] 
wrote:



On 3/8/22 13:02, Eric Snowberg wrote:

quoted

quoted

On Mar 8, 2022, at 5:45 AM, Mimi Zohar [off-list ref] wrote:

On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:

quoted

On 3/7/22 18:38, Eric Snowberg wrote:

quoted

quoted

On Mar 7, 2022, at 4:01 PM, Mimi Zohar [off-list ref] 
wrote:

On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:

quoted

quoted

quoted

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.

quoted

+        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.