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

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

[wufan]

From: Fan Wu <wufan@kernel.org>

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).

Per ITU-T X.690-02/2021 section 8.2.2:
"If the boolean value is TRUE, the octet shall have any non-zero
value, as a sender's option."

Most implementations, including OpenSSL, encode TRUE as 0xFF.

Link: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
Fixes: 30eae2b037af ("KEYS: X.509: Parse Basic Constraints for CA")
Signed-off-by: Fan Wu <wufan@kernel.org>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 2ffe4ae90bea..4dfec6c45772 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -613,8 +613,10 @@ int x509_process_extension(void *context, size_t hdrlen,
 		 *	(Expect 0x2 or greater, making it 1 or more bytes)
 		 * v[2] is the encoding type
 		 *	(Expect an ASN1_BOOL for the CA)
-		 * v[3] is the contents of the ASN1_BOOL
-		 *      (Expect 1 if the CA is TRUE)
+		 * v[3] is the length of the ASN1_BOOL
+		 *	(Expect 1 for a single byte boolean)
+		 * v[4] is the contents of the ASN1_BOOL
+		 *	(Expect non-zero if the CA is TRUE, typically 0xFF)
 		 * vlen should match the entire extension size
 		 */
 		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
@@ -623,7 +625,7 @@ int x509_process_extension(void *context, size_t hdrlen,
 			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;
 	}
-- 
2.50.1

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

[Lukas Wunner]

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

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

[Fan Wu]

On Fri, Sep 12, 2025 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> 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?
>

Unrelated context, I was exploring the possibility of adding Extended
Key Usage (EKU) support and noticed this code didn't look quite right.

> > +++ 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.
>

Actually, we need to be careful here. OpenSSL produces
BasicConstraints with CA:FALSE as just an empty SEQUENCE:

06 03 55 1d 13 | 01 01 ff | 04 02 | 30 00
[----OID------] [critical] [OCTET] [empty SEQ]

-Fan

> 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
>

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

[Lukas Wunner]

On Fri, Sep 12, 2025 at 02:14:49PM -0700, Fan Wu wrote:
> On Fri, Sep 12, 2025 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote:
> > On Thu, Sep 11, 2025 at 10:53:56PM +0000, wufan@kernel.org wrote:
> > > +++ 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.
> 
> Actually, we need to be careful here. OpenSSL produces
> BasicConstraints with CA:FALSE as just an empty SEQUENCE:
> 
> 06 03 55 1d 13 | 01 01 ff | 04 02 | 30 00
> [----OID------] [critical] [OCTET] [empty SEQ]

I see, thanks for the explanation.

This behavior of OpenSSL doesn't seem spec-compliant, or is it?
RFC 5280 sec 4.2.1.9 says the pathLenConstraint is optional,
but the cA boolean is not optional.  Is there a rule that booleans
need not be rendered if they are false?

BTW, I note that X.690 sec 11.1 says that for DER encoding,
all bits of a "true" boolean must be set, hence the 0xff value.
But I'm fine with your more permissive approach which checks for
a non-zero value, hence also allows BER encoding per X.690 sec 8.2.2.

Thanks!

Lukas

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

[Fan Wu]

On Fri, Sep 12, 2025 at 9:38 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Fri, Sep 12, 2025 at 02:14:49PM -0700, Fan Wu wrote:
> > On Fri, Sep 12, 2025 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Sep 11, 2025 at 10:53:56PM +0000, wufan@kernel.org wrote:
> > > > +++ 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.
> >
> > Actually, we need to be careful here. OpenSSL produces
> > BasicConstraints with CA:FALSE as just an empty SEQUENCE:
> >
> > 06 03 55 1d 13 | 01 01 ff | 04 02 | 30 00
> > [----OID------] [critical] [OCTET] [empty SEQ]
>
> I see, thanks for the explanation.
>
> This behavior of OpenSSL doesn't seem spec-compliant, or is it?
> RFC 5280 sec 4.2.1.9 says the pathLenConstraint is optional,
> but the cA boolean is not optional.  Is there a rule that booleans
> need not be rendered if they are false?
>
> BTW, I note that X.690 sec 11.1 says that for DER encoding,
> all bits of a "true" boolean must be set, hence the 0xff value.
> But I'm fine with your more permissive approach which checks for
> a non-zero value, hence also allows BER encoding per X.690 sec 8.2.2.
>
> Thanks!
>
> Lukas

I double-checked RFC 5280 and X.690 after your comment. I found RFC
5280 section 4.1 explicitly requires X.509 certificates to use DER
encoding, so my original patch allowing any non-zero value was
incorrect. I'll update the patch to check specifically for 0xFF and
return -EBADMSG for any other value.

For OpenSSL's empty SEQUENCE, X.690 section 11.5 states: "The encoding
of a set value or sequence value shall not include an encoding for any
component value which is equal to its default value." Since RFC 5280
defines BasicConstraints with "cA BOOLEAN DEFAULT FALSE", the field
must be omitted when false under DER encoding. So OpenSSL's behavior
is correct.

Thanks for catching the boolean encoding issue.

-Fan

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

[wufan]

From: Fan Wu <wufan@kernel.org>

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.

Per RFC 5280 section 4.1, X.509 certificates must use ASN.1 DER encoding.
According to ITU-T X.690, a DER-encoded BOOLEAN is represented as:

Tag (0x01), Length (0x01), Value (0x00 for FALSE, 0xFF 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 in DER encoding).

Also handle the case where the extension is an empty SEQUENCE (30 00),
which is valid for CA:FALSE when the default value is omitted as
required by DER encoding rules (X.690 section 11.5).

Per ITU-T X.690-0207:
- Section 11.5: Default values must be omitted in DER
- Section 11.1: DER requires TRUE to be encoded as 0xFF

Link: https://datatracker.ietf.org/doc/html/rfc5280
Link: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
Fixes: 30eae2b037af ("KEYS: X.509: Parse Basic Constraints for CA")
Signed-off-by: Fan Wu <wufan@kernel.org>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 2ffe4ae90bea..8df3fa60a44f 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -610,11 +610,14 @@ int x509_process_extension(void *context, size_t hdrlen,
 		/*
 		 * Get hold of the basicConstraints
 		 * v[1] is the encoding size
-		 *	(Expect 0x2 or greater, making it 1 or more bytes)
+		 *	(Expect 0x00 for empty SEQUENCE with CA:FALSE, or
+		 *	0x03 or greater for non-empty SEQUENCE)
 		 * v[2] is the encoding type
 		 *	(Expect an ASN1_BOOL for the CA)
-		 * v[3] is the contents of the ASN1_BOOL
-		 *      (Expect 1 if the CA is TRUE)
+		 * v[3] is the length of the ASN1_BOOL
+		 *	(Expect 1 for a single byte boolean)
+		 * v[4] is the contents of the ASN1_BOOL
+		 *	(Expect 0xFF if the CA is TRUE)
 		 * vlen should match the entire extension size
 		 */
 		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
@@ -623,8 +626,13 @@ int x509_process_extension(void *context, size_t hdrlen,
 			return -EBADMSG;
 		if (v[1] != vlen - 2)
 			return -EBADMSG;
-		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+		/* Empty SEQUENCE means CA:FALSE (default value omitted per DER) */
+		if (v[1] == 0)
+			return 0;
+		if (vlen >= 5 && v[2] == ASN1_BOOL && v[3] == 1 && v[4] == 0xFF)
 			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_CA;
+		else
+			return -EBADMSG;
 		return 0;
 	}
 
-- 
2.51.0

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

[Lukas Wunner]

On Mon, Sep 15, 2025 at 09:15:50PM +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.
[...]
> Link: https://datatracker.ietf.org/doc/html/rfc5280
> Link: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
> Fixes: 30eae2b037af ("KEYS: X.509: Parse Basic Constraints for CA")
> Signed-off-by: Fan Wu <wufan@kernel.org>

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

> @@ -623,8 +626,13 @@ int x509_process_extension(void *context, size_t hdrlen,
>  			return -EBADMSG;
>  		if (v[1] != vlen - 2)
>  			return -EBADMSG;
> -		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +		/* Empty SEQUENCE means CA:FALSE (default value omitted per DER) */
> +		if (v[1] == 0)
> +			return 0;
> +		if (vlen >= 5 && v[2] == ASN1_BOOL && v[3] == 1 && v[4] == 0xFF)
>  			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_CA;
> +		else
> +			return -EBADMSG;
>  		return 0;
>  	}

For anyone who didn't follow the discussion and/or isn't familiar
with ASN.1, the patch first checks for a well-formed "CA:FALSE",
then checks for a well-formed "CA:TRUE", finally rejects anything
else as malformed.

Thanks,

Lukas

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

[Herbert Xu]

On Mon, Sep 15, 2025 at 09:15:50PM +0000, wufan@kernel.org wrote:
> From: Fan Wu <wufan@kernel.org>
> 
> 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.
> 
> Per RFC 5280 section 4.1, X.509 certificates must use ASN.1 DER encoding.
> According to ITU-T X.690, a DER-encoded BOOLEAN is represented as:
> 
> Tag (0x01), Length (0x01), Value (0x00 for FALSE, 0xFF 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 in DER encoding).
> 
> Also handle the case where the extension is an empty SEQUENCE (30 00),
> which is valid for CA:FALSE when the default value is omitted as
> required by DER encoding rules (X.690 section 11.5).
> 
> Per ITU-T X.690-0207:
> - Section 11.5: Default values must be omitted in DER
> - Section 11.1: DER requires TRUE to be encoded as 0xFF
> 
> Link: https://datatracker.ietf.org/doc/html/rfc5280
> Link: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
> Fixes: 30eae2b037af ("KEYS: X.509: Parse Basic Constraints for CA")
> Signed-off-by: Fan Wu <wufan@kernel.org>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt