SSL: return nullptr from SSLNetVCAccess for non-SSL handles - #13630
Draft
brbzull0 wants to merge 1 commit into
Draft
SSL: return nullptr from SSLNetVCAccess for non-SSL handles#13630brbzull0 wants to merge 1 commit into
brbzull0 wants to merge 1 commit into
Conversation
SSLNetVCAttach() is only ever called by SSLNetVConnection, so an SSL handle belonging to a QUIC connection carries no ssl_vc_index ex_data. SSLNetVCAccess() static_cast that null result to SSLNetVConnection * and checked the dynamic_cast only through ink_assert, so release builds returned it unchecked to three callbacks that QUIC also reaches. Use the dynamic_cast as the return value and guard those three callers.
Contributor
Author
|
[approve ci autest 0 2] |
Contributor
Author
|
[approve ci autest 2] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SSLNetVCAttach()(src/iocore/net/SSLUtils.cc:1994) is called from exactly oneplace,
SSLNetVConnection::SSLNetVConnection(
src/iocore/net/SSLNetVConnection.cc:145). Nothing on the QUIC side attaches,so an
SSLhandle belonging to a QUIC connection has nossl_vc_indexex_data.SSLNetVCAccess()then did:The
dynamic_castwas only evaluated insideink_assert, so in a release buildthe unchecked value was returned as-is. Three callbacks that a QUIC handshake
also reaches then used it without a null check:
SNI_IpAllow::SNIAction()--src/iocore/net/SNIActionPerformer.cc:418ssl_client_cert_callback()--src/iocore/net/SSLClientUtils.cc:211ssl_verify_client_callback()--src/iocore/net/SSLUtils.cc:200This makes the
dynamic_castthe return value, so the accessor reports a handlethat is not an
SSLNetVConnectioninstead of handing back something unchecked,and adds the corresponding guard at those three call sites.
The cast is well defined:
SSLNetVConnectionderives fromUnixNetVConnection, which derives fromNetVConnection, leftmost at bothlevels, so
NetVConnectionis the primary base subobject at offset 0 and thevoid *recovered from ex_data needs no adjustment. Theink_assertbeingremoved already performed this exact cast.
Test
No new test. Reaching the null case requires a QUIC handshake to drive an
SSL_CTXcallback shared with the TLS path, which the existing autest harnessdoes not set up directly.
Existing coverage run against this change, 8/8 pass:
tls_sni_ip_allow-- exercisesSNI_IpAllow::SNIAction()directlytls_client_verify,tls_client_verify2,tls_client_verify3--ssl_verify_client_callback()tls_client_cert,tls_client_cert_override--ssl_client_cert_callback()h3_sni_check,h3_proxy_verifier-- the QUIC pathThese confirm the non-null path through all three callbacks is unchanged.