[PATCH v3 0/5] pkcs7: better handling of signed attributes

[PATCH v3 0/5] pkcs7: better handling of signed attributes

[James Bottomley]

v3 updates for the now-upstream ml-dsa.  The ml-dsa patches actually
changed the definition of authattrs in struct pkcs7_signed_info, which
means I now don't have to justify stepping one back in patch 4
(thanks!).  Just in case I checked that nothing else was affected by
this change.

Original cover letter:

Although the biggest use of signed attributes is PKCS#7 and X509
specific data, they can be added to a signature to support arbitrary
and verifiable objects.  This makes them particularly useful when you
want to take an existing signature scheme and extend it with
additional (but always verified) data in such a way that it still
looks valid to both the old and new schemes.

To use a scheme like this to extend signatures requires that the
authenticated attribute only be pulled out of a signer info that can
be verified by one of the trusted keys, so the implementation loops
over all signer infos, discarding those that haven't been verified and
returns the first OID match it finds in the verified ones.  Note that
if you reparse a pkcs7 it starts out with no trusted signer infos, and
you must anchor trust by calling validate_pkcs7_trust() with the
trusted keyring.

The first three patches in this series are new to v2.  They add the
new validate_pkcs7_trust() call, thread a verified flag through struct
pkcs7_signer_info so we can tell which signers have been validated
against the trusted keyring. And finally thread pkcs7_digest through
the pkcs7_validate functions so they can operate on a plain parsed
pkcs7 structure that hasn't gone through pkcs7_verify.  Note we could
simply drop the last patch and insist that the pkcs7 be re-verified;
it just looked a bit inefficient, especially as the default way of
doing this (verify_pkcs7_signature() frees the pkcs7 structure before
returning.

The final two patches search for the authenticated attribute by OID,
stopping at the first one it finds belonging to a verified signer
info.  The final patch demonstrates how to use it.  I've added a check
to show that if you don't in any way validate the pkcs7 then no signed
attributes get returned.

Regards,

James

---

James Bottomley (5):
  certs: break out pkcs7 check into its own function
  crypto: pkcs7: add flag for validated trust on a signed info block
  crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust
  crypto: pkcs7: add ability to extract signed attributes by OID
  crypto: pkcs7: add tests for pkcs7_get_authattr

 certs/system_keyring.c                  | 76 +++++++++++++----------
 crypto/asymmetric_keys/Makefile         |  4 +-
 crypto/asymmetric_keys/pkcs7_aa.asn1    | 18 ++++++
 crypto/asymmetric_keys/pkcs7_key_type.c | 42 ++++++++++++-
 crypto/asymmetric_keys/pkcs7_parser.c   | 81 +++++++++++++++++++++++++
 crypto/asymmetric_keys/pkcs7_parser.h   |  4 ++
 crypto/asymmetric_keys/pkcs7_trust.c    |  9 +++
 crypto/asymmetric_keys/pkcs7_verify.c   | 13 ++--
 include/crypto/pkcs7.h                  |  4 ++
 include/linux/verification.h            |  2 +
 10 files changed, 216 insertions(+), 37 deletions(-)
 create mode 100644 crypto/asymmetric_keys/pkcs7_aa.asn1

-- 
2.51.0

[PATCH v3 1/5] certs: break out pkcs7 check into its own function

[James Bottomley]

Add new validate_pkcs7_trust() function which can operate on the
system keyrings and is simply some of the innards of
verify_pkcs7_message_sig().

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 certs/system_keyring.c       | 76 +++++++++++++++++++++---------------
 include/linux/verification.h |  2 +
 2 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index e0761436ec7f..dcbefc2d3f6d 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -298,42 +298,19 @@ late_initcall(load_system_certificate_list);
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 /**
- * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
- * @data: The data to be verified (NULL if expecting internal data).
- * @len: Size of @data.
+ * validate_pkcs7_trust - add trust markers based on keyring
  * @pkcs7: The PKCS#7 message that is the signature.
  * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
  *					(void *)1UL for all trusted keys).
- * @usage: The use to which the key is being put.
- * @view_content: Callback to gain access to content.
- * @ctx: Context for callback.
  */
-int verify_pkcs7_message_sig(const void *data, size_t len,
-			     struct pkcs7_message *pkcs7,
-			     struct key *trusted_keys,
-			     enum key_being_used_for usage,
-			     int (*view_content)(void *ctx,
-						 const void *data, size_t len,
-						 size_t asn1hdrlen),
-			     void *ctx)
+int validate_pkcs7_trust(struct pkcs7_message *pkcs7, struct key *trusted_keys)
 {
 	int ret;
 
-	/* The data should be detached - so we need to supply it. */
-	if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
-		pr_err("PKCS#7 signature with non-detached data\n");
-		ret = -EBADMSG;
-		goto error;
-	}
-
-	ret = pkcs7_verify(pkcs7, usage);
-	if (ret < 0)
-		goto error;
-
 	ret = is_key_on_revocation_list(pkcs7);
 	if (ret != -ENOKEY) {
 		pr_devel("PKCS#7 key is on revocation list\n");
-		goto error;
+		return ret;
 	}
 
 	if (!trusted_keys) {
@@ -351,18 +328,55 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
 		trusted_keys = NULL;
 #endif
 		if (!trusted_keys) {
-			ret = -ENOKEY;
 			pr_devel("PKCS#7 platform keyring is not available\n");
-			goto error;
+			return -ENOKEY;
 		}
 	}
 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
-	if (ret < 0) {
-		if (ret == -ENOKEY)
-			pr_devel("PKCS#7 signature not signed with a trusted key\n");
+	if (ret == -ENOKEY)
+		pr_devel("PKCS#7 signature not signed with a trusted key\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(validate_pkcs7_trust);
+
+/**
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @pkcs7: The PKCS#7 message that is the signature.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ *					(void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_message_sig(const void *data, size_t len,
+			     struct pkcs7_message *pkcs7,
+			     struct key *trusted_keys,
+			     enum key_being_used_for usage,
+			     int (*view_content)(void *ctx,
+						 const void *data, size_t len,
+						 size_t asn1hdrlen),
+			     void *ctx)
+{
+	int ret;
+
+	/* The data should be detached - so we need to supply it. */
+	if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
+		pr_err("PKCS#7 signature with non-detached data\n");
+		ret = -EBADMSG;
 		goto error;
 	}
 
+	ret = pkcs7_verify(pkcs7, usage);
+	if (ret < 0)
+		goto error;
+
+	ret = validate_pkcs7_trust(pkcs7, trusted_keys);
+	if (ret < 0)
+		goto error;
+
 	if (view_content) {
 		size_t asn1hdrlen;
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index dec7f2beabfd..57f1460d36f1 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -44,6 +44,8 @@ enum key_being_used_for {
 struct key;
 struct pkcs7_message;
 
+extern int validate_pkcs7_trust(struct pkcs7_message *pkcs7,
+				struct key *trusted_keys);
 extern int verify_pkcs7_signature(const void *data, size_t len,
 				  const void *raw_pkcs7, size_t pkcs7_len,
 				  struct key *trusted_keys,
-- 
2.51.0

[PATCH v3 2/5] crypto: pkcs7: add flag for validated trust on a signed info block

[James Bottomley]

Allow consumers of struct pkcs7_message to tell if any of the sinfo
fields has passed a trust validation.  Note that this does not happen
in parsing, pkcs7_validate_trust() must be explicitly called or called
via validate_pkcs7_trust().

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 crypto/asymmetric_keys/pkcs7_parser.h | 1 +
 crypto/asymmetric_keys/pkcs7_trust.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.h b/crypto/asymmetric_keys/pkcs7_parser.h
index 6ef9f335bb17..203062a33def 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.h
+++ b/crypto/asymmetric_keys/pkcs7_parser.h
@@ -20,6 +20,7 @@ struct pkcs7_signed_info {
 	unsigned	index;
 	bool		unsupported_crypto;	/* T if not usable due to missing crypto */
 	bool		blacklisted;
+	bool		verified; /* T if this signer has validated trust */
 
 	/* Message digest - the digest of the Content Data (or NULL) */
 	const void	*msgdigest;
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index 9a87c34ed173..78ebfb6373b6 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -127,6 +127,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
 		for (p = sinfo->signer; p != x509; p = p->signer)
 			p->verified = true;
 	}
+	sinfo->verified = true;
 	kleave(" = 0");
 	return 0;
 }
-- 
2.51.0

[PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[James Bottomley]

Trying to run pkcs7_validate_trust() on something that parsed
correctly but is not verified doesn't work because the signature
digest hasn't been calculated.  Fix this by adding a digest calclation
in to pkcs7_validate_one().  This is almost a nop if the digest exists.

Additionally, the trust validation doesn't know the data payload, so
adjust the digest calculator to skip checking the data digest if
pkcs7->data is NULL.  A check is added in pkcs7_verify() for
pkcs7->data being null (returning -EBADMSG) to guard against someone
forgetting to supply data and getting an invalid success return.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 crypto/asymmetric_keys/pkcs7_parser.h |  3 +++
 crypto/asymmetric_keys/pkcs7_trust.c  |  8 ++++++++
 crypto/asymmetric_keys/pkcs7_verify.c | 13 +++++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.h b/crypto/asymmetric_keys/pkcs7_parser.h
index 203062a33def..cbe823aeac06 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.h
+++ b/crypto/asymmetric_keys/pkcs7_parser.h
@@ -66,3 +66,6 @@ struct pkcs7_message {
 	size_t		data_hdrlen;	/* Length of Data ASN.1 header */
 	const void	*data;		/* Content Data (or 0) */
 };
+
+int pkcs7_digest(struct pkcs7_message *pkcs7,
+		 struct pkcs7_signed_info *sinfo);
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index 78ebfb6373b6..7cb0a6bc7b32 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -30,6 +30,14 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
 
 	kenter(",%u,", sinfo->index);
 
+	/*
+	 * if we're being called immediately after parse, the
+	 * signature won't have a calculated digest yet, so calculate
+	 * one.  This function returns immediately if a digest has
+	 * already been calculated
+	 */
+	pkcs7_digest(pkcs7, sinfo);
+
 	if (sinfo->unsupported_crypto) {
 		kleave(" = -ENOPKG [cached]");
 		return -ENOPKG;
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 474e2c1ae21b..3080f0ec52e0 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -19,8 +19,8 @@
 /*
  * Digest the relevant parts of the PKCS#7 data
  */
-static int pkcs7_digest(struct pkcs7_message *pkcs7,
-			struct pkcs7_signed_info *sinfo)
+int pkcs7_digest(struct pkcs7_message *pkcs7,
+		 struct pkcs7_signed_info *sinfo)
 {
 	struct public_key_signature *sig = sinfo->sig;
 	struct crypto_shash *tfm;
@@ -93,8 +93,8 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 			goto error;
 		}
 
-		if (memcmp(sig->m, sinfo->msgdigest,
-			   sinfo->msgdigest_len) != 0) {
+		if (pkcs7->data && memcmp(sig->m, sinfo->msgdigest,
+					  sinfo->msgdigest_len) != 0) {
 			pr_warn("Sig %u: Message digest doesn't match\n",
 				sinfo->index);
 			ret = -EKEYREJECTED;
@@ -463,6 +463,11 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 		return -EINVAL;
 	}
 
+	if (!pkcs7->data) {
+		pr_warn("Data not supplied to verify operation\n");
+		return -EBADMSG;
+	}
+
 	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
 		ret = pkcs7_verify_one(pkcs7, sinfo);
 		if (sinfo->blacklisted) {
-- 
2.51.0

[PATCH v3 4/5] crypto: pkcs7: add ability to extract signed attributes by OID

[James Bottomley]

Signers may add any information they like in signed attributes and
sometimes this information turns out to be relevant to specific
signing cases, so add an api pkcs7_get_authattr() to extract the value
of an authenticated attribute by specific OID.  The current
implementation is designed for the single signer use case and simply
terminates the search when it finds the relevant OID.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
v2: fix docbook
---
 crypto/asymmetric_keys/Makefile       |  4 +-
 crypto/asymmetric_keys/pkcs7_aa.asn1  | 18 ++++++
 crypto/asymmetric_keys/pkcs7_parser.c | 81 +++++++++++++++++++++++++++
 include/crypto/pkcs7.h                |  4 ++
 4 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 crypto/asymmetric_keys/pkcs7_aa.asn1

diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile
index bc65d3b98dcb..f99b7169ae7c 100644
--- a/crypto/asymmetric_keys/Makefile
+++ b/crypto/asymmetric_keys/Makefile
@@ -53,12 +53,14 @@ clean-files	+= pkcs8.asn1.c pkcs8.asn1.h
 obj-$(CONFIG_PKCS7_MESSAGE_PARSER) += pkcs7_message.o
 pkcs7_message-y := \
 	pkcs7.asn1.o \
+	pkcs7_aa.asn1.o \
 	pkcs7_parser.o \
 	pkcs7_trust.o \
 	pkcs7_verify.o
 
-$(obj)/pkcs7_parser.o: $(obj)/pkcs7.asn1.h
+$(obj)/pkcs7_parser.o: $(obj)/pkcs7.asn1.h $(obj)/pkcs7_aa.asn1.h
 $(obj)/pkcs7.asn1.o: $(obj)/pkcs7.asn1.c $(obj)/pkcs7.asn1.h
+$(obj)/pkcs7_aa.asn1.o: $(obj)/pkcs7_aa.asn1.c $(obj)/pkcs7_aa.asn1.h
 
 #
 # PKCS#7 parser testing key
diff --git a/crypto/asymmetric_keys/pkcs7_aa.asn1 b/crypto/asymmetric_keys/pkcs7_aa.asn1
new file mode 100644
index 000000000000..7a8857bdf56e
--- /dev/null
+++ b/crypto/asymmetric_keys/pkcs7_aa.asn1
@@ -0,0 +1,18 @@
+-- SPDX-License-Identifier: BSD-3-Clause
+--
+-- Copyright (C) 2009 IETF Trust and the persons identified as authors
+-- of the code
+--
+-- https://www.rfc-editor.org/rfc/rfc5652#section-3
+
+AA ::= 	CHOICE {
+	aaSet		[0] IMPLICIT AASet,
+	aaSequence	[2] EXPLICIT SEQUENCE OF AuthenticatedAttribute
+}
+
+AASet ::= SET OF AuthenticatedAttribute
+
+AuthenticatedAttribute ::= SEQUENCE {
+	type	OBJECT IDENTIFIER ({ pkcs7_aa_note_OID }),
+	values	SET OF ANY ({ pkcs7_aa_note_attr })
+}
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index 6e3ffdac83ac..d467866f7d93 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -15,6 +15,7 @@
 #include <crypto/public_key.h>
 #include "pkcs7_parser.h"
 #include "pkcs7.asn1.h"
+#include "pkcs7_aa.asn1.h"
 
 MODULE_DESCRIPTION("PKCS#7 parser");
 MODULE_AUTHOR("Red Hat, Inc.");
@@ -211,6 +212,86 @@ int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 }
 EXPORT_SYMBOL_GPL(pkcs7_get_content_data);
 
+struct pkcs7_aa_context {
+	bool found;
+	enum OID oid_to_find;
+	const void *data;
+	size_t len;
+};
+
+int pkcs7_aa_note_OID(void *context, size_t hdrlen,
+		      unsigned char tag,
+		      const void *value, size_t vlen)
+{
+	struct pkcs7_aa_context *ctx = context;
+	enum OID oid = look_up_OID(value, vlen);
+
+	ctx->found = (oid == ctx->oid_to_find);
+
+	return 0;
+}
+
+int pkcs7_aa_note_attr(void *context, size_t hdrlen,
+		       unsigned char tag,
+		       const void *value, size_t vlen)
+{
+	struct pkcs7_aa_context *ctx = context;
+
+	if (ctx->found) {
+		ctx->data = value;
+		ctx->len = vlen;
+	}
+
+	return 0;
+}
+
+/**
+ * pkcs7_get_authattr - get authenticated attribute by OID
+ * @pkcs7: The preparsed PKCS#7 message
+ * @oid: the enum value of the OID to find
+ * @_data: Place to return a pointer to the attribute value
+ * @_len: length of the attribute value
+ *
+ * Searches the authenticated attributes until one is found with a
+ * matching OID.  Note that because the attributes are per signer
+ * there could be multiple signers with different values, but this
+ * routine will simply return the first one in parse order.
+ *
+ * Returns -ENODATA if the attribute can't be found
+ */
+int pkcs7_get_authattr(const struct pkcs7_message *pkcs7,
+		       enum OID oid,
+		       const void **_data, size_t *_len)
+{
+	struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
+	struct pkcs7_aa_context ctx;
+
+	ctx.data = NULL;
+	ctx.oid_to_find = oid;
+
+	for (; sinfo; sinfo = sinfo->next) {
+		int ret;
+
+		/* only extract OIDs from validated signers */
+		if (!sinfo->verified)
+			continue;
+
+		ret = asn1_ber_decoder(&pkcs7_aa_decoder, &ctx,
+				       sinfo->authattrs, sinfo->authattrs_len);
+		if (ret < 0 || ctx.data != NULL)
+			break;
+	}
+
+	if (!ctx.data)
+		return -ENODATA;
+
+	*_data = ctx.data;
+	*_len = ctx.len;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pkcs7_get_authattr);
+
 /*
  * Note an OID when we find one for later processing when we know how
  * to interpret it.
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 38ec7f5f9041..bd83202cd805 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -25,6 +25,10 @@ extern void pkcs7_free_message(struct pkcs7_message *pkcs7);
 extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 				  const void **_data, size_t *_datalen,
 				  size_t *_headerlen);
+extern int pkcs7_get_authattr(const struct pkcs7_message *pkcs7,
+			      enum OID oid,
+			      const void **_data, size_t *_len);
+
 
 /*
  * pkcs7_trust.c
-- 
2.51.0

[PATCH v3 5/5] crypto: pkcs7: add tests for pkcs7_get_authattr

[James Bottomley]

Add example code to the test module pkcs7_key_type.c that verifies a
message and then pulls out a known authenticated attribute.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Acked-by: David Howells <dhowells@redhat.com>

---
v2: add ack

I'm not convinced this needs adding, but it provided a convenient
mechanism for testing the pcks7_get_authattr() call so I added it in
case others find it useful.
---
 crypto/asymmetric_keys/pkcs7_key_type.c | 42 ++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c b/crypto/asymmetric_keys/pkcs7_key_type.c
index b930d3bbf1af..5a1ecb5501b2 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -12,6 +12,7 @@
 #include <linux/verification.h>
 #include <linux/key-type.h>
 #include <keys/user-type.h>
+#include <crypto/pkcs7.h>
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("PKCS#7 testing key type");
@@ -51,16 +52,55 @@ static int pkcs7_view_content(void *ctx, const void *data, size_t len,
 static int pkcs7_preparse(struct key_preparsed_payload *prep)
 {
 	enum key_being_used_for usage = pkcs7_usage;
+	int ret;
+	struct pkcs7_message *pkcs7;
+	const void *data;
+	size_t len;
 
 	if (usage >= NR__KEY_BEING_USED_FOR) {
 		pr_err("Invalid usage type %d\n", usage);
 		return -EINVAL;
 	}
 
-	return verify_pkcs7_signature(NULL, 0,
+	ret = verify_pkcs7_signature(NULL, 0,
 				      prep->data, prep->datalen,
 				      VERIFY_USE_SECONDARY_KEYRING, usage,
 				      pkcs7_view_content, prep);
+	if (ret)
+		return ret;
+
+	pkcs7 = pkcs7_parse_message(prep->data, prep->datalen);
+	if (IS_ERR(pkcs7)) {
+		pr_err("pkcs7 parse error\n");
+		return PTR_ERR(pkcs7);
+	}
+
+	/*
+	 * the parsed message has no trusted signer, so nothing should
+	 * be returned here
+	 */
+	ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
+	if (ret == 0) {
+		pr_err("OID returned when no trust in signer\n");
+		goto out;
+	}
+	/* add trust and check again */
+	ret = validate_pkcs7_trust(pkcs7, VERIFY_USE_SECONDARY_KEYRING);
+	if (ret) {
+		pr_err("validate_pkcs7_trust failed!!\n");
+		goto out;
+	}
+	/* now we should find the OID */
+	ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
+	if (ret) {
+		pr_err("Failed to get message digest\n");
+		goto out;
+	}
+	pr_info("Correctly Got message hash, size=%ld\n", len);
+
+ out:
+	pkcs7_free_message(pkcs7);
+	return 0;
 }
 
 /*
-- 
2.51.0

Re: [PATCH v3 5/5] crypto: pkcs7: add tests for pkcs7_get_authattr

[kernel test robot]

Hi James,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v7.0-rc1 next-20260225]
[cannot apply to herbert-cryptodev-2.6/master herbert-crypto-2.6/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Bottomley/certs-break-out-pkcs7-check-into-its-own-function/20260226-052454
base:   linus/master
patch link:    https://lore.kernel.org/r/20260225211907.7368-6-James.Bottomley%40HansenPartnership.com
patch subject: [PATCH v3 5/5] crypto: pkcs7: add tests for pkcs7_get_authattr
config: i386-buildonly-randconfig-002-20260226 (https://download.01.org/0day-ci/archive/20260226/202602260949.uDrsr8hd-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260226/202602260949.uDrsr8hd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602260949.uDrsr8hd-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> crypto/asymmetric_keys/pkcs7_key_type.c:99:52: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
      99 |         pr_info("Correctly Got message hash, size=%ld\n", len);
         |                                                   ~~~     ^~~
         |                                                   %zu
   include/linux/printk.h:584:34: note: expanded from macro 'pr_info'
     584 |         printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
         |                                 ~~~     ^~~~~~~~~~~
   include/linux/printk.h:511:60: note: expanded from macro 'printk'
     511 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
         |                                                     ~~~    ^~~~~~~~~~~
   include/linux/printk.h:483:19: note: expanded from macro 'printk_index_wrap'
     483 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ~~~~    ^~~~~~~~~~~
   1 warning generated.


vim +99 crypto/asymmetric_keys/pkcs7_key_type.c

    48	
    49	/*
    50	 * Preparse a PKCS#7 wrapped and validated data blob.
    51	 */
    52	static int pkcs7_preparse(struct key_preparsed_payload *prep)
    53	{
    54		enum key_being_used_for usage = pkcs7_usage;
    55		int ret;
    56		struct pkcs7_message *pkcs7;
    57		const void *data;
    58		size_t len;
    59	
    60		if (usage >= NR__KEY_BEING_USED_FOR) {
    61			pr_err("Invalid usage type %d\n", usage);
    62			return -EINVAL;
    63		}
    64	
    65		ret = verify_pkcs7_signature(NULL, 0,
    66					      prep->data, prep->datalen,
    67					      VERIFY_USE_SECONDARY_KEYRING, usage,
    68					      pkcs7_view_content, prep);
    69		if (ret)
    70			return ret;
    71	
    72		pkcs7 = pkcs7_parse_message(prep->data, prep->datalen);
    73		if (IS_ERR(pkcs7)) {
    74			pr_err("pkcs7 parse error\n");
    75			return PTR_ERR(pkcs7);
    76		}
    77	
    78		/*
    79		 * the parsed message has no trusted signer, so nothing should
    80		 * be returned here
    81		 */
    82		ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
    83		if (ret == 0) {
    84			pr_err("OID returned when no trust in signer\n");
    85			goto out;
    86		}
    87		/* add trust and check again */
    88		ret = validate_pkcs7_trust(pkcs7, VERIFY_USE_SECONDARY_KEYRING);
    89		if (ret) {
    90			pr_err("validate_pkcs7_trust failed!!\n");
    91			goto out;
    92		}
    93		/* now we should find the OID */
    94		ret = pkcs7_get_authattr(pkcs7, OID_messageDigest, &data, &len);
    95		if (ret) {
    96			pr_err("Failed to get message digest\n");
    97			goto out;
    98		}
  > 99		pr_info("Correctly Got message hash, size=%ld\n", len);
   100	
   101	 out:
   102		pkcs7_free_message(pkcs7);
   103		return 0;
   104	}
   105	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[Eric Biggers]

On Wed, Feb 25, 2026 at 04:19:02PM -0500, James Bottomley wrote:
> Although the biggest use of signed attributes is PKCS#7 and X509
> specific data, they can be added to a signature to support arbitrary
> and verifiable objects.  This makes them particularly useful when you
> want to take an existing signature scheme and extend it with
> additional (but always verified) data in such a way that it still
> looks valid to both the old and new schemes.

What kernel subsystem is going to use this, and how?  As-is the only
caller of pkcs7_get_authattr() that you've proposed is in test code.

If this is for some out-of-tree module, we don't do that.

I'll also note that we should generally be aiming to simplify the PKCS#7
signature verification code, not making it even more complex.

- Eric

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[James Bottomley]

On Wed, 2026-02-25 at 18:13 -0800, Eric Biggers wrote:
> On Wed, Feb 25, 2026 at 04:19:02PM -0500, James Bottomley wrote:
> > Although the biggest use of signed attributes is PKCS#7 and X509
> > specific data, they can be added to a signature to support
> > arbitrary and verifiable objects.  This makes them particularly
> > useful when you want to take an existing signature scheme and
> > extend it with additional (but always verified) data in such a way
> > that it still looks valid to both the old and new schemes.
> 
> What kernel subsystem is going to use this, and how?  As-is the only
> caller of pkcs7_get_authattr() that you've proposed is in test code.

Sure, it wasn't posted when v2 was so I couldn't add it, but it has
been posted since:

https://lore.kernel.org/all/20251211021257.1208712-1-bboscaccy@linux.microsoft.com/

> If this is for some out-of-tree module, we don't do that.
> 
> I'll also note that we should generally be aiming to simplify the
> PKCS#7 signature verification code, not making it even more complex.

I'm fine with the general goal, but since the current code verifies the
signature, pulls out the message hash and other attributes, compares
the message against the MessageDigest one and then frees the whole
structure it's a bit hard to see how the current goal can be achieved
without extracting at least the first part of that ... but if you have
a suggestion, I'm happy to implement.

Regards,

James

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[Eric Biggers]

On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley wrote:
> +	/*
> +	 * if we're being called immediately after parse, the
> +	 * signature won't have a calculated digest yet, so calculate
> +	 * one.  This function returns immediately if a digest has
> +	 * already been calculated
> +	 */
> +	pkcs7_digest(pkcs7, sinfo);

pkcs7_digest() can fail, returning an error code and leaving sig->m ==
NULL && sig->m_size == 0.  Here, the error is just being ignored.
Doesn't that then cause the signature verification to proceed against an
empty message, rather than anything related to the data provided?

- Eric

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[James Bottomley]

On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley wrote:
> > +	/*
> > +	 * if we're being called immediately after parse, the
> > +	 * signature won't have a calculated digest yet, so
> > calculate
> > +	 * one.  This function returns immediately if a digest has
> > +	 * already been calculated
> > +	 */
> > +	pkcs7_digest(pkcs7, sinfo);
> 
> pkcs7_digest() can fail, returning an error code and leaving sig->m
> == NULL && sig->m_size == 0.  Here, the error is just being ignored.

That's right.  Basically I wasn't sure what to return on error
(although -ENOKEY looks about right since it will cause retries on a
different sig chain).

> Doesn't that then cause the signature verification to proceed against
> an empty message, rather than anything related to the data provided?

Not if sig->m is NULL, no, because the verifier will try to reget the
digest in that case (and error out if it fails).

However, were you possibly thinking of the case where you first call
pkcs7_validate_trust() to verify the signature (and extract attributes)
and then call verify_pkcs7_message_sig() on the same pkcs7?  Because in
the latter case the hash of the supplied data wouldn't get checked
because the second call to pkcs7_digest() would find sig->m populated
and assume the data hash had been verified against the attributes.

Regards,

James

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[Eric Biggers]

On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley wrote:
> > If this is for some out-of-tree module, we don't do that.
> > 
> > I'll also note that we should generally be aiming to simplify the
> > PKCS#7 signature verification code, not making it even more complex.
> 
> I'm fine with the general goal, but since the current code verifies the
> signature, pulls out the message hash and other attributes, compares
> the message against the MessageDigest one and then frees the whole
> structure it's a bit hard to see how the current goal can be achieved
> without extracting at least the first part of that ... but if you have
> a suggestion, I'm happy to implement.

Sure, just incorporate your auxiliary data into the actual message being
signed and verified.  Something like:
    
    program_len || program || hash*

Then there's no need for the complexity of signed attributes.

You could also just use a regular signature without all the pointless
complexity and bugs of PKCS#7.

- Eric

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[Eric Biggers]

On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley wrote:
> On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley wrote:
> > > +	/*
> > > +	 * if we're being called immediately after parse, the
> > > +	 * signature won't have a calculated digest yet, so
> > > calculate
> > > +	 * one.  This function returns immediately if a digest has
> > > +	 * already been calculated
> > > +	 */
> > > +	pkcs7_digest(pkcs7, sinfo);
> > 
> > pkcs7_digest() can fail, returning an error code and leaving sig->m
> > == NULL && sig->m_size == 0.  Here, the error is just being ignored.
> 
> That's right.  Basically I wasn't sure what to return on error
> (although -ENOKEY looks about right since it will cause retries on a
> different sig chain).
> 
> > Doesn't that then cause the signature verification to proceed against
> > an empty message, rather than anything related to the data provided?
> 
> Not if sig->m is NULL, no, because the verifier will try to reget the
> digest in that case (and error out if it fails).

Can you point to where that happens?  It still looks like it just
proceeds with an empty message.

I would think verifying the message is kind of the point, right?

- Eric

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[James Bottomley]

On Wed, 2026-03-04 at 23:55 -0800, Eric Biggers wrote:
> On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley wrote:
> > > If this is for some out-of-tree module, we don't do that.
> > > 
> > > I'll also note that we should generally be aiming to simplify the
> > > PKCS#7 signature verification code, not making it even more
> > > complex.
> > 
> > I'm fine with the general goal, but since the current code verifies
> > the signature, pulls out the message hash and other attributes,
> > compares the message against the MessageDigest one and then frees
> > the whole structure it's a bit hard to see how the current goal can
> > be achieved without extracting at least the first part of that ...
> > but if you have   suggestion, I'm happy to implement.
> 
> Sure, just incorporate your auxiliary data into the actual message
> being signed and verified.  Something like:
>     
>     program_len || program || hash*

We can't do that because the second hash is for the LSM.  If there's no
LSM then we need the signature to pass the current eBPF signature check
because the second hash will be verified by the loader, which means the
program hash and nothing else must be in the messageDigest attr.

Regards,

James
dddddddddddddddddddddddddddddddddddddddddddddddddddddddd

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[James Bottomley]

On Wed, 2026-03-04 at 23:58 -0800, Eric Biggers wrote:
> On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley wrote:
> > On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley wrote:
> > > > +	/*
> > > > +	 * if we're being called immediately after parse, the
> > > > +	 * signature won't have a calculated digest yet, so
> > > > calculate
> > > > +	 * one.  This function returns immediately if a digest
> > > > has
> > > > +	 * already been calculated
> > > > +	 */
> > > > +	pkcs7_digest(pkcs7, sinfo);
> > > 
> > > pkcs7_digest() can fail, returning an error code and leaving sig-
> > > >m
> > > == NULL && sig->m_size == 0.  Here, the error is just being
> > > ignored.
> > 
> > That's right.  Basically I wasn't sure what to return on error
> > (although -ENOKEY looks about right since it will cause retries on
> > a
> > different sig chain).
> > 
> > > Doesn't that then cause the signature verification to proceed
> > > against
> > > an empty message, rather than anything related to the data
> > > provided?
> > 
> > Not if sig->m is NULL, no, because the verifier will try to reget
> > the digest in that case (and error out if it fails).
> 
> Can you point to where that happens?  It still looks like it just
> proceeds with an empty message.

It's the obvious one:

verify_pkcs7_message_sig->pkcs7_verify->pkcs7_verify_one->pkcs7_digest

The latter will allocate and calculate the digest if sig->m is null.

Regards,

James

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[Eric Biggers]

On Thu, Mar 05, 2026 at 09:53:56AM -0500, James Bottomley wrote:
> On Wed, 2026-03-04 at 23:58 -0800, Eric Biggers wrote:
> > On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley wrote:
> > > On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > > > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley wrote:
> > > > > +	/*
> > > > > +	 * if we're being called immediately after parse, the
> > > > > +	 * signature won't have a calculated digest yet, so
> > > > > calculate
> > > > > +	 * one.  This function returns immediately if a digest
> > > > > has
> > > > > +	 * already been calculated
> > > > > +	 */
> > > > > +	pkcs7_digest(pkcs7, sinfo);
> > > > 
> > > > pkcs7_digest() can fail, returning an error code and leaving sig-
> > > > >m
> > > > == NULL && sig->m_size == 0.  Here, the error is just being
> > > > ignored.
> > > 
> > > That's right.  Basically I wasn't sure what to return on error
> > > (although -ENOKEY looks about right since it will cause retries on
> > > a
> > > different sig chain).
> > > 
> > > > Doesn't that then cause the signature verification to proceed
> > > > against
> > > > an empty message, rather than anything related to the data
> > > > provided?
> > > 
> > > Not if sig->m is NULL, no, because the verifier will try to reget
> > > the digest in that case (and error out if it fails).
> > 
> > Can you point to where that happens?  It still looks like it just
> > proceeds with an empty message.
> 
> It's the obvious one:
> 
> verify_pkcs7_message_sig->pkcs7_verify->pkcs7_verify_one->pkcs7_digest
> 
> The latter will allocate and calculate the digest if sig->m is null.
> 
> Regards,
> 
> James
> 

But looking at hornet_check_program() from
https://lore.kernel.org/linux-security-module/20251211021257.1208712-9-bboscaccy@linux.microsoft.com/,
it calls:

    pkcs7_parse_message()
    validate_pkcs7_trust()
    pkcs7_get_authattr()

The actual signature check happens in validate_pkcs7_trust(), which
appears to have the issue where it can proceed with an empty message, as
I mentioned.

- Eric

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[Eric Biggers]

On Thu, Mar 05, 2026 at 09:46:42AM -0500, James Bottomley wrote:
> On Wed, 2026-03-04 at 23:55 -0800, Eric Biggers wrote:
> > On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley wrote:
> > > > If this is for some out-of-tree module, we don't do that.
> > > > 
> > > > I'll also note that we should generally be aiming to simplify the
> > > > PKCS#7 signature verification code, not making it even more
> > > > complex.
> > > 
> > > I'm fine with the general goal, but since the current code verifies
> > > the signature, pulls out the message hash and other attributes,
> > > compares the message against the MessageDigest one and then frees
> > > the whole structure it's a bit hard to see how the current goal can
> > > be achieved without extracting at least the first part of that ...
> > > but if you have   suggestion, I'm happy to implement.
> > 
> > Sure, just incorporate your auxiliary data into the actual message
> > being signed and verified.  Something like:
> >     
> >     program_len || program || hash*
> 
> We can't do that because the second hash is for the LSM.  If there's no
> LSM then we need the signature to pass the current eBPF signature check
> because the second hash will be verified by the loader, which means the
> program hash and nothing else must be in the messageDigest attr.
> 

Why does the loader need to verify the signature if the kernel has to do
it anyway, and why does the loader need to skip verifying the maps?

- Eric

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[James Bottomley]

On Thu, 2026-03-05 at 10:50 -0800, Eric Biggers wrote:
> On Thu, Mar 05, 2026 at 09:53:56AM -0500, James Bottomley wrote:
> > On Wed, 2026-03-04 at 23:58 -0800, Eric Biggers wrote:
> > > On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley wrote:
> > > > On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > > > > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley
> > > > > wrote:
> > > > > > +	/*
> > > > > > +	 * if we're being called immediately after parse,
> > > > > > the
> > > > > > +	 * signature won't have a calculated digest yet,
> > > > > > so
> > > > > > calculate
> > > > > > +	 * one.  This function returns immediately if a
> > > > > > digest
> > > > > > has
> > > > > > +	 * already been calculated
> > > > > > +	 */
> > > > > > +	pkcs7_digest(pkcs7, sinfo);
> > > > > 
> > > > > pkcs7_digest() can fail, returning an error code and leaving
> > > > > sig-
> > > > > > m
> > > > > == NULL && sig->m_size == 0.  Here, the error is just being
> > > > > ignored.
> > > > 
> > > > That's right.  Basically I wasn't sure what to return on error
> > > > (although -ENOKEY looks about right since it will cause retries
> > > > on
> > > > a
> > > > different sig chain).
> > > > 
> > > > > Doesn't that then cause the signature verification to proceed
> > > > > against an empty message, rather than anything related to the
> > > > > data provided?
> > > > 
> > > > Not if sig->m is NULL, no, because the verifier will try to
> > > > reget the digest in that case (and error out if it fails).
> > > 
> > > Can you point to where that happens?  It still looks like it just
> > > proceeds with an empty message.
> > 
> > It's the obvious one:
> > 
> > verify_pkcs7_message_sig->pkcs7_verify->pkcs7_verify_one-
> > >pkcs7_digest
> > 
> > The latter will allocate and calculate the digest if sig->m is
> > null.
> > 
> > Regards,
> > 
> > James
> > 
> 
> But looking at hornet_check_program() from
> https://lore.kernel.org/linux-security-module/20251211021257.1208712-9-bboscaccy@linux.microsoft.com/
> ,
> it calls:
> 
>     pkcs7_parse_message()
>     validate_pkcs7_trust()
>     pkcs7_get_authattr()
> 
> The actual signature check happens in validate_pkcs7_trust(), which
> appears to have the issue where it can proceed with an empty message,
> as I mentioned.

The whole design of validate_pkccs7_trust() is to validate the
signature only so we can trust the attributes.  It doesn't actually
verify the digest against the data, that's the job of the
verify_pkcs7_sig.. class of functions.  The original thought behind
this was that we might not have the original data by the time we came
to extract the OID.  However, it turns out we do, so the split of the
trust functions is not really necessary..

Regards,

James

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[James Bottomley]

On Thu, 2026-03-05 at 10:51 -0800, Eric Biggers wrote:
> On Thu, Mar 05, 2026 at 09:46:42AM -0500, James Bottomley wrote:
> > On Wed, 2026-03-04 at 23:55 -0800, Eric Biggers wrote:
> > > On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley wrote:
> > > > > If this is for some out-of-tree module, we don't do that.
> > > > > 
> > > > > I'll also note that we should generally be aiming to simplify
> > > > > the PKCS#7 signature verification code, not making it even
> > > > > more complex.
> > > > 
> > > > I'm fine with the general goal, but since the current code
> > > > verifies the signature, pulls out the message hash and other
> > > > attributes, compares the message against the MessageDigest one
> > > > and then frees the whole structure it's a bit hard to see how
> > > > the current goal can be achieved without extracting at least
> > > > the first part of that
> > > > ...
> > > > but if you have   suggestion, I'm happy to implement.
> > > 
> > > Sure, just incorporate your auxiliary data into the actual
> > > message being signed and verified.  Something like:
> > >     
> > >     program_len || program || hash*
> > 
> > We can't do that because the second hash is for the LSM.  If
> > there's no LSM then we need the signature to pass the current eBPF
> > signature check because the second hash will be verified by the
> > loader, which means the program hash and nothing else must be in
> > the messageDigest attr.
> > 
> 
> Why does the loader need to verify the signature if the kernel has to
> do it anyway, and why does the loader need to skip verifying the
> maps?

Well, I didn't say kernel, I said LSM.  The problem is that the last
hook in the LSM chain for eBPF loading occurs before the loader has
actually run.  This means that either the LSM needs to be assured
verification will complete (by running it itself), which is what the
patch set I pointed to does; or that we need an additional verification
hook in eBPF somewhere in the verifier after the loader has run, which
the eBPF people are looking at but haven't actually found anything yet.

The OID helps the LSM do the additional verification without changing
any of the eBPF loading flow.

Regards,

James

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[Eric Biggers]

On Thu, Mar 05, 2026 at 03:11:29PM -0500, James Bottomley wrote:
> On Thu, 2026-03-05 at 10:50 -0800, Eric Biggers wrote:
> > On Thu, Mar 05, 2026 at 09:53:56AM -0500, James Bottomley wrote:
> > > On Wed, 2026-03-04 at 23:58 -0800, Eric Biggers wrote:
> > > > On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley wrote:
> > > > > On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > > > > > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley
> > > > > > wrote:
> > > > > > > +	/*
> > > > > > > +	 * if we're being called immediately after parse,
> > > > > > > the
> > > > > > > +	 * signature won't have a calculated digest yet,
> > > > > > > so
> > > > > > > calculate
> > > > > > > +	 * one.  This function returns immediately if a
> > > > > > > digest
> > > > > > > has
> > > > > > > +	 * already been calculated
> > > > > > > +	 */
> > > > > > > +	pkcs7_digest(pkcs7, sinfo);
> > > > > > 
> > > > > > pkcs7_digest() can fail, returning an error code and leaving
> > > > > > sig-
> > > > > > > m
> > > > > > == NULL && sig->m_size == 0.  Here, the error is just being
> > > > > > ignored.
> > > > > 
> > > > > That's right.  Basically I wasn't sure what to return on error
> > > > > (although -ENOKEY looks about right since it will cause retries
> > > > > on
> > > > > a
> > > > > different sig chain).
> > > > > 
> > > > > > Doesn't that then cause the signature verification to proceed
> > > > > > against an empty message, rather than anything related to the
> > > > > > data provided?
> > > > > 
> > > > > Not if sig->m is NULL, no, because the verifier will try to
> > > > > reget the digest in that case (and error out if it fails).
> > > > 
> > > > Can you point to where that happens?  It still looks like it just
> > > > proceeds with an empty message.
> > > 
> > > It's the obvious one:
> > > 
> > > verify_pkcs7_message_sig->pkcs7_verify->pkcs7_verify_one-
> > > >pkcs7_digest
> > > 
> > > The latter will allocate and calculate the digest if sig->m is
> > > null.
> > > 
> > > Regards,
> > > 
> > > James
> > > 
> > 
> > But looking at hornet_check_program() from
> > https://lore.kernel.org/linux-security-module/20251211021257.1208712-9-bboscaccy@linux.microsoft.com/
> > ,
> > it calls:
> > 
> >     pkcs7_parse_message()
> >     validate_pkcs7_trust()
> >     pkcs7_get_authattr()
> > 
> > The actual signature check happens in validate_pkcs7_trust(), which
> > appears to have the issue where it can proceed with an empty message,
> > as I mentioned.
> 
> The whole design of validate_pkccs7_trust() is to validate the
> signature only so we can trust the attributes.  It doesn't actually
> verify the digest against the data, that's the job of the
> verify_pkcs7_sig.. class of functions.  The original thought behind
> this was that we might not have the original data by the time we came
> to extract the OID.  However, it turns out we do, so the split of the
> trust functions is not really necessary..

But surely you *do* want to verify the data as well, as that is the
contents of the BPF program?  I think the API you want is something like
verify_pkcs7_signature() extended to return your signed attribute as
extra information on success.

Of course, again it would be a lot simpler if you could find a way to
format all the information you want to sign into one message, instead of
relying on the PKCS#7 message-within-a-message thing that is hard to
understand and implement correctly.

- Eric

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[Eric Biggers]

On Thu, Mar 05, 2026 at 03:18:09PM -0500, James Bottomley wrote:
> On Thu, 2026-03-05 at 10:51 -0800, Eric Biggers wrote:
> > On Thu, Mar 05, 2026 at 09:46:42AM -0500, James Bottomley wrote:
> > > On Wed, 2026-03-04 at 23:55 -0800, Eric Biggers wrote:
> > > > On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley wrote:
> > > > > > If this is for some out-of-tree module, we don't do that.
> > > > > > 
> > > > > > I'll also note that we should generally be aiming to simplify
> > > > > > the PKCS#7 signature verification code, not making it even
> > > > > > more complex.
> > > > > 
> > > > > I'm fine with the general goal, but since the current code
> > > > > verifies the signature, pulls out the message hash and other
> > > > > attributes, compares the message against the MessageDigest one
> > > > > and then frees the whole structure it's a bit hard to see how
> > > > > the current goal can be achieved without extracting at least
> > > > > the first part of that
> > > > > ...
> > > > > but if you have   suggestion, I'm happy to implement.
> > > > 
> > > > Sure, just incorporate your auxiliary data into the actual
> > > > message being signed and verified.  Something like:
> > > >     
> > > >     program_len || program || hash*
> > > 
> > > We can't do that because the second hash is for the LSM.  If
> > > there's no LSM then we need the signature to pass the current eBPF
> > > signature check because the second hash will be verified by the
> > > loader, which means the program hash and nothing else must be in
> > > the messageDigest attr.
> > > 
> > 
> > Why does the loader need to verify the signature if the kernel has to
> > do it anyway, and why does the loader need to skip verifying the
> > maps?
> 
> Well, I didn't say kernel, I said LSM.  The problem is that the last
> hook in the LSM chain for eBPF loading occurs before the loader has
> actually run.  This means that either the LSM needs to be assured
> verification will complete (by running it itself), which is what the
> patch set I pointed to does; or that we need an additional verification
> hook in eBPF somewhere in the verifier after the loader has run, which
> the eBPF people are looking at but haven't actually found anything yet.
> 
> The OID helps the LSM do the additional verification without changing
> any of the eBPF loading flow.

LSMs are part of the kernel.

Not sure I follow.  How can the kernel verify something before it's been
loaded into the kernel?

- Eric

Re: [PATCH v3 3/5] crypto: pkcs7: allow pkcs7_digest() to be called from pkcs7_trust

[James Bottomley]

On Thu, 2026-03-05 at 13:36 -0800, Eric Biggers wrote:
> On Thu, Mar 05, 2026 at 03:11:29PM -0500, James Bottomley wrote:
> > On Thu, 2026-03-05 at 10:50 -0800, Eric Biggers wrote:
> > > On Thu, Mar 05, 2026 at 09:53:56AM -0500, James Bottomley wrote:
> > > > On Wed, 2026-03-04 at 23:58 -0800, Eric Biggers wrote:
> > > > > On Thu, Feb 26, 2026 at 10:50:10PM -0500, James Bottomley
> > > > > wrote:
> > > > > > On Thu, 2026-02-26 at 12:31 -0800, Eric Biggers wrote:
> > > > > > > On Wed, Feb 25, 2026 at 04:19:05PM -0500, James Bottomley
> > > > > > > wrote:
> > > > > > > > +	/*
> > > > > > > > +	 * if we're being called immediately after
> > > > > > > > parse,
> > > > > > > > the
> > > > > > > > +	 * signature won't have a calculated digest
> > > > > > > > yet,
> > > > > > > > so
> > > > > > > > calculate
> > > > > > > > +	 * one.  This function returns immediately if
> > > > > > > > a
> > > > > > > > digest
> > > > > > > > has
> > > > > > > > +	 * already been calculated
> > > > > > > > +	 */
> > > > > > > > +	pkcs7_digest(pkcs7, sinfo);
> > > > > > > 
> > > > > > > pkcs7_digest() can fail, returning an error code and
> > > > > > > leaving
> > > > > > > sig-
> > > > > > > > m
> > > > > > > == NULL && sig->m_size == 0.  Here, the error is just
> > > > > > > being
> > > > > > > ignored.
> > > > > > 
> > > > > > That's right.  Basically I wasn't sure what to return on
> > > > > > error
> > > > > > (although -ENOKEY looks about right since it will cause
> > > > > > retries
> > > > > > on
> > > > > > a
> > > > > > different sig chain).
> > > > > > 
> > > > > > > Doesn't that then cause the signature verification to
> > > > > > > proceed
> > > > > > > against an empty message, rather than anything related to
> > > > > > > the
> > > > > > > data provided?
> > > > > > 
> > > > > > Not if sig->m is NULL, no, because the verifier will try to
> > > > > > reget the digest in that case (and error out if it fails).
> > > > > 
> > > > > Can you point to where that happens?  It still looks like it
> > > > > just
> > > > > proceeds with an empty message.
> > > > 
> > > > It's the obvious one:
> > > > 
> > > > verify_pkcs7_message_sig->pkcs7_verify->pkcs7_verify_one-
> > > > > pkcs7_digest
> > > > 
> > > > The latter will allocate and calculate the digest if sig->m is
> > > > null.
> > > > 
> > > > Regards,
> > > > 
> > > > James
> > > > 
> > > 
> > > But looking at hornet_check_program() from
> > > https://lore.kernel.org/linux-security-module/20251211021257.1208712-9-bboscaccy@linux.microsoft.com/
> > > ,
> > > it calls:
> > > 
> > >     pkcs7_parse_message()
> > >     validate_pkcs7_trust()
> > >     pkcs7_get_authattr()
> > > 
> > > The actual signature check happens in validate_pkcs7_trust(),
> > > which
> > > appears to have the issue where it can proceed with an empty
> > > message,
> > > as I mentioned.
> > 
> > The whole design of validate_pkccs7_trust() is to validate the
> > signature only so we can trust the attributes.  It doesn't actually
> > verify the digest against the data, that's the job of the
> > verify_pkcs7_sig.. class of functions.  The original thought behind
> > this was that we might not have the original data by the time we
> > came
> > to extract the OID.  However, it turns out we do, so the split of
> > the
> > trust functions is not really necessary..
> 
> But surely you *do* want to verify the data as well, as that is the
> contents of the BPF program?  I think the API you want is something
> like verify_pkcs7_signature() extended to return your signed
> attribute as extra information on success.

No, the data has already been verified in eBPF before the hook was
called.  Because of the way the hooks work, that's a guarantee we can
rely on.

> Of course, again it would be a lot simpler if you could find a way to
> format all the information you want to sign into one message, instead
> of relying on the PKCS#7 message-within-a-message thing that is hard
> to understand and implement correctly.

Well, as I said, we have enough information to reverify if required,
but the original verification also has to happen in the way eBPF
designed it.

Regards,

James

Re: [PATCH v3 0/5] pkcs7: better handling of signed attributes

[James Bottomley]

On Thu, 2026-03-05 at 13:40 -0800, Eric Biggers wrote:
> On Thu, Mar 05, 2026 at 03:18:09PM -0500, James Bottomley wrote:
> > On Thu, 2026-03-05 at 10:51 -0800, Eric Biggers wrote:
> > > On Thu, Mar 05, 2026 at 09:46:42AM -0500, James Bottomley wrote:
> > > > On Wed, 2026-03-04 at 23:55 -0800, Eric Biggers wrote:
> > > > > On Thu, Feb 26, 2026 at 07:43:54AM -0500, James Bottomley
> > > > > wrote:
> > > > > > > If this is for some out-of-tree module, we don't do that.
> > > > > > > 
> > > > > > > I'll also note that we should generally be aiming to
> > > > > > > simplify the PKCS#7 signature verification code, not
> > > > > > > making it even more complex.
> > > > > > 
> > > > > > I'm fine with the general goal, but since the current code
> > > > > > verifies the signature, pulls out the message hash and
> > > > > > other attributes, compares the message against the
> > > > > > MessageDigest one and then frees the whole structure it's a
> > > > > > bit hard to see how the current goal can be achieved
> > > > > > without extracting at least the first part of that
> > > > > > ... but if you have   suggestion, I'm happy to implement.
> > > > > 
> > > > > Sure, just incorporate your auxiliary data into the actual
> > > > > message being signed and verified.  Something like:
> > > > >     
> > > > >     program_len || program || hash*
> > > > 
> > > > We can't do that because the second hash is for the LSM.  If
> > > > there's no LSM then we need the signature to pass the current
> > > > eBPF signature check because the second hash will be verified
> > > > by the loader, which means the program hash and nothing else
> > > > must be in the messageDigest attr.
> > > > 
> > > 
> > > Why does the loader need to verify the signature if the kernel
> > > has to do it anyway, and why does the loader need to skip
> > > verifying the maps?
> > 
> > Well, I didn't say kernel, I said LSM.  The problem is that the
> > last hook in the LSM chain for eBPF loading occurs before the
> > loader has actually run.  This means that either the LSM needs to
> > be assured verification will complete (by running it itself), which
> > is what the patch set I pointed to does; or that we need an
> > additional verification hook in eBPF somewhere in the verifier
> > after the loader has run, which the eBPF people are looking at but
> > haven't actually found anything
> > yet.
> > 
> > The OID helps the LSM do the additional verification without
> > changing any of the eBPF loading flow.
> 
> LSMs are part of the kernel.

They are, but they're policy driven.  The policy could be anything
including don't bother calling the LSM.

> Not sure I follow.  How can the kernel verify something before it's
> been loaded into the kernel?

There's a disconnect somewhere but I'm not sure where. The eBPF code is
loaded into the kernel and eBPF will have verified the signature over
the loader component of the light skeleton before the hook is called,
but the loader won't verify the actual program until it runs, which
will happen after the hook is called.

Regards,

James