Copilot commented on code in PR #13476:
URL: https://github.com/apache/trafficserver/pull/13476#discussion_r3708332610
##########
src/tscore/X509HostnameValidator.cc:
##########
@@ -269,13 +268,13 @@ validate_hostname(X509 *x, std::string_view hostname,
bool is_ip, char **peernam
}
}
// No SAN match -- check the subject
- i = -1;
- name = X509_get_subject_name(x);
+ i = -1;
+ auto *name = X509_get_subject_name(x);
while ((i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) >= 0) {
- ASN1_STRING *str;
- int astrlen;
- unsigned char *astr;
+ const ASN1_STRING *str;
+ int astrlen;
+ unsigned char *astr;
str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i));
// Convert to UTF-8
astrlen = ASN1_STRING_to_UTF8(&astr, str);
Review Comment:
`validate_hostname()` returns `-1` on `ASN1_STRING_to_UTF8()` failure, but
the function's return type is `bool`. Returning `-1` will convert to `true`,
incorrectly treating conversion failures as hostname matches.
##########
src/api/InkAPI.cc:
##########
@@ -8316,9 +8316,9 @@ 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);
+ const int pos =
X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName,
-1);
+ const X509_NAME_ENTRY *common_name =
X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos);
+ const 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)));
if (ASN1_STRING_length(common_name_asn1) !=
static_cast<int>(strlen(common_name_str))) {
Review Comment:
This CN extraction does not handle missing/invalid commonName (`pos < 0`,
null entry/data) and calls `strlen()` on `ASN1_STRING_get0_data()`, which is
not guaranteed to be NUL-terminated. That can cause out-of-bounds reads and
crashes when updating server certs.
--
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]
