Make ASN1_STRING opaque and X509 more memory-efficient in 4.x

[davidben]

This is something @bob-beck and I have been talking about recently. We should make ASN1_STRING opaque.

Motivation

X509 object is currently quite inefficient. It needs to allocate small copies of every field in the certificate, which adds up, in both malloc overhead and redundancy with the cached TBSCertificate encoding. Given that X509 has to cache the TBSCertificate encoding anyway, a good implementation would simply alias the data into the underlying buffer.

Unfortunately, OpenSSL's X.509 API does not lend itself to a good implementation here. Imagine if ASN1_STRING had a mode where it did not own str->data and just kept a pointer to some buffer that was maintained elsewhere. We could have all those fields point into the cached encoding.

However, this would require two things:

1. Every time someone tried to modify the ASN1_STRING, we make a copy of the buffer first
2. There is a lifetime contract here. Before the underlying cache is dropped, we must go through and make a copy of all the strings.

The things that in X509 that make this difficult are:

1. Although opaque, much of X509 is not const-correct.
2. ASN1_STRING_data gives mutable access to the underlying buffer, so OpenSSL must allow for callers to scribble over individual fields. That means it can't alias without causing other problems.
3. ASN1_STRING is exposed, so callers can get at the buffers even if ASN1_STRING_data were deprecated

ASN1_STRING changes

Const-correctness is also helpful, as it generally helps you get a handle on your mutations (there's a lot of cached state on OpenSSL's X509 design, which is buggy at best), but I think the most important change here is making ASN1_STRING opaque and removing ASN1_STRING_data in favor of ASN1_STRING_get0_data.

ASN1_STRING_data is already deprecated, so hopefully you can just remove that. Or perhaps make it into an alias for the const-correct ASN1_STRING_get0_data.

To make ASN1_STRING opaque, the missing pieces are:

• No way to set type
• No way to read flags
• No way to set flags

However, I do not think it makes sense to simply expose getters and setters for those. Making flags public allows callers to set ASN1_STRING_FLAG_EMBED at will and break the library. Setting ASN1_STRING_FLAG_NDEF and ASN1_STRING_FLAG_CONT is also questionable. Indeed, if we go through all your flags, I think ASN1_STRING_FLAG_BITS_LEFT is the only one with a public API use case, and that one is extremely difficult to use and not documented anywhere.

Likewise, I don't think it makes sense to set type independent of the other values. If you set type but leave data alone, now you have something in a completely unrelated format. If you set type but leave flags alone, now a non-BIT-STRING ASN1_STRING has unused bits!

New APIs

If we suppose the only useful public API feature of flags is the count of unused bits in a BIT STRING, that suggests a different set of APIs:

First, given an ASN1_STRING, return the number of unused bits. Now, this is a little subtle because OpenSSL has a mode where, if ASN1_STRING_FLAG_BITS_LEFT is clear, the true byte and bit length of the ASN1_STRING is truncated. So we may want an API that returns a tuple of (num_bytes, unused_bits).

Next, to construct ASN1_STRINGs, perhaps, a pair of APIs:

// ASN1_STRING_set1_bit_string sets |str| to contain a BIT STRING (i.e.
// type |V_ASN1_BIT_STRING|) containing |len| bytes from |in|. The last
// |unused_bits| of |in| will be unused. It returns one on success and
// zero on error.
//
// The following are error conditions and will cause the function to fail:
// - |unused_bits| is not between zero and seven.
// - |unused_bits| is non-zero and |len| is zero.
// - The bottom |unused_bits| of |in| are non-zero.
// - |len| is too large to fit in an |ASN1_STRING|.
int ASN1_STRING_set1_bit_string(
    ASN1_BIT_STRING *str, const uint8_t *in, size_t len, int unused_bits);

// ASN1_STRING_set1_value sets |str| to contain a string of type |type|
// and value |len| bytes from |in|. It returns one on success and
// zero on error. If |len| is too large to fit in an |ASN1_STRING|, this
// function will fail and return an error.
//
// This function can only be used to set a whole number of bytes.
// For general BIT STRING support, use |ASN1_STRING_set1_bit_string|.
int ASN1_STRING_set1_value(
    ASN1_STRING *str, int type, const uint8_t *in, size_t len);

Or something in that vein.

Other flags

There are many more flags than ASN1_STRING_FLAG_BITS_LEFT. Going through them here:

