From 2300d5a7512cdf81c95d6a42ec87d09f7c7b9109 Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:26:28 +0000 Subject: [PATCH 1/6] Fix unsafe certificate common name extraction Guard missing common names and use ASN.1 length when extracting certificate CNs. Add a cert_update regression test for certificates without a common name. Fixes #13484. --- src/api/InkAPI.cc | 31 ++++++++++++++----- .../cert_update/cert_update.test.py | 13 ++++++++ .../cert_update/gold/update-no-cn.gold | 1 + 3 files changed, 38 insertions(+), 7 deletions(-) create mode 100644 tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 581dae89982..e7d69a31b60 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8316,19 +8316,36 @@ 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); + // 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 X509_NAME *subject = X509_get_subject_name(cert.get()); + const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1); + if (pos < 0) { + return TS_ERROR; + } + + const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(subject, pos); const ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); - char *common_name_str = reinterpret_cast(const_cast(ASN1_STRING_get0_data(common_name_asn1))); - if (ASN1_STRING_length(common_name_asn1) != static_cast(strlen(common_name_str))) { + if (!common_name_asn1) { + 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) { + return TS_ERROR; + } + + const std::string common_name_str{reinterpret_cast(common_name_data), static_cast(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, common_name_str.c_str()); // Update context to use cert - cc = lookup->find(common_name_str); + cc = lookup->find(common_name_str.c_str()); if (cc && cc->getCtx()) { test_ctx = shared_SSL_CTX(SSLCreateServerContext(config, cc->userconfig.get(), cert_path, key_path), SSLReleaseContext); if (!test_ctx) { diff --git a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py index bbbaa31aa02..4285cb0a143 100644 --- a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py +++ b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py @@ -99,6 +99,19 @@ ts.Disk.traffic_out.Content = "gold/update.gold" ts.StillRunningAfter = server +# Server-Cert-Update-No-CN +# A certificate without a common name must be rejected without crashing ATS. +tr = Test.AddTestRun("Server-Cert-Update-No-CN") +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Command = ( + 'openssl req -x509 -newkey rsa:2048 -nodes -keyout {0}/no-cn.key -out {0}/no-cn.crt ' + '-subj /O=NoCN -days 1 >/dev/null 2>&1 && ' + 'cat {0}/no-cn.key {0}/no-cn.crt > {0}/no-cn.pem && ' + '{1}/traffic_ctl plugin msg cert_update.server {0}/no-cn.pem'.format(ts.Variables.SSLDir, ts.Variables.BINDIR)) +ts.Disk.traffic_out.Content = "gold/update-no-cn.gold" +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + # Server-Cert-After # after use traffic_ctl to update server cert, curl should see bar.com cert from bob tr = Test.AddTestRun("Server-Cert-After") diff --git a/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold b/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold new file mode 100644 index 00000000000..edebce3c258 --- /dev/null +++ b/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold @@ -0,0 +1 @@ +``Failed to update server cert with .*no-cn.pem`` From dde7e2b91c0f58e4d3b72a4078c5b91e662adfc0 Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:04:22 +0000 Subject: [PATCH 2/6] Fix cert update regression test assertions Preserve the existing traffic output testers when adding the no-CN regression assertion, and pass the length-bounded common name directly to the lookup. The test now explicitly verifies that ATS remains running. --- src/api/InkAPI.cc | 2 +- .../gold_tests/pluginTest/cert_update/cert_update.test.py | 8 +++++--- .../pluginTest/cert_update/gold/update-no-cn.gold | 1 - 3 files changed, 6 insertions(+), 5 deletions(-) delete mode 100644 tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index e7d69a31b60..de943c04248 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8345,7 +8345,7 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) Dbg(dbg_ctl_ssl_cert_update, "Updating from %s with common name %s", cert_path, common_name_str.c_str()); // Update context to use cert - cc = lookup->find(common_name_str.c_str()); + cc = lookup->find(common_name_str); if (cc && cc->getCtx()) { test_ctx = shared_SSL_CTX(SSLCreateServerContext(config, cc->userconfig.get(), cert_path, key_path), SSLReleaseContext); if (!test_ctx) { diff --git a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py index 4285cb0a143..2558e59538c 100644 --- a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py +++ b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py @@ -96,7 +96,7 @@ tr.Processes.Default.Env = ts.Env tr.Processes.Default.Command = ( '{0}/traffic_ctl plugin msg cert_update.server {1}/server2.pem'.format(ts.Variables.BINDIR, ts.Variables.SSLDir)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += "gold/update.gold" ts.StillRunningAfter = server # Server-Cert-Update-No-CN @@ -108,8 +108,10 @@ '-subj /O=NoCN -days 1 >/dev/null 2>&1 && ' 'cat {0}/no-cn.key {0}/no-cn.crt > {0}/no-cn.pem && ' '{1}/traffic_ctl plugin msg cert_update.server {0}/no-cn.pem'.format(ts.Variables.SSLDir, ts.Variables.BINDIR)) -ts.Disk.traffic_out.Content = "gold/update-no-cn.gold" +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + r"Failed to update server cert with .*no-cn\.pem", "ATS should reject a certificate that has no common name") tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = ts ts.StillRunningAfter = server # Server-Cert-After @@ -141,7 +143,7 @@ tr.Processes.Default.Command = ( 'mv {0}/client2.pem {0}/client1.pem && {1}/traffic_ctl plugin msg cert_update.client {0}/client1.pem'.format( ts.Variables.SSLDir, ts.Variables.BINDIR)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += "gold/update.gold" ts.StillRunningAfter = server # Client-Cert-After diff --git a/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold b/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold deleted file mode 100644 index edebce3c258..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/update-no-cn.gold +++ /dev/null @@ -1 +0,0 @@ -``Failed to update server cert with .*no-cn.pem`` From a9019708d89a83c3b38f9fea6c900204d805000e Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:14:20 +0000 Subject: [PATCH 3/6] Improve certificate update diagnostics Report malformed or CN-less certificates and keep certificate names length-bounded in debug output. The focused cert_update AuTest passes with the updated API implementation. --- src/api/InkAPI.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index de943c04248..5dfa88ac9f3 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8322,18 +8322,21 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) const X509_NAME *subject = X509_get_subject_name(cert.get()); const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1); if (pos < 0) { + SSLError("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 ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); if (!common_name_asn1) { + SSLError("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) { + SSLError("Failed to extract common name from certificate %s: invalid ASN.1 value", cert_path); return TS_ERROR; } @@ -8343,7 +8346,8 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) return TS_ERROR; } - Dbg(dbg_ctl_ssl_cert_update, "Updating from %s with common name %s", cert_path, common_name_str.c_str()); + Dbg(dbg_ctl_ssl_cert_update, "Updating from %s with common name %.*s", cert_path, static_cast(common_name_str.size()), + common_name_str.data()); // Update context to use cert cc = lookup->find(common_name_str); if (cc && cc->getCtx()) { From 9a9e3283baf2843e871b82ab522b152c9b9a9fa5 Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:26:02 +0000 Subject: [PATCH 4/6] Avoid error logging for invalid certificate CN --- src/api/InkAPI.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 5dfa88ac9f3..fc2889f05bd 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8322,21 +8322,21 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) const X509_NAME *subject = X509_get_subject_name(cert.get()); const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1); if (pos < 0) { - SSLError("Failed to extract common name from certificate %s: no commonName", cert_path); + 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 ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); if (!common_name_asn1) { - SSLError("Failed to extract common name from certificate %s: missing ASN.1 value", cert_path); + 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) { - SSLError("Failed to extract common name from certificate %s: invalid ASN.1 value", cert_path); + Dbg(dbg_ctl_ssl_cert_update, "Failed to extract common name from certificate %s: invalid ASN.1 value", cert_path); return TS_ERROR; } From fecbe50f6db270af76498a85126f53e5a5bef031 Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:56:53 +0000 Subject: [PATCH 5/6] Use inferred type for OpenSSL certificate entry data --- src/api/InkAPI.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index fc2889f05bd..7f5cfb0355c 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8327,7 +8327,7 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) } const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(subject, pos); - const ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); + 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; From 1421f8bb2bb9cfa2cb65617fbf23d23cdd6c0e28 Mon Sep 17 00:00:00 2001 From: RajaMuhammadAwais <1.19938988e+08+RajaMuhammadAwais@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:43:54 +0000 Subject: [PATCH 6/6] Use inferred type for OpenSSL subject name --- src/api/InkAPI.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 7f5cfb0355c..628ee00eba3 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8319,8 +8319,8 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) // 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 X509_NAME *subject = X509_get_subject_name(cert.get()); - const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1); + const auto *subject = X509_get_subject_name(cert.get()); + const int pos = X509_NAME_get_index_by_NID(subject, NID_commonName, -1); if (pos < 0) { Dbg(dbg_ctl_ssl_cert_update, "Failed to extract common name from certificate %s: no commonName", cert_path); return TS_ERROR;