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

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] [day] [month] [year] [list]
Message-ID: <C32A61D2-4C87-482E-B7CE-37C5C5780D89@oracle.com>
Date: Fri, 7 Feb 2025 23:09:30 +0000
From: Eric Snowberg <eric.snowberg@...cle.com>
To: Jarkko Sakkinen <jarkko.sakkinen@...nel.org>
CC: "open list:SECURITY SUBSYSTEM" <linux-security-module@...r.kernel.org>,
        David Howells <dhowells@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Ard Biesheuvel
	<ardb@...nel.org>, Paul Moore <paul@...l-moore.com>,
        James Morris
	<jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Mimi Zohar
	<zohar@...ux.ibm.com>,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Dmitry
 Kasatkin <dmitry.kasatkin@...il.com>,
        Mickaël Salaün
	<mic@...ikod.net>,
        "casey@...aufler-ca.com" <casey@...aufler-ca.com>,
        Stefan
 Berger <stefanb@...ux.ibm.com>,
        "ebiggers@...nel.org" <ebiggers@...nel.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        open list
	<linux-kernel@...r.kernel.org>,
        "keyrings@...r.kernel.org"
	<keyrings@...r.kernel.org>,
        "linux-crypto@...r.kernel.org"
	<linux-crypto@...r.kernel.org>,
        "linux-efi@...r.kernel.org"
	<linux-efi@...r.kernel.org>,
        "linux-integrity@...r.kernel.org"
	<linux-integrity@...r.kernel.org>
Subject: Re: [RFC PATCH v3 07/13] keys: Add ability to track intended usage of
 the public key



> On Feb 6, 2025, at 1:13 PM, Jarkko Sakkinen <jarkko.sakkinen@...nel.org> 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@...cle.com>
> 
> 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.