Skip to content

Fix unsafe certificate common name extraction #13584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RajaMuhammadAwais wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix unsafe certificate common name extraction #13584

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2300d5a
Fix unsafe certificate common name extraction
Aug 25, 2026
dde7e2b
Fix cert update regression test assertions
Aug 27, 2026
a901970
Improve certificate update diagnostics
Aug 27, 2026
9a9e328
Avoid error logging for invalid certificate CN
Aug 27, 2026
fecbe50
Use inferred type for OpenSSL certificate entry data
Aug 27, 2026
1421f8b
Use inferred type for OpenSSL subject name
Aug 28, 2026
79120a6
Merge upstream master to resolve cert update test conflict
Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions src/api/InkAPI.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8395,17 +8395,38 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path)
return TS_ERROR;
}

// Extract 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))) {
// Extract common name. X509_NAME_get_index_by_NID() returns -1 when the
// certificate has no commonName, and ASN1_STRING_get0_data() does not
// guarantee NUL termination. Check both conditions before using the data.
const auto *subject = X509_get_subject_name(cert.get());

@maskit maskit Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to let auto decide whether it's const or not. Just auto *subject = .

const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1);
if (pos < 0) {

@bneradt bneradt Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: the three new return TS_ERROR paths are silent. The load failure a few lines up uses SSLError("Failed to load certificate/key from %s", cert_path), so an operator who feeds in a CN-less cert gets no diagnostic at all — the only trace is the calling plugin's own Dbg(), which needs its debug tag enabled. A single Dbg(dbg_ctl_ssl_cert_update, ...) or SSLError() naming the reason would make this much easier to support in the field.

Dbg(dbg_ctl_ssl_cert_update, "Failed to extract common name from certificate %s: no commonName", cert_path);
return TS_ERROR;
}

const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(subject, pos);
const auto *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name);
if (!common_name_asn1) {
Dbg(dbg_ctl_ssl_cert_update, "Failed to extract common name from certificate %s: missing ASN.1 value", cert_path);
return TS_ERROR;
}

const auto *common_name_data = ASN1_STRING_get0_data(common_name_asn1);
const int common_name_len = ASN1_STRING_length(common_name_asn1);
if (!common_name_data || common_name_len <= 0) {
Dbg(dbg_ctl_ssl_cert_update, "Failed to extract common name from certificate %s: invalid ASN.1 value", cert_path);
return TS_ERROR;
}

const std::string common_name_str{reinterpret_cast<const char *>(common_name_data), static_cast<size_t>(common_name_len)};
if (common_name_str.find('\0') != std::string::npos) {
// Embedded null char
return TS_ERROR;
}
Dbg(dbg_ctl_ssl_cert_update, "Updating from %s with common name %s", cert_path, common_name_str);

Dbg(dbg_ctl_ssl_cert_update, "Updating from %s with common name %.*s", cert_path, static_cast<int>(common_name_str.size()),
common_name_str.data());
// Update context to use cert
cc = lookup->find(common_name_str);
if (cc && cc->getCtx()) {
Expand Down