bneradt commented on code in PR #13584:
URL: https://github.com/apache/trafficserver/pull/13584#discussion_r3858008812
##########
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) {
Review Comment:
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.
##########
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"
Review Comment:
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.
```python
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.
##########
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<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))) {
+ 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<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, common_name_str.c_str());
// Update context to use cert
- cc = lookup->find(common_name_str);
+ cc = lookup->find(common_name_str.c_str());
Review Comment:
`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:
```cpp
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.
--
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]