ASN1_STRING_FLAG_NDEF and ASN1_STRING_FLAG_CONT seem to be internal book-keeping for the CMS implementation. I cannot tell what they are doing, but I suspect:

1. This was the wrong design
2. If a caller were to mess with them, the library would break

If so, that supports the assumption that they should not be exposed out of the public API.

ASN1_STRING_FLAG_MSTRING is set on MSTRING strings, though not consistently. (You can lose the flag if you X509_set1_notBefore, for example.) It is only queried in X509_time_adj_ex, which toggles whether this function preserves the UTCTime / GeneralizedTime disposition, or implements the X.509 Time choice and switches which it is.

Given that it is possible to lose the flag, I think the above applies: this is not the right design, and even if it were, it should not be possible to toggle it with public API. Rather, "adjust UTCTime", "adjust GeneralizedTime", and "adjust X.509 Time" are distinct operations and should have been distinct functions.

ASN1_STRING_FLAG_EMBED is extremely dangerous for a caller to set. It should not be public API.

ASN1_STRING_FLAG_X509_TIME was added in https://github.com/openssl/openssl/pull/3566 and is a pretty bizarre design. It is only used to control ossl_asn1_time_to_tm behavior, and only set on a temporary stack-allocated ASN1_STRING! This should not have leaked a weird flag into the public API and should instead have just passed a boolean in as an extra parameter to the function.

All this suggests that "flags" should remain an internal aspect of ASN1_STRING and not be exposed publicly.

[bob-beck]

Some discussion with @botovq, and just recording for posterity here.

The setters mutating a non-const ASN1_STRING are fine, and don't get in our way for this purpose (as the reason for doing this would be to always send back const ASN1_STRING's from X509, and know that they can't actually be mutated without breaking the rules and casting away const). Code that is currently directly messing with ->data can just be changed to use the setters which have been available for a while, and therefore, if ASN1_STRING becomes opaque tomorrow, this looks like the code will fail to compile, and then you can just change it to use the setter instead of messing with it directly, and you don't need an #ifdef. As this has been on the radar for a while this shouldn't be a big deal.

As @davidben notes, the lack of a getter/setter for flags means today if you're messing with BIT_STRING and unused bits you will be directly messing with this stuff, and there is no setter/getter in 3.X. What this means is that if we do this in 4.0 this code would require an #ifdef to directly access it in 3.X and use the new api in 4.X. While this is suboptimal, If not a lot of this exists out there, requiring these people to #ifdef may not be a big deal. Will try to find out how extensive this use is out there in the wild.

[davidben]

Ah yeah, OpenSSL's release cycle makes it hard to reliably add the new thing before removing the old thing. Planning for such things happens too infrequently and too late. The ifdef isn't great, but it's hopefully not too bad, as the polyfills aren't that much code.

In Debian, there's a bit to deal with for ASN1_STRING_FLAG_BITS_LEFT, but I think it's tractible: https://codesearch.debian.net/search?q=ASN1_STRING_FLAG_BITS_LEFT+-path%3A%2FCCryptoBoringSSL%2F+-path%3ACryptlib%2FOpenSSL%2F+-pkg%3Aopenssl+-path%3A%2Fboringssl%2F+-pkg%3Aandroid-platform-external-boringssl+-path%3ACryptoPkg%2FLibrary%2FOpensslLib%2F+-path%3Aopenssllib%2Fopenssl_gen%2Fopenssl%2F+-path%3Aopenssl%2Finclude%2Fopenssl+-path%3ACryptlib%2FInclude%2Fopenssl%2F&literal=1

And the new code would be way more understandable. That flag-munging is really horrific. One of the packages even called it a "Weird OpenSSL-ism to get unused bit count", rightly so.

One thing of note, yubico-piv-tool is actually breaking const on the X509 object. In BoringSSL, we added X509_set1_signature_value, X509_REQ_set1_signature_value, and X509_CRL_set1_signature_value APIs. I'd recommend OpenSSL adopt those. They're much more straightforward to use than munging about with ASN1_BIT_STRINGs.

To help with the ifdefs, OpenSSL could consider maintaining a set of compat headers for other projects to vendor in. I talked a bit about this in the 1.1.0 days, but OpenSSL folks at the time tried to push it in a direction that wasn't useful. (It has to be separate thing, vendored into consuming projects. Anything else and the compat headers won't be available where you need them.)

[botovq]

