crypto/asymmetric_keys/x509_cert_parser.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
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.1On 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
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 >
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
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
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.0On 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
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
© 2016 - 2025 Red Hat, Inc.