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

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Premature termination of signer iteration loop on missing or malformed attributes.
- [Medium] Multi-valued attributes and repeated OIDs silently overwrite each other, returning the last match.
--

commit ee97b7abf11653ef22c6a659603d361105f1df57
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

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

Signers may add any information they like in signed attributes and sometimes
this information turns out to be relevant to specific signing cases. This adds
an api pkcs7_get_authattr() to extract the value of an authenticated attribute
by specific OID.

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

Does this callback return the last matching value instead of the first?

If an attribute contains multiple values (as allowed by the SET OF ANY schema),
or if there are multiple attributes with the same OID, this callback will be
invoked repeatedly, unconditionally overwriting ctx->data.

This seems to result in the last parsed value being returned, which contradicts
the kernel-doc stating it will return the first one in parse order.

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

Can this loop terminate prematurely if a signer has missing or malformed
attributes?

If a verified signer lacks authenticated attributes (sinfo->authattrs == NULL),
asn1_ber_decoder() might return a negative error code. Because the condition
breaks on ret < 0, this will immediately abort the search across all remaining
signers instead of skipping the current one.

This could cause operations like Hornet LSM BPF program verification to fail if
a valid signature is shadowed by an earlier one lacking attributes.

> +	}
> +
> +	if (!ctx.data)
> +		return -ENODATA;
> +
> +	*_data = ctx.data;
> +	*_len = ctx.len;
> +
> +	return 0;
> +}

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