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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Premature loop termination if a verified signer lacks authenticated attributes (`sinfo->authattrs == NULL`).
- [Medium] The function returns the last matching attribute value rather than the first one, contradicting its API documentation.
--

commit 8f64855e85aaa27bba380d666827191e50cceb99
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

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

This commit adds an API, pkcs7_get_authattr(), to extract the value of an
authenticated attribute by a specific OID. It parses the signed attributes
and is intended to return the first matching value.

> 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,
[ ... ]
> +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;
> +}

The documentation for pkcs7_get_authattr() mentions it returns the first match
in parse order.

If an authenticated attribute contains multiple values within its SET OF ANY,
or if multiple attributes share the same OID, won't this callback continuously
overwrite ctx->data and return the last matching value instead of the first
one?

Could we enforce capturing only the first value by checking if ctx->data is
already set before assigning it?

[ ... ]
> +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;
> +	}

PKCS#7 allows authenticated attributes to be optional, meaning sinfo->authattrs
could legitimately be NULL with a length of 0.

If a verified signer has no authenticated attributes, asn1_ber_decoder() will
be called with datalen == 0 and fail with a data overrun error, returning a
negative value.

Because the loop breaks on ret < 0, won't this permanently abort the search
across all subsequent signers instead of skipping to the next one?

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