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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMQcnoETIt4t4Tqz@wunner.de>
Date: Fri, 12 Sep 2025 15:14:06 +0200
From: Lukas Wunner <lukas@...ner.de>
To: wufan@...nel.org
Cc: dhowells@...hat.com, ignat@...udflare.com, herbert@...dor.apana.org.au,
	davem@...emloft.net, jarkko@...nel.org, zohar@...ux.ibm.com,
	eric.snowberg@...cle.com, keyrings@...r.kernel.org,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KEYS: X.509: Fix Basic Constraints CA flag parsing

On Thu, Sep 11, 2025 at 10:53:56PM +0000, wufan@...nel.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@...ner.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