Re: [PATCH] KEYS: X.509: Fix Basic Constraints CA flag parsing

[Lukas Wunner <lukas@wunner.de>]

On Thu, Sep 11, 2025 at 10:53:56PM +0000, wufan@kernel.org wrote:
> Fix the X.509 Basic Constraints CA flag parsing to correctly handle
> the ASN.1 DER encoded structure. The parser was incorrectly treating
> the length field as the boolean value.
> 
> According to ITU-T X.690 section 8.2, a BOOLEAN is encoded as:
> 
> Tag (0x01), Length (0x01), Value (0x00 for FALSE, non-zero for TRUE)
> 
> The basicConstraints extension with CA:TRUE is encoded as:
> 
>   SEQUENCE (0x30) | Length | BOOLEAN (0x01) | Length (0x01) | Value (0xFF)
>                              ^-- v[2]         ^-- v[3]        ^-- v[4]
> 
> The parser was checking v[3] (the length field, always 0x01) instead
> of v[4] (the actual boolean value, 0xFF for TRUE).

Excellent catch!  How did you find it?

> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -623,7 +625,7 @@ int x509_process_extension(void *context, size_t hdrlen,
>  		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
>  			return -EBADMSG;
>  		if (vlen < 2)
>  			return -EBADMSG;
>  		if (v[1] != vlen - 2)
>  			return -EBADMSG;
> -		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +		if (vlen >= 5 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1 && v[4] != 0)
>  			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_CA;
>  		return 0;
>  	}

Your patch is correct, however the conditions ...

  vlen >= 5 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1

... all check well-formedness of the BasicConstraints object,
so it seems if any of those checks fails, -EBADMSG should be returned.

The check "if (vlen < 2)" could be changed to "if (vlen < 5)" because
5 bytes seems to be the minimum size of a well-formed BasicConstraints
object.  Then the "vlen >= 5" and "v[1] != 0" checks can be dropped.

Up to you whether to respin this patch or make those changes in
a separate patch on top.  And up to Herbert whether to take this
patch as is or wait for a respin.

Reviewed-by: Lukas Wunner <lukas@wunner.de>

I note that parsing the v[] array is quite error-prone and it
might have been better to either declare a packed struct for the
BasicConstraints object with human-readable member names,
or create a separate ASN.1 module for it.

Thanks,

Lukas