cmcfarlen commented on code in PR #13482:
URL: https://github.com/apache/trafficserver/pull/13482#discussion_r3707747797
##########
src/tscore/X509HostnameValidator.cc:
##########
@@ -206,12 +206,12 @@ do_check_string(ASN1_STRING *a, int cmp_type, equal_fn
equal, const unsigned cha
{
bool retval = false;
- if (!a->data || !a->length || cmp_type != a->type) {
+ if (!ASN1_STRING_get0_data(a) || !ASN1_STRING_length(a) || cmp_type !=
ASN1_STRING_type(a)) {
return false;
}
- retval = equal(a->data, a->length, b, blen);
+ retval = equal(ASN1_STRING_get0_data(a), ASN1_STRING_length(a), b, blen);
if (retval && peername) {
- *peername = ats_strndup((char *)a->data, a->length);
+ *peername = ats_strndup((char *)ASN1_STRING_get0_data(a),
ASN1_STRING_length(a));
}
Review Comment:
Fixed in 83ef1d27f4. `ats_strndup` is a macro over `_xstrdup(const char *,
int, const char *)`, so the cast to `char *` was discarding const for no reason
— now `reinterpret_cast<const char *>`.
##########
src/iocore/net/unit_tests/test_SSLDHParams.cc:
##########
@@ -132,9 +132,14 @@ make_cert_and_key(EVP_CIPHER const *cipher = nullptr, char
*pass = nullptr)
X509_gmtime_adj(X509_getm_notAfter(x509), 60L * 60L * 24L * 365L);
REQUIRE(X509_set_pubkey(x509, pkey) == 1);
- X509_NAME *name = X509_get_subject_name(x509);
+ // OpenSSL 4.0 returns a pointer to const from X509_get_subject_name(), so
the name
+ // has to be duplicated before it can be modified and stored back.
+ X509_NAME *name = X509_NAME_dup(X509_get_subject_name(x509));
+ REQUIRE(name != nullptr);
X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC,
reinterpret_cast<unsigned char const *>("ats-test"), -1, -1, 0);
+ REQUIRE(X509_set_subject_name(x509, name) == 1);
REQUIRE(X509_set_issuer_name(x509, name) == 1);
+ X509_NAME_free(name);
Review Comment:
Fixed in 83ef1d27f4 — the `X509_NAME_add_entry_by_txt` call is now wrapped
in `REQUIRE`, consistent with the
`X509_set_subject_name`/`X509_set_issuer_name` calls next to it.
##########
plugins/certifier/certifier.cc:
##########
@@ -85,13 +85,21 @@ template <> struct default_delete<SSL_CTX> {
SSL_CTX_free(n);
}
};
+template <> struct default_delete<X509_NAME> {
+ void
+ operator()(X509_NAME *n)
+ {
+ X509_NAME_free(n);
+ }
+};
Review Comment:
Fixed in 83ef1d27f4. Switched to `std::unique_ptr<X509_NAME,
decltype(&X509_NAME_free)>` so no new `std::default_delete` specialization is
added. There is precedent for this form in the codebase already
(`std::unique_ptr<BIO, decltype(&BIO_free)>` in `include/cripts/Certs.hpp`).
The four pre-existing specializations in this file are left alone as out of
scope here.
##########
tests/tools/plugins/ssl_client_verify_test.cc:
##########
@@ -67,10 +69,10 @@ check_names(X509 *cert)
break;
}
- X509_NAME_ENTRY *e = X509_NAME_get_entry(subject, pos);
- ASN1_STRING *cn = X509_NAME_ENTRY_get_data(e);
- char *subj_name = strndup(reinterpret_cast<const char
*>(ASN1_STRING_get0_data(cn)), ASN1_STRING_length(cn));
- retval = check_name(subj_name);
+ auto *e = X509_NAME_get_entry(subject, pos);
+ auto *cn = X509_NAME_ENTRY_get_data(e);
+ char *subj_name = strndup(reinterpret_cast<const char
*>(ASN1_STRING_get0_data(cn)), ASN1_STRING_length(cn));
+ retval = check_name(subj_name);
free(subj_name);
Review Comment:
Fixed in 83ef1d27f4 — `subj_name` is now a `std::string` constructed
directly from the ASN1 pointer and length, dropping `strndup`/`free` and the
null-deref path entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