My impression is that people tend not to use the clunky accessors unless they really have to . I suspect one reason is that they're ugly, inconvenient, and they're all weirdly and inconsistently named. I saw some attempts in OpenSSL 3 to add _get_ where it was "missing", but that far from covered all of them and clearly ASN1_STRING wasn't reached by that particular sweep.

As an aside, X509_PUBKEY and X509_ALGOR are another source of rw access to certs (e.g., via X509_get_X509_PUBKEY() and X509_PUBKEY_get0_param()). I've cleaned many of the direct accesses of X509_ALGOR up, so making that opaque won't hurt all that much. Basically you need to investigate everything in the aptly named x509_set.c.

There are at least a few dozen downstream projects that would be impacted by making asn1_string_st opaque. It's not terrible compared to the massive pain inflicted by the last two major OpenSSL updates since the patches are simple and they don't have the tendency of making the code ~3x longer and far less readable like both 1.1 and 3.0 did, but I'd say it's still significant churn. Major projects like curl, Apache's httpd, PHP, Ruby, and Qt would be impacted. Some of them still nominally target 1.0.2, where the string accessors are missing, so the compat layering is going to be doubly inconvenient.

Since ASN1_STRING is the underlying type of most ASN.1 types, you'll have to call ASN1_STRING_type() on an ASN1_INTEGER if you want to know if it's negative (of course to prepend a - sign for printing), or on an ASN1_TIME if you want to konw if it's a UTCTime or a GeneralizedTime. That's pretty ugly.

People want to print bit strings, octet strings, validity times, subject alternative names, serial numbers, and sometimes extension data, so they access the data, length and sometimes type fields. I guess they do that since the printing functions provided by OpenSSL are hard to use (often they need you to set up a BIO) and like the ongoing 00: debate shows, some developers sometimes change the output for what I would call dubious reasons.

Very few projects set things by hand apart from the few projects fiddling with the unused bits for bit strings that you found on Debian's codesearch.

I ran a partial bulk in the OpenBSD ports tree to get an idea of the damage. I've now seen enough since it's not my battle. I think there might easily be 3x-5x this breakage out there. As already mentioned, it's all rather trivial to fix or work around. You can certainly ram this down OpenSSL's consumers' throats since they have little choice but to adapt. As with every breaking change you need to consider if it's really worth it. I for one am really tired of the unpleasant churn in major OpenSSL updates and I think I'm far from alone.

• databases/mongodb data/length/type (SAN)

• databases/openldap data/length (common name)

• databases/pgbouncer data/length/type (validity)

• devel/git data/length (SAN)

• lang/php/8.3 data/length

• lang/python (2.7 and 3.13.9) data/length (extensions/SAN)

• lang/ruby (3.3 and 3.4) bit string fiddling, plus other direct getting

• mail/alpine-c-client data,type,length (SAN, validity)

• mail/fetchmail data,length (SAN)

• mail/imapfilter data/type/length (serial number)

• mail/isync data/length (SAN)

• net/curl ASN1_INTEGER, ASN1_BIT_STRING (serial number, pubkey, signature)

• net/fort ASN1_BIT_STRING (ASN1_STRING_FLAG_BITS_LEFT), data/length/type

• net/gloox ASN1_TIME type (validity)

• net/grpc data/length (SAN)

• net/icinga/core2 data/length (serial number)

• net/libtorrent-rasterbar data/length (SAN)

• net/neon data/type/length (validity)

• net/serf data/length (SAN)

• net/socat ASN1_OCTET_STRING

• net/tg_owt type/data/length (Validity)

• security/stunnel compares bit strings for signatures?

• security/tcltls length/data (serial number)

• security/wpa_supplicant length/data (SAN, common name)

• security/xmlsec length/data/type (validity)

• www/apache-httpd data/length

• www/kore,acme type/data/length

• www/privoxy type/data/length (prints serial number)

• textproc/wkhtml2pdf (embedded qt4), x11/qt5/qtbase, x11/qt6/qtbase data/length (SAN/validity)

[davidben]

Ah yeah, my comment above was specifically about who would need ifdefs for flags APIs that don't yet exist. Whether or not you opaquify it now, those should be added because the existing OpenSSL APIs for it are horrible. (And indeed calculating the number of unused bits is tricky because of the implicit truncation thing OpenSSL does sometimes.)

It's true that type, data, and length will now need accessors. I expect that to be a amount of downstream cleanup. However, those accessors have existed for some time and will not require ifdefs to add. I'm happy to help clean that out by sending projects patches, once we've established the preferred APIs. (Again, this is the problem with OpenSSL's release schedule. The time-frames are huge so we have to plan for long horizons, instead of working incrementally.)

