Type confusion in OpenSSL's X.400 address handling (CVE-2023-0286)

Two lines from two files in OpenSSL 3.0.7. The first is from include/openssl/x509v3.h.in, part of the public API:

ASN1_TYPE *x400Address;

The second is from crypto/x509/v3_genn.c, the code that actually decodes the thing:

/* Don't decode this */
ASN1_IMP(GENERAL_NAME, d.x400Address, ASN1_SEQUENCE, GEN_X400),

They disagree, and the disagreement is CVE-2023-0286. David Benjamin of Google reported it to OpenSSL on 11 January 2023, and Hugo Landau wrote the fix. It's rated High in the advisory of 7 February 2023.

GeneralName and its union#

X.509 certificates are encoded in DER, which is a binary serialisation of ASN.1. One of the recurring types in the certificate specs is GeneralName: a way of naming something that isn't necessarily a directory name. A DNS name, an email address, an IP address, a URI, or one of a few more exotic options. In ASN.1 that's a CHOICE, which is a tagged union. On the wire, one tag byte tells you which alternative you're looking at, and the contents follow.

GeneralName is everywhere. Subject Alternative Name is a SEQUENCE OF GeneralName. The CRL distribution point extension reaches one level deeper: each distribution point's fullName is a GeneralNames. When your browser decides a certificate is valid for example.com, it's matching against a GeneralName with the DNS tag.

OpenSSL models the whole thing as a C struct with a type field and a union:

ASN1_CHOICE(GENERAL_NAME) = {
        ASN1_IMP(GENERAL_NAME, d.otherName, OTHERNAME, GEN_OTHERNAME),
        ASN1_IMP(GENERAL_NAME, d.rfc822Name, ASN1_IA5STRING, GEN_EMAIL),
        ASN1_IMP(GENERAL_NAME, d.dNSName, ASN1_IA5STRING, GEN_DNS),
        /* Don't decode this */
        ASN1_IMP(GENERAL_NAME, d.x400Address, ASN1_SEQUENCE, GEN_X400),
        /* X509_NAME is a CHOICE type so use EXPLICIT */
        ASN1_EXP(GENERAL_NAME, d.directoryName, X509_NAME, GEN_DIRNAME),
        ASN1_IMP(GENERAL_NAME, d.ediPartyName, EDIPARTYNAME, GEN_EDIPARTY),
        ASN1_IMP(GENERAL_NAME, d.uniformResourceIdentifier, ASN1_IA5STRING, GEN_URI),
        ASN1_IMP(GENERAL_NAME, d.iPAddress, ASN1_OCTET_STRING, GEN_IPADD),
        ASN1_IMP(GENERAL_NAME, d.registeredID, ASN1_OBJECT, GEN_RID)
} ASN1_CHOICE_END(GENERAL_NAME)

That block is a template. OpenSSL's ASN.1 layer is table-driven. You describe the structure once, and generic decoder and encoder machinery walks the description at runtime. Each line says "tag N means: parse the contents as this type and store the result in that union member".

Now look at the fourth line and its comment. x400Address is an X.400 address, from the ITU-T message-handling standards, and you have almost certainly never seen one in a certificate. Nobody wanted to write a decoder for it, so nobody did. ASN1_SEQUENCE as a template item means "don't interpret the contents, just hang on to the encoded bytes". The bytes land in an ASN1_STRING, which is OpenSSL's general-purpose length-and-buffer type.

The header said ASN1_TYPE *. The template produced an ASN1_STRING *. The template was right. The header had been wrong since whenever.

The struct layouts#

Both of these are structs whose first field is an int. That is what makes the mismatch dangerous rather than merely wrong.

struct asn1_string_st {
    int length;
    int type;
    unsigned char *data;
    long flags;
};

struct asn1_type_st {
    int type;
    union {
        char *ptr;
        ASN1_BOOLEAN boolean;
        ASN1_STRING *asn1_string;
        /* ... eighteen more pointer members ... */
    } value;
};

An ASN1_STRING starts with a length, then a type tag, then a data pointer. An ASN1_TYPE starts with a type tag, then (after padding, on any 64-bit ABI) a union of pointers. So if you read an ASN1_STRING through an ASN1_TYPE *, the field you think is type is really length. The pointer you think is value.ptr is really data.

