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 #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)