On Fri, Sep 12, 2025 at 6:14 AM Lukas Wunner <lukas@xxxxxxxxx> wrote: > > On Thu, Sep 11, 2025 at 10:53:56PM +0000, wufan@xxxxxxxxxx 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@xxxxxxxxx> > > 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 >