I think this is worth it. The inefficiencies of OpenSSL's X509 type are quite significant. We've saw this in Chrome's user-level performance metrics when we migrated away from them years ago with a parallel API. I would like for those performance wins to be available to all applications, without everyone having to migrate to whole parallel APIs. But for that to happen, non-const access to backing buffers need to go.

[davidben]

I'm happy to help clean that out by sending projects patches, once we've established the preferred APIs.

For that matter, since type, data, and length APIs already exist, I suppose I can start chipping at those now. I've sent patches to CPython, gRPC, and wpa_supplicant before, so I can start with those...

[bob-beck]

@botovq - How many of those are breakages that would require an #ifdef, (I.e. they are BIT_STRING stuff needing flags, etc.). vs how many are simply "we use ->data and ->len, and the code needs a fix to use the accessors.

The point being I would really like to know the difference between

1. Code that today could use accessors that are present in OpenSSL <=3.X / LibreSSL / Boring" but just has not done so yet.
2. Code that needs stuff for which there are not accessors/setters/getters in any current version (i.e. BIT_STRING and flags)

[bob-beck]

I guess we sort of know most of this is accessors. It's just that there's still a lot of it out there.

(Hoping our libre numbers aren't biased too much by the persistent myth that libre is 1.0.2 because that's where we started and so people keep #ifdefing libre to doing it like that)

[botovq]

All these are breakage of the first kind. Breakage of the second kind is covered by the handful of things that @davidben's codesearch found, there may be a few more:

krb5 mariadb qatengine qemu ruby3.3 uncrustify wolfssl wpa yubico-piv-tool

I can take care of the rpki-family: fort-validator rpki-client

[bob-beck]

Yeah, but if we want to do this before OpenSSL has two major releases, we probably need to have a feel for how much of that can be upstreamed and changed reasonably, ideally knowing that the required accessors to at least get rid of the direct ->data and length use are present in modern versions of OpenSSL/LibreSSL/BoringSSL to not require ifdefs for folks compiling only against reasonably recent stuff.

And yes, I think OpenSSL should do major releases much more often, working on tilting at that windmill.

[botovq]

I honestly do not anticipate much pushback. I think helping out with proactively fixing upcoming breakage will be welcome by downstreams.

Diffs are all pretty much along the lines of this quick one I came up with for curl. As I mentioned, one thing that grates is applying ASN1_STRING_foo() to an ASN1_INTEGER or an ASN1_BIT_STRING and the other thing is that some logic will be needed for curl's OpenSSL 1.0.2 support where these accessors are missing (at least according to Ingo's manpages).

