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: <CAKtyLkExV9dqMWa5j9O5n8oTHXh8McwVbjjCm6L9L=eFsH3HNw@mail.gmail.com>
Date: Fri, 12 Sep 2025 14:14:49 -0700
From: Fan Wu <wufan@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: wufan@...nel.org, 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 Fri, Sep 12, 2025 at 6:14 AM Lukas Wunner <lukas@...ner.de> wrote:
>
> 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?
>

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