Re: [v6 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 skips valid signers when a prior signer lacks authenticated attributes or contains malformed data.
- [Medium] Parser differential risk due to blindly overwriting multiple attribute values in `pkcs7_aa_note_attr`.
--

commit 06f8e69e2d93f6dbe1e1035741251e357d0c5e30
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
crypto: pkcs7: add ability to extract signed attributes by OID

This commit adds an API to extract authenticated attributes from a PKCS#7
message by OID. It introduces a new ASN.1 grammar and a set of callbacks to
parse the attributes for verified signers.

> 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

[ ... ]

> +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 create a parser differential risk?

The ASN.1 definition uses a set of ANY for authenticated attributes. If a
single attribute contains multiple values, this callback will be triggered
for each element.

Since the callback overwrites ctx->data without checking if a value was already
found, it will silently return only the last value in the set. Could an
attacker craft a signature with multiple conflicting values, causing Linux
to extract and enforce a policy based on a different payload than what other
tools verify?

[ ... ]

> +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 exit prematurely when a prior signer lacks authenticated
attributes or contains malformed data?

If a verified PKCS#7 signer has no authenticated attributes, sinfo->authattrs
is NULL and sinfo->authattrs_len is 0. Calling asn1_ber_decoder() with a
0-length buffer triggers a data overrun check and returns a negative error code:

lib/asn1_decoder.c:asn1_ber_decoder() {
    ...
    if (unlikely(datalen - dp < 2))
        goto data_overrun_error;
    ...
}

Because of the break statement on a negative return value, this aborts the
search entirely and returns -ENODATA.

Does this prevent the parser from checking any subsequent verified signers that
might actually contain the requested attribute? Should this skip empty
attributes and use continue instead of break?

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

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