--- lib/vtls/openssl.c.orig
+++ lib/vtls/openssl.c
@@ -382,6 +382,7 @@ static CURLcode ossl_certchain(struct Curl_easy *data,
 
   for(i = 0; !result && (i < (int)numcerts); i++) {
     ASN1_INTEGER *num;
+    const unsigned char *numdata;
     X509 *x = sk_X509_value(sk, (ossl_valsize_t)i);
     EVP_PKEY *pubkey = NULL;
     int j;
@@ -403,10 +404,11 @@ static CURLcode ossl_certchain(struct Curl_easy *data,
       break;
 
     num = X509_get_serialNumber(x);
-    if(num->type == V_ASN1_NEG_INTEGER)
+    if(ASN1_STRING_type(num) == V_ASN1_NEG_INTEGER)
       BIO_puts(mem, "-");
-    for(j = 0; j < num->length; j++)
-      BIO_printf(mem, "%02x", num->data[j]);
+    numdata = ASN1_STRING_get0_data(num);
+    for(j = 0; j < ASN1_STRING_length(num); j++)
+      BIO_printf(mem, "%02x", numdata[j]);
     result = push_certinfo(data, mem, "Serial Number", i);
     if(result)
       break;
@@ -604,8 +606,9 @@ static CURLcode ossl_certchain(struct Curl_easy *data,
     }
 
     if(!result && psig) {
-      for(j = 0; j < psig->length; j++)
-        BIO_printf(mem, "%02x:", psig->data[j]);
+      const unsigned char *pdata = ASN1_STRING_get0_data(psig);
+      for(j = 0; j < ASN1_STRING_length(psig); j++)
+        BIO_printf(mem, "%02x:", pdata[j]);
       result = push_certinfo(data, mem, "Signature", i);
     }

[davidben]

Yeah, for better or worse, the fact that ASN1_INTEGER is the same type as ASN1_STRING is thoroughly exposed in the public API.

[bob-beck]

Yeah, for better or worse, the fact that ASN1_INTEGER is the same type as ASN1_STRING is thoroughly exposed in the public API.

And I think if we try to worry about adding more smoke and mirrors type safety to this because it grates on us, we'll just go crazy.. So while it grates on me I put this in the category of "the patience to endure the things I can not change"

[bob-beck]

I think some of these things (like curl) that may possibly support really old OpenSSL versions have not taken such changes, because, well, they don't need to, and if they do, they must either decide to introduce #ifdefs, or decide they aren't going to support compiling with an OpenSSL from 15 or 20 years ago. Today without the object being opaque they don't NEED to do anything.

So many projects may take such changes because they work on everything they care about, some will not if they think they have to continue to support necrophila.

[davidben]

I assume it was less a conscious choice and more that it compiled, so no one noticed. (Curl seems to actually set a pretty high minimum now. https://github.com/curl/curl/pull/18330 and https://github.com/curl/curl/pull/18822)

[botovq]

I modified my version of LibreSSL's libcrypto as follows:

• make asn1_string_st opaque, as discussed here
• remove ASN1_STRING_data()
• add ASN1_BIT_STRING_get_length(): #29184
• add ASN1_BIT_STRING_set1() #29185

I built the full OpenBSD ports tree by adding patches where needed (see the roughly 60 commits listed above, which modify ~200 files in fewer than 100 ports - the vast majority of the changes are completely trivial). I was not exceedingly careful in my patching, but with a little bit of polishing this should be easy to convert into upstreamable changes.

There were three ports I did not fix:

• ettercap does something funky with the "server cert" and a newly created cert. Among other things it writes the magic octets e7:7e to the AKI which the code then looks for elsewher. I was too lazy to figure out why and how to do this nicely with opaque ASN1_STRING.
• ClamAV modifies an ASN1_TIME for a strange conversion that made no sense to me after pondering it for 5 minutes, so I punted on it.
• OpenSC just looked like too much work for an experiment.

The good news is that there were indeed no flag accessess other than the ones in @davidben's codesearch upthread.

There are obviously a few things more that will need fixing since we don't have ports for all the projects. Only very few of the fixes were in LibreSSL-specific paths.

In short: The two APIs in #29184 and #29185 should be enough. I strongly agree that we should avoid flag accessors since no flags other than ASN1_STRING_FLAG_BITS_LEFT are used.

I think with a concerted effort of upstreaming the changes we can remove much of the downstream friction this change will cause.

[bob-beck]

Thanks very much for that tb :)

[vdukhovni]

I'm in favour of having this done in 4.0.

Cc: @mattcaswell , @t8m, @t-j-h

[t8m]

I'm in favour of having this done in 4.0.

Cc: @mattcaswell , @t8m, @t-j-h

+1

[t-j-h]

+1

[davidben]

@botovq Wowwww, thanks for going through all that!

[bob-beck]

• ClamAV modifies an ASN1_TIME for a strange conversion that made no sense to me after pondering it for 5 minutes, so I punted on it.

I tossed something at ClamAV for this. The conversion is a bit strange because they are basically doing stuff to sort of treat an ASN1_TIME value as a local time no matter what it says, but I'll just assume that's correct.

[bob-beck]

Similar for ettercap, just make a copy for the data to fiddle with it, and ASN1_STRING_set.

[bob-beck]

In short: The two APIs in #29184 and #29185 should be enough. I strongly agree that we should avoid flag accessors since no flags other than ASN1_STRING_FLAG_BITS_LEFT are used.

So does this mean you're confirming you don't think we need the API in 29186 for this. (Unless we decide separately from this issue we really want a generic validating setter-of-all-the-things

[botovq]

So does this mean you're confirming you don't think we need the API in https://github.com/openssl/openssl/issues/29186 for this. (Unless we decide separately from this issue we really want a generic validating setter-of-all-the-things

Yes. I found no use for it. I also didn't need to add single patch using ASN1_STRING_new_type().

[davidben]

Ah, interesting. In retrospect, that makes some sense. Outside the library, there's much less use in replacing an ASN1_STRING in-place. You can probably just make a new one. (X509_set1_notBefore vs X509_getm_notBefore.)