Re: [RFC PATCH v3 07/13] keys: Add ability to track intended usage of the public key

[Eric Snowberg]

> On Feb 6, 2025, at 1:13 PM, Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxx> wrote:
> 
> On Thu, Oct 17, 2024 at 09:55:10AM -0600, Eric Snowberg wrote:
>> Add two new fields in public_key_signature to track the intended usage of
>> the signature.  Also add a flag for the revocation pass.  During signature
>> validation, two verifications can take place for the same signature.  One
>> to see if it verifies against something on the .blacklist keyring and
>> the other to see if it verifies against the supplied keyring. The flag
>> is used to determine which stage the verification is in.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
> 
> Mimi, was this the patch set you asked to look at while ago?</offtopic>
> 
> This patch adds PKS_REVOCATION_PASS and REVOCATION_SET. It would be
> nice to put in simple terms how they are used in down to eart terms.

Ok, I'll add more details explaining them better in the next round.

>> ---
>> certs/blacklist.c                     |  3 +++
>> crypto/asymmetric_keys/pkcs7_trust.c  | 20 ++++++++++++++++++++
>> crypto/asymmetric_keys/pkcs7_verify.c |  4 ++++
>> include/crypto/pkcs7.h                |  3 +++
>> include/crypto/public_key.h           |  4 ++++
>> 5 files changed, 34 insertions(+)
>> 
>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>> index 675dd7a8f07a..dd34e56a6362 100644
>> --- a/certs/blacklist.c
>> +++ b/certs/blacklist.c
>> @@ -17,6 +17,7 @@
>> #include <linux/uidgid.h>
>> #include <keys/asymmetric-type.h>
>> #include <keys/system_keyring.h>
>> +#include <crypto/public_key.h>
>> #include "blacklist.h"
>> 
>> /*
>> @@ -289,7 +290,9 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>> {
>> int ret;
>> 
>> + pkcs7_set_usage_flag(pkcs7, PKS_REVOCATION_PASS);
>> ret = pkcs7_validate_trust(pkcs7, blacklist_keyring);
>> + pkcs7_clear_usage_flag(pkcs7, PKS_REVOCATION_PASS);
>> 
>> if (ret == 0)
>> return -EKEYREJECTED;
>> diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
>> index 9a87c34ed173..64d70eb68864 100644
>> --- a/crypto/asymmetric_keys/pkcs7_trust.c
>> +++ b/crypto/asymmetric_keys/pkcs7_trust.c
>> @@ -131,6 +131,26 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
>> return 0;
>> }
>> 
>> +void pkcs7_clear_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage)
>> +{
>> + struct pkcs7_signed_info *sinfo;
>> +
>> + for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
>> + if (sinfo->sig)
>> + clear_bit(usage, &sinfo->sig->usage_flags);
>> + }
>> +}
>> +
>> +void pkcs7_set_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage)
>> +{
>> + struct pkcs7_signed_info *sinfo;
>> +
>> + for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
>> + if (sinfo->sig)
>> + set_bit(usage, &sinfo->sig->usage_flags);
>> + }
>> +}
>> +
>> /**
>>  * pkcs7_validate_trust - Validate PKCS#7 trust chain
>>  * @pkcs7: The PKCS#7 certificate to validate
>> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
>> index 1dc80e68ce96..44b8bd0ad4d8 100644
>> --- a/crypto/asymmetric_keys/pkcs7_verify.c
>> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
>> @@ -455,6 +455,10 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
>> return ret;
>> }
>> actual_ret = 0;
>> + if (sinfo->sig) {
>> + sinfo->sig->usage = usage;
>> + set_bit(PKS_USAGE_SET, &sinfo->sig->usage_flags);
>> + }
>> }
>> 
>> kleave(" = %d", actual_ret);
>> diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
>> index 38ec7f5f9041..6c3c9061b118 100644
>> --- a/include/crypto/pkcs7.h
>> +++ b/include/crypto/pkcs7.h
>> @@ -32,6 +32,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
>> extern int pkcs7_validate_trust(struct pkcs7_message *pkcs7,
>> struct key *trust_keyring);
>> 
>> +extern void pkcs7_set_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage);
>> +extern void pkcs7_clear_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage);
>> +
>> /*
>>  * pkcs7_verify.c
>>  */
>> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
>> index b7f308977c84..394022b5d856 100644
>> --- a/include/crypto/public_key.h
>> +++ b/include/crypto/public_key.h
>> @@ -49,6 +49,10 @@ struct public_key_signature {
>> const char *pkey_algo;
>> const char *hash_algo;
>> const char *encoding;
>> + u32 usage; /* Intended usage */
> 
> I'd would not size encode this but instead use "unsigned int".

I'll change that

> 
>> + unsigned long usage_flags;
> 
> It would be less convoluting if this was just "flags". Now this leaves
> to wonder what it is encoded for. E.g. for ioctl API one doees it for
> the API to be size agnostic between 32/64-bit kernels.

and change usage_flags to just "flags"

> 
>> +#define PKS_USAGE_SET 0
>> +#define PKS_REVOCATION_PASS 1
>> };
> 
> I'd add these before the struct and:
> 
> /* ... */
> #define PKS_USAGE_SET 0
> 
> /* ... */
> #define PKS_REVOCATION_PASS 1

I was following the same pattern done elsewhere in the crypto code, 
but I can move them before the structure.

>> 
>> extern void public_key_signature_free(struct public_key_signature *sig);
>> -- 
>> 2.45.0
>> 
> 
> BR, Jarkko

Thanks for your review.