Copilot commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r3779977557
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -205,6 +246,107 @@ ssl_verify_client_callback(int preverify_ok,
X509_STORE_CTX *ctx)
return preverify_ok;
}
+#if HAVE_SSL_CREDENTIAL_NEW_RAW_PUBLIC_KEY
+// BoringSSL's SSL_CTX_set_custom_verify(), required to accept RPK client
certs, replaces its
+// automatic X.509 chain verification entirely -- unlike OpenSSL's classic
SSL_CTX_set_verify(),
+// which only lets ssl_verify_client_callback() observe/override a chain
BoringSSL already
+// validated. For the X.509 fallback case (the client didn't offer an RPK this
time), this
+// callback must therefore redo that validation manually via the legacy
X509_STORE_CTX API,
+// against the same certificate store _setup_client_cert_verification()
configured on this ctx.
+static enum ssl_verify_result_t
+ssl_custom_verify_client_callback(SSL *ssl, uint8_t *out_alert)
+{
+ Dbg(dbg_ctl_ssl_verify, "Callback: custom verify client cert (RPK-enabled
ctx)");
+ SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
+ TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
+ if (tbs == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "ssl_custom_verify_client_callback call back on
stale netvc");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+
+ SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
+
+ if (SSL_get_peer_cert_type(ssl) == TLSEXT_cert_type_rpk) {
+ EVP_PKEY *peer_rpk = SSL_get0_peer_rpk(ssl);
+ auto *trusted = static_cast<SSLRPKUtils::TrustedKeySet
*>(SSL_CTX_get_ex_data(ctx, ssl_client_rpk_ca_index));
+ bool pin_ok = trusted != nullptr &&
SSLRPKUtils::pinnedKeyMatches(peer_rpk, *trusted);
+ if (!pin_ok) {
+ Warning("client raw public key did not match any trusted key for %s",
netvc->options.sni_servername.get());
+ }
+ // As above: the hook always runs, and can add rejection but not override
a failed pin match.
+ if (tbs->verify_certificate(nullptr) == 1 || !pin_ok) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ // X.509 fallback: BoringSSL hands back the raw chain as CRYPTO_BUFFERs, not
X509 objects, so
+ // rebuild it and run the same verification SSL_CTX_set_verify() would
otherwise do for us.
+ const STACK_OF(CRYPTO_BUFFER) *chain = SSL_get0_peer_certificates(ssl);
+ if (chain == nullptr || sk_CRYPTO_BUFFER_num(chain) == 0) {
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
+
+ X509 *leaf = nullptr;
+ STACK_OF(X509) *intermediates = sk_X509_new_null();
+ if (intermediates == nullptr) {
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(chain); i++) {
+ const CRYPTO_BUFFER *buf = sk_CRYPTO_BUFFER_value(chain, i);
+ const uint8_t *data = CRYPTO_BUFFER_data(buf);
+ X509 *cert = d2i_X509(nullptr, &data,
CRYPTO_BUFFER_len(buf));
+ if (cert == nullptr) {
+ SSLError("failed to parse a client certificate offered to a RPK-enabled
context");
+ X509_free(leaf);
+ sk_X509_pop_free(intermediates, X509_free);
+ *out_alert = SSL_AD_BAD_CERTIFICATE;
+ return ssl_verify_invalid;
+ }
+ if (i == 0) {
+ leaf = cert;
+ } else {
+ sk_X509_push(intermediates, cert);
+ }
Review Comment:
When rebuilding the X.509 chain for the BoringSSL custom-verify path,
`sk_X509_push()` return value is ignored. If it fails (allocation failure),
`cert` is leaked and verification proceeds with an incomplete chain. Handle
push failure explicitly and abort verification with an internal error.
##########
src/iocore/net/SSLClientUtils.cc:
##########
@@ -155,6 +225,171 @@ verify_callback(int signature_ok, X509_STORE_CTX *ctx)
return true;
}
+#if HAVE_SSL_CREDENTIAL_NEW_RAW_PUBLIC_KEY
+// BoringSSL rejects raw public keys outright unless a custom verify callback
is installed, and
+// SSL_set_custom_verify() displaces SSL_set_verify() (and with it BoringSSL's
automatic chain
+// verification) for the whole connection. So this callback owns both cases:
pin the peer's raw
+// public key, or -- when the next hop negotiated X.509 after all, the normal
state mid-rollout --
+// rebuild and verify the chain by hand before deferring to the usual
policy/name/hook logic.
+static enum ssl_verify_result_t
+ssl_client_custom_verify_callback(SSL *ssl, uint8_t *out_alert)
+{
+ SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
+ if (netvc == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "WARNING, NetVC is NULL in custom cert verify
callback");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ if (netvc->options.verifyServerPolicy == YamlSNIConfig::Policy::DISABLED) {
+ return ssl_verify_ok;
+ }
+
+ bool const enforce_mode = netvc->options.verifyServerPolicy ==
YamlSNIConfig::Policy::ENFORCED;
+
+ TLSBasicSupport *tbs = TLSBasicSupport::getInstance(ssl);
+ if (tbs == nullptr) {
+ Dbg(dbg_ctl_ssl_verify, "custom verify callback on stale netvc");
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+
+ if (SSL_get_peer_cert_type(ssl) == TLSEXT_cert_type_rpk) {
+ EVP_PKEY *peer_rpk = SSL_get0_peer_rpk(ssl);
+ const SSLRPKUtils::TrustedKeySet *trusted = ssl_get_trusted_rpk(ssl);
+ bool pin_ok = trusted != nullptr &&
SSLRPKUtils::pinnedKeyMatches(peer_rpk, *trusted);
+ Dbg(dbg_ctl_ssl_verify, "Origin authenticated with a raw public key (RFC
7250), pin match=%s", pin_ok ? "yes" : "no");
+ if (!pin_ok) {
+ char buff[INET6_ADDRSTRLEN];
+ ats_ip_ntop(netvc->get_effective_remote_addr(), buff, INET6_ADDRSTRLEN);
+ Warning("Origin raw public key did not match any trusted key. Action=%s
server=%s(%s)",
+ enforce_mode ? "Terminate" : "Continue",
netvc->options.ssl_servername.get(), buff);
+ }
+
+ // There is no X509_STORE_CTX to hand the hook for a raw public key, but
the hook still runs
+ // on every attempt, as on the X.509 paths.
+ if (tbs->verify_certificate(nullptr) == 1) {
+ Warning("TS_EVENT_SSL_VERIFY_SERVER plugin failed the origin raw public
key check for %s. Action=%s",
+ netvc->options.ssl_servername.get(), enforce_mode ? "Terminate"
: "Continue");
+ if (enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+ if (!pin_ok && enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_UNKNOWN;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ // X.509 fallback. Rebuild the chain BoringSSL hands back as CRYPTO_BUFFERs
so the shared
+ // verify_callback() logic (signature/name/policy/hook) can run against a
real X509_STORE_CTX.
+ const STACK_OF(CRYPTO_BUFFER) *chain = SSL_get0_peer_certificates(ssl);
+ if (chain == nullptr || sk_CRYPTO_BUFFER_num(chain) == 0) {
+ if (enforce_mode) {
+ *out_alert = SSL_AD_CERTIFICATE_REQUIRED;
+ return ssl_verify_invalid;
+ }
+ return ssl_verify_ok;
+ }
+
+ X509 *leaf = nullptr;
+ STACK_OF(X509) *intermediates = sk_X509_new_null();
+ if (intermediates == nullptr) {
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_verify_invalid;
+ }
+ for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(chain); i++) {
+ const CRYPTO_BUFFER *buf = sk_CRYPTO_BUFFER_value(chain, i);
+ const uint8_t *data = CRYPTO_BUFFER_data(buf);
+ X509 *cert = d2i_X509(nullptr, &data,
CRYPTO_BUFFER_len(buf));
+ if (cert == nullptr) {
+ SSLError("failed to parse an origin certificate on a RPK-enabled
connection");
+ X509_free(leaf);
+ sk_X509_pop_free(intermediates, X509_free);
+ *out_alert = SSL_AD_BAD_CERTIFICATE;
+ return ssl_verify_invalid;
+ }
+ if (i == 0) {
+ leaf = cert;
+ } else {
+ sk_X509_push(intermediates, cert);
+ }
Review Comment:
When rebuilding the X.509 chain in the client-side BoringSSL custom verify
callback, `sk_X509_push()` is not checked. If the push fails, `cert` is leaked
and verification continues with a truncated intermediates list. Check the
return value and fail the handshake with an internal error on failure.
--
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]
