Re: [PATCH v5 02/10] crypto: pkcs7: add ability to extract signed attributes by OID

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] API Flaw / Data Corruption with Constructed Types (`SET OF ANY` behavior)
- [High] Early Loop Termination on Missing Authenticated Attributes
- [High] Multi-value Overwrite / Parser Differential
- [Medium] Genuine ASN.1 Decoding Errors are Masked
--

commit 1ec29c2985cc84f3d3e1912ff4626eaa51d31e0c
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

crypto: pkcs7: add ability to extract signed attributes by OID

This patch adds an API to extract the value of an authenticated attribute
by a specific OID. It introduces a new ASN.1 grammar and decoder to parse
the PKCS#7 signed attributes.

> diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
> index 6e3ffdac83ace..d467866f7d930 100644
> --- a/crypto/asymmetric_keys/pkcs7_parser.c
> +++ b/crypto/asymmetric_keys/pkcs7_parser.c

[ ... ]

> @@ -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;
> +	}

Since the ASN.1 SET OF ANY grammar allows constructed types, does
asn1_ber_decoder() descend into the constructed type and repeatedly call this
action for each nested primitive? If so, this would overwrite ctx->data with
the last inner leaf element rather than capturing the outer bounds.

Also, if multiple attributes with the same OID exist, wouldn't this overwrite
ctx->data with the last matching attribute instead of returning the first one
as the documentation states?

> +
> +	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 a verified signer omits authenticated attributes, sinfo->authattrs is
NULL and sinfo->authattrs_len is 0. Does passing a 0-length buffer to
asn1_ber_decoder() cause it to return an error?

If so, the loop will break immediately below, meaning we could miss attributes
from subsequent valid signers in the message. Should there be an explicit
continue if !sinfo->authattrs?

> +		if (ret < 0 || ctx.data != NULL)
> +			break;

If asn1_ber_decoder() returns an error due to the constructed type
misalignment, does checking ctx.data != NULL here mask that error and
allow the function to successfully return a corrupted inner pointer?

> +	}
> +
> +	if (!ctx.data)
> +		return -ENODATA;

If asn1_ber_decoder() fails with a genuine structural error like -EBADMSG,
the loop breaks but we unconditionally return -ENODATA here. Does this
incorrectly mask malformed data errors from the caller?

> +
> +	*_data = ctx.data;
> +	*_len = ctx.len;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pkcs7_get_authattr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420212653.438685-1-bboscaccy@linux.microsoft.com?part=2