bryancall commented on PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5638768164
Reviewed at `8e08afa4d8`. I verified each of the five blocking items against
the code rather than taking the summary's word for it, and they are all
genuinely fixed:
- `SSL_get_verify_mode(ssl)` at `SSLUtils.cc:535`, with no
`SSL_CTX_get_verify_mode` left anywhere, and the `setClientCertLevel()` branch
removed rather than patched.
- The inbound callback no longer assumes an `SSLNetVConnection`, and the
server name comes from `TLSSNISupport`.
- `X509_STORE_CTX_set_default()`, `X509_VERIFY_PARAM_set1()` and
`X509_STORE_CTX_set_ex_data()` in both directions, `&&`-chained so the returns
are checked. Dropping the explicit `X509_STORE_CTX_set_depth()` is correct:
BoringSSL's `SSL_set_verify_depth()` writes into `config->param` and
`SSL_get0_param()` returns that same param, so depth still travels.
- Origin session reuse is scoped to the name the connection authenticated
for.
- The smaller items all check out, and the fail-closed change on a
configured-but-missing CA store is better than what I asked for.
Two of these are fixed more carefully than the report I gave you, so thank
you for that.
I am still requesting changes, because this round introduced a new defect in
the code that fixes one of them.
## Blocking: use-after-free in the new verify-store stash
`SSLUtils.cc:1386-1394` writes the stash, `:318-331` reads it,
`SNIActionPerformer.cc:312` and `SSLUtils.cc:518` are the two call sites.
`9b9b95fdd1` replaced the per-handshake rebuild with a non-owning pointer in
`ssl_verify_store_index`. The stash is written in one place, the success path
at `:1389`, and cleared nowhere. `SSL_set0_verify_cert_store()` takes
ownership, and `SSL_set_SSL_CTX()` replaces `config->cert` wholesale, whose
destructor frees `verify_store`. The codebase already knows this: the comment
at `:517` ("After replacing the SSL_CTX, make sure the overridden ca_cert_file
is still set") is why `setClientCertCACerts()` is called a second time there.
So:
1. A `verify_client` action on an entry that also sets
`ssl_client_rpk_ca_name` builds store A, hands it to
`SSL_set0_verify_cert_store()`, and stashes A.
2. `selectCertificate()` calls `SSL_set_SSL_CTX()`, which frees A. The stash
still holds A.
3. `:518` rebuilds. If `X509_STORE_new()` returns null or
`X509_STORE_load_locations()` fails, which is the CA file rotated or its
permissions changed since config load, the `else` branch at `:1393` frees the
new store and leaves the stash pointing at the freed A.
4. A client presents an X.509 certificate, `:318` reads a non-null dangling
A and passes it to `X509_STORE_CTX_init()`.
The unfortunate part is that step 3 is exactly the condition the new
`ca_configured` guard at `:320-327` was added to fail closed on. A stale
non-null stash skips that guard, so a clean `SSL_AD_INTERNAL_ERROR` becomes a
use-after-free on attacker-reachable input instead. The `SSL_set_ex_data()`
failure path at `:1389` has the same shape: it logs and leaves the previous
pointer in place.
Clearing the slot on both failure paths covers it, and clearing it right
after `selectCertificate()` returns would make the invalidation point explicit.
Using `SSL_set1_verify_cert_store()` with an owning free function on the
ex_data index would remove the category.
## Also worth addressing
**`SSLUtils.cc:238` still has the unguarded dereference.** The note says
both unguarded uses were inbound and are fixed, but only
`ssl_custom_verify_client_callback` was. The RPK branch this PR adds to
`ssl_verify_client_callback` still calls `netvc->options.sni_servername.get()`
on a pin mismatch at `:238`, and again at `:241`, under the same
"TLSBasicSupport is bound but `SSLNetVCAttach` was never called" condition. It
needs OpenSSL 3.5 plus QUIC to reach, but it is the same one-line fix as its
twin.
**RPK origin sessions stop pooling when the outbound SNI is empty.**
`SSLClientUtils.cc:427` compares `netvc->options.sni_servername` against the
`hostname` argument, but those come from different sources.
`get_outbound_sni_for_cert_verification()` falls back to the Host header and
then to `current.server->name` (`HttpSM.cc:5510-5518`), while `sni_servername`
is only set when the SNI is non-empty. With an empty outbound SNI the
comparison is `""` against the Host header, so it returns false on every call
and `validate_session_origin_cert()` never releases an H2 session from the
pool. It fails closed, so this is a functional regression rather than a hole,
but it is silent outside `ssl_verify` debug. Comparing against the same source
the establishing side used would fix it. Note this branch also sits above the
`check_name` gate, so it applies to operators who deliberately left `NAME` out
of `verify_server_properties`.
**On QUIC, `ssl_cert_callback` never runs.**
`QUICMultiCertConfigLoader::_set_handshake_callbacks()` overrides the base and
installs a `cert_cb` that only calls `select_quic_sni_context()`. So the
premise behind removing the `setClientCertLevel()` branch, that
`ssl_cert_callback` runs afterwards and is the single decision point, does not
hold for H3. `ssl_client_rpk_ca_name` on a non-default entry stays inert there,
which is the same bug this PR fixes for TLS. It is fail-closed, so a gap rather
than a bypass, but the comments read as though it works everywhere. Wiring it
into the QUIC callback or documenting it as unsupported would both be fine.
## The CI paragraph in the description is wrong
It says the BoringSSL jobs pin a June 2024 revision predating
`SSL_CREDENTIAL_new_raw_public_key`. `tools/build_boringssl_h3_tools.sh:42`
pins `c3ffc3300a9450cf8e396c7880be7c6cadc16a4a`, which is dated 2026-04-24 and
was last bumped on 2026-05-22 in
[#13190](https://github.com/apache/trafficserver/pull/13190). I checked that
revision's `include/openssl/ssl.h` directly and it declares
`SSL_CREDENTIAL_new_raw_public_key`, `SSL_get0_peer_rpk` and
`SSL_set1_accepted_peer_cert_types`, so `TS_USE_RPK` would be on for a
BoringSSL build produced by the repo's own script.
The error understates the coverage rather than overstating it, so nothing
rests on it, but the docs should be right. The caveat that does hold is
narrower: the Jenkins images are built out of the `trafficserver-ci` repo, so
whether they actually track this pin is not visible from this tree. Worth
saying that instead, or checking the image directly.
## Tests
The four new scenarios are a real improvement, and scenario 12 closes the
hole I cared about most, since the accept path under ENFORCED was previously
only exercised under PERMISSIVE where the verdict was discarded. The
`server.ocsp.pem` and `ca.ocsp.pem` pair is a clean purpose isolation: it
chains cleanly and fails on purpose alone, which is what makes the
`set_default()` mutation meaningful.
One gap. Scenarios 9, 10 and 11 assert only `ExcludesExpression('origin
response')`, so they establish that the connection did not work rather than
that the fallback rejected it for the intended reason. Scenario 9 and scenario
10 are currently indistinguishable, and scenario 9 would also pass if the edge
sent no certificate at all, in which case the empty-chain early return fires
and the hand-rolled verification never runs. Adding the distinguishing string
to each would fix it, and for scenario 10 that string is free, since
`X509_verify_cert_error_string()` prints `unsupported certificate purpose`.
Separately, the new stash plumbing has no coverage at all. No scenario
combines a `verify_client` CA override with an RPK-enabled entry, which is the
path the use-after-free above lives on, and this delta removed the rebuild that
used to cover it.
--
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]
