bryancall commented on PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#issuecomment-5606762599
Re-reviewed at `f9580f6e88`. Note that the branch has not moved since my
last comment — the newest commit is dated Aug 26 and that comment was Sep 8 —
so everything in it is still open, including both blocking items.
`SSLUtils.cc:522` still reads `SSL_CTX_get_verify_mode(matched_ctx)`. I am not
repeating those here.
What follows is new, from a pass focused on the BoringSSL paths. All three
blocking items are in code that no CI job compiles.
## Blocking
**1. Null dereference on QUIC connections. `SSLUtils.cc:305`**
`ssl_custom_verify_client_callback()` checks `tbs` for null but then
dereferences `netvc` unconditionally:
```cpp
const char *ca_cert_file = netvc->get_ca_cert_file();
```
The `tbs` check does not imply `netvc` is non-null — they are separate
ex_data slots. `SSLNetVConnection::_bindSSLObject()`
(`SSLNetVConnection.cc:143`) calls both `SSLNetVCAttach()` and
`TLSBasicSupport::bind()`, but `QUICNetVConnection::_bindSSLObject()`
(`QUICNetVConnection.cc:569`, and identically
`OpenSSLQUICNetVConnection.cc:594`) binds `TLSBasicSupport` and never calls
`SSLNetVCAttach`. So on a QUIC connection `TLSBasicSupport::getInstance()`
returns non-null while `SSLNetVCAccess()` returns null.
`QUICMultiCertConfigLoader` inherits `load_certs()` and
`_setup_client_cert_verification()` unchanged, so a QUIC listener sharing an
`ssl_multicert.yaml` entry that sets `ssl_client_rpk_ca_name` gets
`SSL_CTX_set_custom_verify(ctx, ..., ssl_custom_verify_client_callback)`. On a
BoringSSL build with HTTP/3 enabled and `clientCertLevel != 0`, any H3 client
presenting an X.509 client certificate takes the fallback branch and crashes
`traffic_server`.
The outbound counterpart at `SSLClientUtils.cc:239` gets this right. The RPK
branch's `netvc->options.sni_servername.get()` at `:277` needs the same guard.
**2. Both hand-rolled X.509 fallbacks drop certificate purpose enforcement.
`SSLUtils.cc:322`, `SSLClientUtils.cc:329`**
Both rebuilds do `X509_STORE_CTX_new()` → `X509_STORE_CTX_init()` →
`X509_STORE_CTX_set_depth()` → `X509_verify_cert()`. What they replace does
more. BoringSSL's `ssl_crypto_x509_session_verify_cert_chain()`
(`ssl/ssl_x509.cc`) also calls:
```cpp
!X509_STORE_CTX_set_default(ctx.get(), ssl->server ? "ssl_client" :
"ssl_server") ||
!X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()),
hs->config->param) ||
```
OpenSSL's `ssl_verify_internal()` in `ssl/ssl_cert.c` makes the identical
pair of calls, so this is a regression against both libraries, not a BoringSSL
quirk.
`X509_STORE_CTX_set_default()` sets `param->purpose` and `param->trust`. In
`crypto/x509/x509_vfy.c` the purpose check is gated on `ctx->param->purpose >
0`, so with purpose left at 0 `X509_check_purpose()` never runs and neither
`xku_reject()` nor `ku_reject()` fire. I grepped the tree: ATS never sets
`X509_VERIFY_PARAM` anywhere itself, so these defaults were coming entirely
from inside the library path this PR bypasses. The missing
`X509_VERIFY_PARAM_set1` is therefore inert, but the purpose and trust loss is
not.
Inbound this matters more than it first looks. Purpose `ssl_client` is the
only thing ATS's client-certificate verification uses to distinguish a client
certificate from any other certificate — there is no name check on the inbound
side. Previously a leaf with `extendedKeyUsage = serverAuth` was rejected with
`X509_V_ERR_INVALID_PURPOSE`; now anyone holding any end-entity certificate
issued by the trusted client CA authenticates as a valid mTLS client, including
an origin's own TLS certificate.
Outbound is the same shape, in the configuration this feature recommends:
`tls_rpk_hop.test.py:107` sets `verify.server.properties: SIGNATURE`, so there
is no name check either, and a `clientAuth`-only certificate chaining to the
trusted CA authenticates the next hop.
Adding `X509_STORE_CTX_set_default(store_ctx, "ssl_client")` /
`"ssl_server"` after `X509_STORE_CTX_init()`, and checking the return, restores
it.
**3. The RPK skip in `validate_server_certificate_hostname()` defeats the
cross-origin session-reuse guard. `SSLClientUtils.cc:413-424`**
The comment describes this as being about resumption, but the function is
not on the handshake path at all. Its only callers are
`HttpSessionManager.cc:52` (`validate_session_origin_cert`) and
`ConnectingEntry.cc:101`. In `ServerSessionPool::acquireSession()` it is the
gate that stops a multiplexed H2 origin session found in the pool from being
reused for a request to a different hostname, and in the IP-match branch at
`HttpSessionManager.cc:223-227` it is the only gate.
So with `proxy.config.http.server_session_sharing.match` set to `ip` and H2
to origin: hops A and B behind one VIP, `sni.yaml` pinning A's key under one
fqdn and a different key (or none) under B's. An H2 session authenticated to A
is found in the IP pool for a request to B, and the early return makes the
guard unconditionally true. B's traffic goes over a connection authenticated
only against A's pin set. The X.509 path re-runs `validate_hostname()` on every
reuse and does not have this hole.
To be fair on severity: the default `match` is `both`, which takes the other
branch and compares `hostname_hash` first, so this needs an explicit `match:
ip`. It is not default-exploitable.
Two smaller notes on the same block. `SSL_get0_peer_rpk()` *is* the session
lookup on both libraries — OpenSSL's returns `sc->session->peer_rpk` and
BoringSSL's returns `SSL_get_session(ssl)->peer_raw_public_key.get()` — so the
stated reason for consulting the session instead is not a real constraint. And
because `SSL_get_session()` returns the current session on a fresh connection
too, the check fires on every RPK connection, not only resumed ones.
## Also worth addressing
- **`ssl_client_rpk_ca_name` is silently inert when
`proxy.config.ssl.client.certification_level` is 0.** The entire body of
`_setup_client_cert_verification()` is inside `if (params->clientCertLevel !=
0)` (`SSLUtils.cc:1656`), so at the default 0 no verify mode and no callback
are ever installed. `load_certs()` has already loaded the pin set and
advertised `{rpk, x509}`, so the server announces RPK acceptance, never
requests a credential, and pins nothing — with no warning and a successful
`traffic_ctl config reload`. `ssl_multicert.yaml.en.rst:155` tells the operator
"An unmatched pin is always fatal to the connection." Failing the config load
here would be better than a warning.
- **`SSLUtils.cc:1096`** does not check `ssl_client_rpk_ca_index < 0`,
unlike the outbound counterpart at `SSLClientUtils.cc:504`. On BoringSSL this
is not survivable the way it is on OpenSSL: `CRYPTO_set_ex_data()` returns 1
unconditionally, so `SSL_CTX_set_ex_data()` reports success, the
`TrustedKeySet` leaks and is never stored, and the classic non-RPK callback is
installed while the config load reports success.
- **`SSLUtils.cc:305-318`** builds an `X509_STORE` and calls
`X509_STORE_load_locations()` inside the handshake, on the event thread, once
per connection. That is a synchronous open plus PEM parse per inbound
connection on any RPK-enabled entry with a per-connection CA set. Caching the
parsed store, or stashing the one `setClientCertCACerts()` already built,
avoids it. Note also that if the load fails the code falls back to the ctx
default store rather than failing, which silently widens trust relative to what
the per-SNI action asked for.
- **`SSLUtils.cc:323` and `SSLClientUtils.cc:330`** do not call
`X509_STORE_CTX_set_ex_data(store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx(),
ssl)`. BoringSSL's own path does, and ATS's own callbacks depend on that idiom
(`SSLUtils.cc:205`, `SSLClientUtils.cc:90`). A `TS_EVENT_SSL_VERIFY_*` plugin
that recovers the `SSL *` from the store ctx handed to it gets null on these
paths. No in-tree plugin does this, so the blast radius is third-party only.
- **`SSLRPKUtils.cc`**, the EOF check tests `ERR_GET_REASON(err) ==
PEM_R_NO_START_LINE` without also checking `ERR_GET_LIB(err) == ERR_LIB_PEM`.
Reason codes 100-255 are per-sub-library and 27 other OpenSSL sub-libraries
define a reason 108, several of which a failing key decode can plausibly push.
The value also differs across libraries. Both OpenSSL's `ssl/ssl_rsa.c` and
BoringSSL's `crypto/pem/pem_lib.cc` check lib and reason together.
- **Comment accuracy.** `SSLUtils.cc:1679` says `SSL_CTX_set_verify()` and
`SSL_CTX_set_custom_verify()` are mutually exclusive per SSL_CTX. They are not:
they write different fields (`default_verify_callback` vs
`custom_verify_callback`) and collide only on `verify_mode`, and
`ssl_verify_peer_cert()` prefers the custom one whenever it is set. The code at
that site is correct, but the same belief is what makes the reset at `:534` a
no-op, which I raised last time. Separately, the "the hook always runs, as on
the X.509 path below" comment at `SSLClientUtils.cc:138` and `:270` is not true
for that file — outbound `verify_callback()` returns without calling
`tbs->verify_certificate()` in three places (signature failure at `:169`,
`depth != 0` at `:175`, name failure at `:200`). The identical claim in
`SSLUtils.cc:218` is accurate, since the inbound callback really does run the
hook unconditionally.
## Tests
The thing I most want before this merges is a negative test for the
hand-rolled X.509 verification, in either direction. There is currently none.
Changing `verified = X509_verify_cert(store_ctx) == 1` to `verified = true` in
`ssl_custom_verify_client_callback()` passes the entire suite.
Scenario 2 does not cover the outbound side despite reaching it. It runs
`PERMISSIVE`, so at `SSLClientUtils.cc:351` a signature failure sets `accepted
= !enforce_mode` and the chain verdict is discarded before it can affect the
result. Since `server.pem` is self-signed and the edge does not trust it, the
chain check in that test is failing right now and the test passes anyway.
Worth adding: an untrusted client chain rejected on an RPK-enabled entry; a
`serverAuth`-only client certificate rejected (that one would have caught item
2); an origin whose chain does not verify rejected under `ENFORCED` on a
`server_rpk_ca` hop; and the matching positive, an `ENFORCED` X.509 fallback
that succeeds, which is the rolling-upgrade path the feature exists to serve
and is currently only exercised in permissive mode.
One packaging note: `test_SSLRPKUtils.cc` is wrapped entirely in `#if
TS_USE_RPK`, so on a build with OpenSSL older than 3.2 it compiles to zero test
cases and the suite reports green with no RPK coverage at all. Nothing in the
output distinguishes that from a real pass.
## Checked and not reporting
Two things I looked into and concluded are fine, recorded so they don't get
re-raised:
Optional client certificates are not broken by the unconditional
`SSL_AD_CERTIFICATE_REQUIRED` at `SSLUtils.cc:291`. BoringSSL never invokes the
custom verify callback for a peer that sent no certificate — TLS 1.3
`do_read_client_certificate_verify()` and TLS 1.2
`do_verify_client_certificate()` both guard the `ssl_verify_peer_cert()` call
on `ssl_session_has_peer_cred()`, and `SSL_VERIFY_FAIL_IF_NO_PEER_CERT` is
enforced by the library before that point. The branch is unreachable defensive
code; a comment saying so would save the next reader the trip.
The nested `#if HAVE_SSL_CTX_SET_CLIENT_HELLO_CB` at `SSLUtils.cc:377` is
not redundant, as I first assumed. The outer `#if/#elif/#endif` closes at
`:368`, before the shared function body, so the inner guard is what keeps
`SSL_CTX_get0_server_cert_type()` and friends off the BoringSSL build. It
should stay.
--
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]
