Fix unsafe certificate common name extraction by RajaMuhammadAwais · Pull Request #13584 · apache/trafficserver

RajaMuhammadAwais · 2026-08-25T15:30:36Z

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.

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 apache#13484.

bneradt · 2026-08-25T22:12:20Z

[approve ci]

bneradt

Reviewed and tested this locally (Fedora container, cherry-picked onto current master, cmake --build build -t format clean).

The core fix is correct and I confirmed it does what it says. Reverting only src/api/InkAPI.cc and running the new test reproduces the crash from #13484:

traffic_server: received signal 11 (Segmentation fault)
/lib64/libcrypto.so.3(ASN1_STRING_get0_data+0xc) [0xffffb00d170c]
libtsapi.so(_Z21TSSslServerCertUpdatePKcS0_+0x214) [0xffffb0786464]
cert_update.so(_Z14CB_cert_updateP10tsapi_cont7TSEventPv+0x1f4) [0xffffad19365c]

With the patch applied, traffic_server survives and logs Failed to update server cert with .../no-cn.pem. Both parts of the issue (unguarded pos == -1, and strlen() on a non-NUL-terminated ASN1_STRING) are addressed, and the embedded-NUL rejection is preserved.

Two things to fix before this lands: the new gold file is never actually compared (details inline), and a small std::string round-trip in the lookup->find() call.

bneradt · 2026-08-25T22:28:46Z

+    '-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"


This assignment is dead, so the assertion you added never runs.

Disk.<file>.Content = ... routes to TesterSet.Assign(), which replaces the tester list rather than appending to it. Line 144 (Client-Cert-Update) assigns gold/update.gold to this same ts.Disk.traffic_out.Content afterwards, so gold/update-no-cn.gold is discarded and never compared against anything.

I verified this by replacing the entire contents of gold/update-no-cn.gold with THIS_STRING_WILL_NEVER_APPEAR_ANYWHERE and re-running the test: still Passed: 1.

There is a second problem hiding behind the first: gold files are not regexes. autest/testers/gold_file.py does a whole-content diff and only substitutes the wildcard tokens `` and {}; `.*` is compared literally. When I forced the gold to be the live assertion (by moving it after line 144), it failed with exactly that:

- ... Failed to update server cert with .*no-cn.pem + ... Failed to update server cert with /tmp/sb/cert_update/ts/ssl/no-cn.pem

No existing gold file under tests/gold_tests uses .*, which is consistent with it not being supported.

Suggested fix: drop gold/update-no-cn.gold and use a real regex tester with += so it is additive.

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")

Lines 99 and 144 need to become += as well, otherwise the later Assign() still wipes this one out. I ran that combination locally: it passes with your InkAPI.cc fix, fails when the expression is changed to one that cannot match, and fails against unpatched InkAPI.cc.

One more note: the test does catch the regression today, but only incidentally. traffic_ctl exits non-zero because traffic_server segfaults out from under the RPC connection, so the ReturnCode = 0 check on line 112 is what trips. Adding tr.StillRunningAfter = ts would turn the "without crashing ATS" intent in your comment into an actual assertion instead of relying on that side effect.

Thanks for the detailed review. I removed the unused gold file, switched the testers to additive += with Testers.ContainsExpression, added tr.StillRunningAfter = ts, and changed the lookup to find(common_name_str). These fixes have been pushed to the existing PR branch.

bneradt · 2026-08-25T22:28:46Z

+    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());


SSLCertLookup::find() takes const std::string & (src/iocore/net/P_SSLCertLookup.h:156), so .c_str() discards the length you just computed and makes the compiler build a second std::string — including a strlen() — for the temporary. Pass the string through directly:

cc = lookup->find(common_name_str);

Compare line 8029 in this same file, which passes its std::string to find() unchanged.

Minor, same idea on the Dbg() above: the other dbg_ctl_ssl_cert_update sites in this file use the %.*s form (e.g. lines 8072, 8241), so "%.*s" with static_cast<int>(common_name_str.size()), common_name_str.data() would match local style. Either way works now that the string is length-bounded.

bneradt · 2026-08-25T22:28:46Z

+    // 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) {


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.

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.

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.

maskit · 2026-08-27T16:34:36Z

+    // 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());


The type probably has to be auto to deal with OpenSSL API signature difference.

maskit · 2026-08-27T18:04:12Z

../src/api/InkAPI.cc:8354:32: error: no matching function for call to 'X509_NAME_get_index_by_NID'
    const int        pos     = X509_NAME_get_index_by_NID(subject, NID_commonName, -1);
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/openssl/x509.h:810:5: note: candidate function not viable: 1st argument ('const X509_NAME *' (aka 'const X509_name_st *')) would lose const qualifier
int X509_NAME_get_index_by_NID(X509_NAME *name, int nid, int lastpos);
    ^

maskit · 2026-08-28T18:39:48Z

+    // 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());


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

Fix unsafe certificate common name extraction

2300d5a

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 apache#13484.

JosiahWI assigned RajaMuhammadAwais Aug 25, 2026

JosiahWI added the Cleanup label Aug 25, 2026

JosiahWI added this to the 11.0.0 milestone Aug 25, 2026

JosiahWI added this to ATS v10.2.x Aug 25, 2026

JosiahWI added TS API Bug and removed Cleanup labels Aug 25, 2026

bneradt reviewed Aug 25, 2026

View reviewed changes

RajaMuhammadAwais added 3 commits August 27, 2026 14:04

Fix cert update regression test assertions

dde7e2b

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.

Improve certificate update diagnostics

a901970

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.

Avoid error logging for invalid certificate CN

9a9e328

maskit reviewed Aug 27, 2026

View reviewed changes

Use inferred type for OpenSSL certificate entry data

fecbe50

RajaMuhammadAwais added 2 commits August 28, 2026 08:43

Use inferred type for OpenSSL subject name

1421f8b

Merge upstream master to resolve cert update test conflict

79120a6

maskit reviewed Aug 28, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix unsafe certificate common name extraction - #13584

Fix unsafe certificate common name extraction#13584
RajaMuhammadAwais wants to merge 7 commits into
apache:masterfrom
RajaMuhammadAwais:fix/13484-safe-certificate-cn

RajaMuhammadAwais commented Aug 25, 2026

Uh oh!

bneradt commented Aug 25, 2026

Uh oh!

bneradt left a comment

Uh oh!

bneradt Aug 25, 2026

Uh oh!

RajaMuhammadAwais Aug 27, 2026

Uh oh!

bneradt Aug 25, 2026

Uh oh!

bneradt Aug 25, 2026

Uh oh!

maskit Aug 27, 2026

Uh oh!

maskit commented Aug 27, 2026

Uh oh!

maskit Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

RajaMuhammadAwais commented Aug 25, 2026

Uh oh!

bneradt commented Aug 25, 2026

Uh oh!

bneradt left a comment

Choose a reason for hiding this comment

Uh oh!

bneradt Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

RajaMuhammadAwais Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

bneradt Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

bneradt Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

maskit Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

maskit commented Aug 27, 2026

Uh oh!

maskit Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants