[PATCH] crypto: Treat ASN1_STRING as an opaque type
Jouni Malinen
j at w1.fi
Sat Oct 24 01:37:51 PDT 2026
On Thu, Oct 22, 2026 at 10:02:44AM +0000, Xiangfei Ding wrote:
> OpenSSL has published migration guide mentioning that the type is
> turning opaque. One should use accessor functions instead.
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> @@ -2088,6 +2088,8 @@ char * tls_connection_peer_serial_num(void *tls_ctx,
> struct tls_connection *conn)
> {
> ASN1_INTEGER *ser;
> + const unsigned char *ser_data;
> + int ser_len;
> - len = ASN1_STRING_length(ser) * 2 + 1;
> + ser_data = ASN1_STRING_get0_data(ser);
> + ser_len = ASN1_STRING_length(ser);
> + len = (size_t) ser_len * 2 + 1;
> serial_num = os_malloc(len);
> if (!serial_num)
> return NULL;
> - wpa_snprintf_hex_uppercase(serial_num, len,
> - ASN1_STRING_get0_data(ser),
> - ASN1_STRING_length(ser));
> + wpa_snprintf_hex_uppercase(serial_num, len, ser_data, ser_len);
Why are these changes here? Isn't the existing code already using the
accessor functions and this seems to be just restructuring things to
local variables to get rid of one ASN1_STRING_length() call? I don't see
why this would be needed and this should really not be here with the
changes that are actually addressing an interface change.
> @@ -2296,6 +2298,8 @@ static int match_dn_field(const X509 *cert, int nid, const char *field,
> const ASN1_STRING *cn;
> + const unsigned char *cn_data;
> + int cn_len;
> @@ -2311,6 +2315,9 @@ static int match_dn_field(const X509 *cert, int nid, const char *field,
> + cn_data = ASN1_STRING_get0_data(cn);
> + cn_len = ASN1_STRING_length(cn);
> - ret = ASN1_STRING_length(cn) >= len - 1 &&
> - os_memcmp(ASN1_STRING_get0_data(cn), value,
> - len - 1) == 0;
> + ret = (size_t) cn_len >= len - 1 &&
> + os_memcmp(cn_data, value, len - 1) == 0;
> } else {
> /* Compare actual certificate DN field value with
> * configuration DN field value */
> - ret = ASN1_STRING_length(cn) == len &&
> - os_memcmp(ASN1_STRING_get0_data(cn), value,
> - len) == 0;
> + ret = (size_t) cn_len == len && os_memcmp(cn_data, value, len) == 0;
Aren't these the exact same thing in practice as well?
> wpa_printf(MSG_ERROR,
> - "OpenSSL: Failed to match %s '%s' with certificate DN field value '%s'",
> - field, value, ASN1_STRING_get0_data(cn));
> + "OpenSSL: Failed to match %s '%s' with certificate DN field"
> + " value '%.*s'",
> + field, value, cn_len, (const char *) cn_data);
So here is an actual change that was almost hidden by all the not needed
refactoring.. It would be much better to have a separate patch that
covers the not-NUL-terminated issue without doing any other refactoring
of the code.
> @@ -2458,15 +2463,22 @@ static int tls_match_suffix_helper(X509 *cert, const char *match,
> + const unsigned char *dns_data;
> + int dns_len;
> gen = sk_GENERAL_NAME_value(ext, j);
> - if (gen->type != GEN_DNS)
> + if (gen->type != GEN_DNS || !gen->d.dNSName)
> + continue;
> +
> + dns_data = ASN1_STRING_get0_data(gen->d.dNSName);
> + dns_len = ASN1_STRING_length(gen->d.dNSName);
> + if (!dns_data)
> continue;
> +
> dns_name++;
> wpa_hexdump_ascii(MSG_DEBUG, "TLS: Certificate dNSName",
> - ASN1_STRING_get0_data(gen->d.dNSName),
> - ASN1_STRING_length(gen->d.dNSName));
> - if (domain_suffix_match(ASN1_STRING_get0_data(gen->d.dNSName),
> - ASN1_STRING_length(gen->d.dNSName),
> + dns_data, dns_len);
> + if (domain_suffix_match(dns_data, dns_len,
> match, match_len, full) == 1) {
What is this for? This seems like restructuring without any actual
change, but there might be a change to verify that gen->d.dNSName is not
NULL even if gen->type == GEN_DNS. Is that needed? It should be done in
a separate patch so that it is not hidden with moving existing accessor
function use to using local variables (which does not really seem
necessary in most of the cases in the patch).
> @@ -2486,6 +2498,8 @@ static int tls_match_suffix_helper(X509 *cert, const char *match,
> const ASN1_STRING *cn;
> + const unsigned char *cn_data;
> + int cn_len;
> + cn_data = ASN1_STRING_get0_data(cn);
> + cn_len = ASN1_STRING_length(cn);
> wpa_hexdump_ascii(MSG_DEBUG, "TLS: Certificate commonName",
> - ASN1_STRING_get0_data(cn),
> - ASN1_STRING_length(cn));
> - if (domain_suffix_match(ASN1_STRING_get0_data(cn),
> - ASN1_STRING_length(cn),
> + cn_data, cn_len);
> + if (domain_suffix_match(cn_data, cn_len,
> match, match_len, full) == 1) {
Same here.. Nothing seems to really change here as far as OpenSSL API
use is concerned.
> @@ -2701,6 +2715,8 @@ static void openssl_tls_cert_event(struct tls_connection *conn,
> + const unsigned char *ia5_data;
> + int ia5_len;
> - pos = os_malloc(10 + ASN1_STRING_length(gen->d.ia5) + 1);
> + if (!gen->d.ia5)
> + continue;
> +
> + ia5_data = ASN1_STRING_get0_data(gen->d.ia5);
> + ia5_len = ASN1_STRING_length(gen->d.ia5);
> + if (!ia5_data)
> + continue;
> +
> + pos = os_malloc(10 + ia5_len + 1);
Isn't this all same as well for API use?
> @@ -6063,11 +6086,12 @@ static void openssl_debug_dump_certificate(int i, X509 *cert)
> ser = X509_get_serialNumber(cert);
> - if (ser)
> + if (ser) {
> + const unsigned char *ser_data = ASN1_STRING_get0_data(ser);
> + int ser_len = ASN1_STRING_length(ser);
> wpa_snprintf_hex_uppercase(serial_num, sizeof(serial_num),
> - ASN1_STRING_get0_data(ser),
> - ASN1_STRING_length(ser));
> - else
> + ser_data, ser_len);
Same here..
> diff --git a/src/crypto/tls_openssl_ocsp.c b/src/crypto/tls_openssl_ocsp.c
> @@ -540,7 +545,7 @@ enum ocsp_result check_ocsp_resp(SSL_CTX *ssl_ctx, SSL *ssl, X509 *cert,
> - basic_data = ASN1_STRING_data(bytes->response);
> + basic_data = ASN1_STRING_get0_data(bytes->response);
> basic_len = ASN1_STRING_length(bytes->response);
> wpa_hexdump(MSG_DEBUG, "OpenSSL: BasicOCSPResponse",
> basic_data, basic_len);
And this is somewhat quite different, i.e., nothing to do with the other
changes and the justification in the commit message. This is just
replacing a deprecated accessor function with the newer one for doing
same.
So in conclusion, I would like to see this split into three patches:
- Fix incorrect NUL-terminated-string-assumption
- Start using accessor functions for cases that accessed member
variables directly (and doing this without unneeded restructuring
with local variables)
- ASN1_STRING_data -> ASN1_STRING_get0_data
And if I missed some real need for the restructuring changes with local
variables, that would be a fourth patch.
--
Jouni Malinen PGP id EFC895FA
More information about the Hostap
mailing list