maskit commented on PR #13548: URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5641513780
Fixed at `0fcb674`, one commit per item. **Blocking: the verify-store use-after-free.** Confirmed, and worse than the report — two more paths leave the pointer stale. `sslnetvc` is a `dynamic_cast`, so the rebuild is skipped entirely when it fails, and the whole rebuild block sits inside `#if TS_HAS_TLS_SESSION_TICKET`. Clearing on the failure paths would not have covered either. So instead of clearing, the recorded copy now owns a reference: the index is registered with a free function, and a helper releases the outgoing store when the slot is replaced, since `SSL_set_ex_data()` runs the free function only at `SSL_free`. That removes the category rather than patching the paths, and it degrades better — after `SSL_set_SSL_CTX()` the recorded store is still the one the SNI action asked for, so a failed rebuild now verifies against correct trust instead of either faulting or failing the connection closed. I also chained the `SSL_set0_verify_cert_store()` result into the success condition, since its failure would otherwise have leaked the store. **`SSLUtils.cc:238` unguarded dereference.** Confirmed and fixed, and you were right that I only did the twin. The name now comes from `TLSSNISupport` in the whole function, including the pre-existing uses further down — leaving an identical crash two lines from the fix seemed gratuitous. **Origin sessions and the empty outbound SNI.** Confirmed. Rather than fix the comparison in place I deleted it: `ServerSessionPool::validate_sni()` already answers exactly this question correctly, reading the session's name from `TLSSNISupport`, comparing against `get_outbound_sni()`, and treating two empty names as a match. The gate moved to the two callers that have an `HttpSM`, behind a predicate that also keeps the `verify.server.policy: DISABLED` early-out. It stays above the name-property check, as you noted. The autest incidentally confirms the premise — a hop toward `127.0.0.1` sends no SNI at all, so the old comparison was `""` against the Host header on every call. **HTTP/3.** Took the documentation option. Tracing it first was worth it, because the mechanism is narrower than "the callback does not run": on OpenSSL, `SSL_set_SSL_CTX()` replaces `sc->cert` but never refreshes `sc->client_cert_type`/`server_cert_type`, which are copied once from the original `SSL_CTX` in `ossl_ssl_init()`. So an H3 connection keeps the **default** entry's certificate types, and it is specifically a non-default entry that goes inert. Separately, `VerifyClient::SNIAction` returns early when there is no `SSLNetVConnection`, so a `verify_client` action does not reach H3 either — leaving the global certification level on the default entry as the only way to request a client credential there. The docs now say that; there is a one-line pointer at the QUIC callback. **Tests.** Added the distinguishing string to all three: `unable to get local issuer certificate`, `unsupported certificate purpose`, and for the outbound one `Core server certificate verification failed` with `Action=Terminate`. I verified each of these actually fails when the code it covers is broken, rather than reasoning about it. Removing `X509_STORE_CTX_set_default(..., "ssl_client")` makes the purpose scenario fail on both assertions — the serverAuth-only certificate authenticates and the reason string disappears. That was worth doing: my first attempt at the third assertion was on `traffic_out` when the text is a `Warning` and lands in `diags.log`, so it passed while checking nothing until the break exposed it. New scenario for the CA override, sharper than the one I first wrote: the global CA cannot verify the offered chain and the per-SNI override can, so reaching the origin is positive proof the fallback used the per-connection store. Forcing the recorded store to null makes it, and only it, fail. It is driven by a client sending a real SNI rather than by a proxy hop, because a hop toward an IP literal sends none and could never match the action — my first version silently tested nothing for exactly that reason. The use-after-free paths themselves are not reachable from a test: they need the store rebuild to fail, or a null netvc. The new scenario covers the plumbing, not the fault. **The CI paragraph.** Here I think the description was right and the reasoning you checked against was a different file. `Rocky` builds against BoringSSL, but not the one `tools/build_boringssl_h3_tools.sh` pins — that script is a developer convenience the images do not use. `trafficserver-ci/docker/rockylinux8/Dockerfile` copies in its **own** `build_boringssl_h3_tools.sh` and runs it, and that copy hardcodes `git checkout 45b2464158379f48cec6e35a1ef503ddea1511a6`, dated 2024-06-21, whose `include/openssl/ssl.h` has no `SSL_CREDENTIAL_new_raw_public_key`. Chain: `rocky.pipeline` image `rockylinux:8`, `--preset ci-rocky`, `OPENSSL_ROOT_DIR=/opt/h3-tools-boringssl/boringssl`. That said, the paragraph had an error of mine you did not catch: it claimed two BoringSSL jobs. There is no live `rocky-asan` — its pipeline asks for a `ci-rocky-asan` preset that does not exist in `CMakePresets.json`, it is not wired into `toplevel.pipeline`, and `gh pr checks` lists only `Rocky`. Rewritten with the full chain spelled out, plus your caveat, which is the right one: whether the deployed images still match those Dockerfiles is not visible from this tree. I also noted `ci-fedora-quiche`, which does point at a BoringSSL new enough for RPK but which no pipeline references — wiring up a job for it looks cheaper than bumping the rocky pin. -- 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]
