Do not assume that ASN1_STRING is null terminated.

[divVerent]

The OpenSSL docs say:

In general it cannot be assumed that the data returned by
ASN1_STRING_data() is null terminated or does not contain embedded
nulls. The actual format of the data will depend on the actual string
type itself: for example for an IA5String the data will be ASCII, for a
BMPString two bytes per character in big endian format, and for a
UTF8String it will be in UTF8 format.

This ASN1 timestamp function was the only remaining function in pyOpenSSL that assumed null termination.

[alex]

is it possible to add a test case?

[divVerent]

is it possible to add a test case?

Good question; right now I do not know how to best test this.

The only "official" way to create such a string right now (before the planned change to BoringSSL that will make all strings like this) is using ASN1_STRING_set0. I'll see if it's possible to use that.

[alex]

Is it possible to construct this scenario with our public APIs?

[divVerent]

It is not possible to use public APIs to construct this scenario, as pyOpenSSL nowhere calls ASN1_STRING_set0.

Both BoringSSL and OpenSSL have been holding off from ever making a non null terminated ASN1_STRING because of users like Python that keep relying on this (even though documentation says not to).

Relevant comments:

BoringSSL crypto/asn1/asn1_lib.cc:

  if (data != nullptr) {
    OPENSSL_memcpy(str->data, data, len);
    // Historically, OpenSSL would NUL-terminate most (but not all)
    // `ASN1_STRING`s, in case anyone accidentally passed `str->data` into a
    // function expecting a C string. We retain this behavior for compatibility,
    // but code must not rely on this. See CVE-2021-3712.
    str->data[len] = '\0';
  }

OpenSSL has in the same place:

    if (data != NULL && str->data != NULL) {
        memcpy(str->data, data, len);
        if (add_nul_byte) {
            /*
             * Add a '\0' terminator. This should not be necessary - but we add it as
             * a safety precaution
             */
            str->data[len] = '\0';
        }
    }

Here's my problem now: this sounds like the pyOpenSSL code is already broken right now against current OpenSSL:

• add_nul_byte is not set when coming from ASN1_STRING_set1_data.
• ossl_asn1_time_from_tm calls ASN1_STRING_set1_data.
• ASN1_TIME_to_generalizedtime calls ossl_asn1_time_from_tm.
• Your _get_asn1_time calls ASN1_TIME_to_generalizedtime.

As such, I strongly suspect that pyOpenSSL is already broken with existing tests (assuming we at least sometimes have an ASN1_TIME, not ASN1_GENERALIZEDTIME). To expose this though, it might be possible that both OpenSSL and CPython need to be compiled against AddressSanitizer.

[alex]

Gotcha, so this is correctness/hardening, but not bug fix.

[divVerent]

Thanks!

[divVerent]

Oh, and the reason why this wasn't hit yet: OpenSSL only moved to not null terminating all ASN1_STRINGs in commit 28179061bfcdfe579f63a129bf0b97766a5d90a7, which is in no released version yet.