Nothing in OpenSSL ever dereferenced d.x400Address except one function. The public accessors only move the pointer around as a void *, which is presumably why the mismatch survived so long. The relevant arm of GENERAL_NAME_cmp, as it stood:

    switch (a->type) {
    case GEN_X400:
        result = ASN1_TYPE_cmp(a->d.x400Address, b->d.x400Address);
        break;

And here's what ASN1_TYPE_cmp does with it:

int ASN1_TYPE_cmp(const ASN1_TYPE *a, const ASN1_TYPE *b)
{
    int result = -1;

    if (!a || !b || a->type != b->type)
        return -1;

    switch (a->type) {
    /* ... cases for OBJECT, BOOLEAN, NULL, and the string types ... */
    default:
        result = ASN1_STRING_cmp((ASN1_STRING *)a->value.ptr,
                                 (ASN1_STRING *)b->value.ptr);
        break;
    }

    return result;
}

Walk that with the wrong struct underneath. a->type != b->type compares the two lengths, so the attacker just has to make both X.400 addresses the same size. The switch then dispatches on a length. A length that collides with one of the ASN.1 tag constants the switch enumerates lands in one of the other arms. Anything else falls straight through to default.

In default, a->value.ptr reads the bytes at offset 8, which in the real struct is data: a pointer to the raw DER contents of the X.400 address, straight out of the certificate. That gets cast to ASN1_STRING * and handed to ASN1_STRING_cmp, which reads a length and a data pointer out of it and calls memcmp.

So the attacker supplies the bytes that get interpreted as an ASN1_STRING struct. Both the pointer and the length passed to memcmp come out of certificate contents. The advisory puts it this way:

may allow an attacker to pass arbitrary pointers to a memcmp call, enabling them to read memory contents or enact a denial of service.

Getting the comparison to run#

GENERAL_NAME_cmp isn't called during ordinary chain building. In crypto/x509/x509_vfy.c there is one call site, at the bottom of idp_check_dp:

    /* Else case 3: two GENERAL_NAMES */
    for (i = 0; i < sk_GENERAL_NAME_num(a->name.fullname); i++) {
        gena = sk_GENERAL_NAME_value(a->name.fullname, i);
        for (j = 0; j < sk_GENERAL_NAME_num(b->name.fullname); j++) {
            genb = sk_GENERAL_NAME_value(b->name.fullname, j);
            if (GENERAL_NAME_cmp(gena, genb) == 0)
                return 1;
        }
    }

That runs during revocation checking. A certificate can carry a CRL distribution point saying where to fetch the revocation list. A CRL can carry an issuing distribution point saying which certificates it covers. Before OpenSSL will accept a CRL as applying to a certificate, it matches those two names against each other. That means cross-multiplying two attacker-supplied lists of GeneralNames and comparing every pair.

Hence the preconditions in the advisory. The application has to have set X509_V_FLAG_CRL_CHECK, which is not the default. In most cases the attacker has to supply both the chain and the CRL. Neither needs a valid signature, because this comparison happens before anything is verified. The other route in is that whichever input the attacker doesn't control already carries an X.400 address as a CRL distribution point. The advisory describes that as uncommon. Applications that have implemented their own functionality for retrieving CRLs over a network are the ones with a real problem.

The fix#

Four files, 29 insertions, 2 deletions. The two deletions are the ones that matter. ASN1_TYPE *x400Address becomes ASN1_STRING *x400Address in the public header, and the GEN_X400 arm calls ASN1_STRING_cmp instead of ASN1_TYPE_cmp. The regression test added in test/v3nametest.c is two bytes, 0xa3 0x00: an empty X.400 address, tag 3, length zero.

Changing the declared type of a member of a public struct in a patch release is not a thing OpenSSL does lightly, since that's what applications compile against. They did it anyway, which tells you they judged every other option worse. Fixed in 3.0.8, 1.1.1t, and 1.0.2zg for premium support customers.

The line I keep coming back to is /* Don't decode this */. Whoever wrote it was making a reasonable call: X.400 addresses are dead, don't build a parser for a format nobody sends. The decision was even implemented correctly. It just never made it into the public header, and for years there was exactly one function in the codebase in a position to notice.