Copilot commented on code in PR #13482:
URL: https://github.com/apache/trafficserver/pull/13482#discussion_r3707299209
##########
src/api/InkAPI.cc:
##########
@@ -8316,10 +8316,10 @@ TSSslServerCertUpdate(const char *cert_path, const char
*key_path)
}
// Extract common name
- int pos =
X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName,
-1);
- X509_NAME_ENTRY *common_name =
X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos);
- ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name);
- char *common_name_str = reinterpret_cast<char *>(const_cast<unsigned char
*>(ASN1_STRING_get0_data(common_name_asn1)));
+ int pos =
X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName,
-1);
+ auto *common_name =
X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos);
+ auto *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name);
+ char *common_name_str = reinterpret_cast<char *>(const_cast<unsigned char
*>(ASN1_STRING_get0_data(common_name_asn1)));
if (ASN1_STRING_length(common_name_asn1) !=
static_cast<int>(strlen(common_name_str))) {
Review Comment:
Extracting the certificate common name via ASN1_STRING_get0_data() and then
calling strlen() is unsafe because the OpenSSL buffer is not guaranteed to be
NUL-terminated. Also, if the cert has no CN, pos will be -1 and
common_name/common_name_asn1 can be null, leading to a crash. Consider
validating the index/pointers and using the explicit ASN1 length (plus an
embedded-NUL check) before converting to a std::string for lookup/logging.
##########
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:
This test mutates the duplicated X509_NAME but does not assert that
X509_NAME_add_entry_by_txt() succeeded. If it fails, the rest of the test may
proceed with an unexpected subject/issuer, reducing test signal or causing
confusing failures later.
##########
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:
ats_strndup() takes a const char*. Casting ASN1_STRING_get0_data() to char*
discards const and signedness; this can trigger warnings and is unnecessary
since ats_strndup copies the buffer.
##########
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:
check_name() takes a std::string; calling it with the char* returned by
strndup() will crash/UB if strndup() returns nullptr (allocation failure).
Since you already have a pointer+length from OpenSSL, you can avoid
strndup/free entirely and construct a std::string directly from the ASN1 buffer.
##########
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:
Adding a new specialization of std::default_delete for an OpenSSL library
type (X509_NAME) further extends undefined behavior (specializing standard
library templates for non-user-defined types). It would be safer to keep the
unique_ptr alias for X509_NAME using an explicit custom deleter instead of
specializing in namespace std.
--
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]
