Copilot commented on code in PR #13908: URL: https://github.com/apache/trafficserver/pull/13908#discussion_r4379853088
########## src/iocore/net/unit_tests/test_SNIActionPerformer.cc: ########## @@ -0,0 +1,523 @@ +/** @file + + Catch based unit tests for the SNI actions that act on the connection. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../SNIActionPerformer.h" +#include "../P_SSLNetVConnection.h" +#include "../P_SSLUtils.h" +#include "../P_UnixNetVConnection.h" +#include "iocore/net/TLSALPNSupport.h" +#include "iocore/net/TLSSNISupport.h" +#include "iocore/net/TLSTunnelSupport.h" +#include "proxy/IPAllow.h" +#include "records/RecHttp.h" +#include "tscore/ink_inet.h" + +#if TS_USE_QUIC == 1 +#include "../P_QUICNetVConnection.h" +#endif + +#include <catch2/catch_test_macros.hpp> + +#include <openssl/ssl.h> + +#include <algorithm> +#include <iterator> +#include <memory> +#include <string> +#include <type_traits> +#include <vector> + +#if TS_USE_QUIC == 1 +// The look-alike connections below stand in for QUICNetVConnection, so it must have the same shape. +static_assert(std::is_convertible_v<QUICNetVConnection *, TLSSNISupport *>); +static_assert(std::is_convertible_v<QUICNetVConnection *, NetVConnection *>); +static_assert(!std::is_base_of_v<SSLNetVConnection, QUICNetVConnection>); +#endif + +namespace +{ +IpEndpoint +make_endpoint(const char *ip) +{ + IpEndpoint ep; + + REQUIRE(ats_ip_pton(ip, &ep) == 0); + return ep; +} + +// A connection that binds TLSSNISupport and ALPNSupport but is not an SSLNetVConnection, the way +// QUICNetVConnection is. +class NonSSLNetVConnection : public UnixNetVConnection, public ALPNSupport, public TLSSNISupport +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + void + bind_to(SSL *ssl) + { + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + } + +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +// The same, but also binding TLSTunnelSupport, so nothing but the connection type stops a tunnel. +class NonSSLTunnelNetVConnection : public NonSSLNetVConnection, public TLSTunnelSupport +{ +public: + void + bind_to(SSL *ssl) + { + NonSSLNetVConnection::bind_to(ssl); + TLSTunnelSupport::bind(ssl, this); + } +}; + +class PeerSSLNetVConnection : public SSLNetVConnection +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + // What _bindSSLObject() binds for the slots the actions read. + void + bind_to(SSL *ssl) + { + this->ssl = ssl; + SSLNetVCAttach(ssl, this); + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + TLSTunnelSupport::bind(ssl, this); + } + + // The SSL object belongs to the test. + ~PeerSSLNetVConnection() override { this->ssl = nullptr; } +}; + +// TLSSNISupport bound on its own, with no connection behind it. +class DetachedSNISupport : public TLSSNISupport +{ +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +struct SSLDeleter { + void + operator()(SSL *ssl) const + { + SSL_free(ssl); + } +}; + +struct SSLCtxDeleter { + void + operator()(SSL_CTX *ctx) const + { + SSL_CTX_free(ctx); + } +}; + +class SSLObject +{ +public: + SSLObject() + { + // The support classes reserve their ex_data slots here, and they assert on a second call. + [[maybe_unused]] static bool const initialized = (SSLInitializeLibrary(), true); + + _ctx.reset(SSL_CTX_new(TLS_server_method())); + REQUIRE(_ctx != nullptr); + _ssl.reset(SSL_new(_ctx.get())); + REQUIRE(_ssl != nullptr); + } + + SSL * + get() const + { + return _ssl.get(); + } + + SSL & + operator*() const + { + return *_ssl; + } + +private: + std::unique_ptr<SSL_CTX, SSLCtxDeleter> _ctx; + std::unique_ptr<SSL, SSLDeleter> _ssl; +}; + +// IpAllow::subjects is process-wide; set it for one test and put it back afterwards. +class ScopedAclSubjects +{ +public: + ScopedAclSubjects(std::initializer_list<uint8_t> subjects) + { + std::copy(std::begin(IpAllow::subjects), std::end(IpAllow::subjects), std::begin(_saved)); + std::fill(std::begin(IpAllow::subjects), std::end(IpAllow::subjects), IpAllow::Subject::MAX_SUBJECTS); + std::copy(subjects.begin(), subjects.end(), std::begin(IpAllow::subjects)); + } + + ~ScopedAclSubjects() { std::copy(std::begin(_saved), std::end(_saved), std::begin(IpAllow::subjects)); } + +private: + uint8_t _saved[IpAllow::Subject::MAX_SUBJECTS]; +}; + +const std::string server_name{"allowed.example.com"}; +const ActionItem::Context no_context{}; + +// TunnelDestination keeps a reference to its ALPN list, so the list has to outlive the action. +const std::vector<int> no_alpn{}; +} // end anonymous namespace + +TEST_CASE("SNI_IpAllow checks the peer of a connection that is not an SSLNetVConnection", "[sni][ip_allow]") +{ + ScopedAclSubjects subjects{IpAllow::Subject::PEER}; + std::string allow_list{"10.1.0.0/16,2001:db8:1::/48"}; + SNI_IpAllow action{allow_list, server_name}; + SSLObject ssl; + auto vc = std::make_unique<NonSSLNetVConnection>(); Review Comment: In these tests, the `SSL` object is destroyed after `vc` because `ssl` is declared before `vc` (reverse destruction order). Since `bind_to()` stores raw pointers to `vc` in `SSL` ex_data, `SSL_free()` may observe dangling pointers during cleanup (now or in the future if ex_data free callbacks are added), which can lead to UAF under sanitizers. Fix by ensuring the connection outlives the `SSL` object (e.g., declare `vc` before `ssl` across the test cases, or unbind/detach all bound ex_data before destroying `vc`). ########## src/iocore/net/unit_tests/test_SNIActionPerformer.cc: ########## @@ -0,0 +1,523 @@ +/** @file + + Catch based unit tests for the SNI actions that act on the connection. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../SNIActionPerformer.h" +#include "../P_SSLNetVConnection.h" +#include "../P_SSLUtils.h" +#include "../P_UnixNetVConnection.h" +#include "iocore/net/TLSALPNSupport.h" +#include "iocore/net/TLSSNISupport.h" +#include "iocore/net/TLSTunnelSupport.h" +#include "proxy/IPAllow.h" +#include "records/RecHttp.h" +#include "tscore/ink_inet.h" + +#if TS_USE_QUIC == 1 +#include "../P_QUICNetVConnection.h" +#endif + +#include <catch2/catch_test_macros.hpp> + +#include <openssl/ssl.h> + +#include <algorithm> +#include <iterator> +#include <memory> +#include <string> +#include <type_traits> +#include <vector> + +#if TS_USE_QUIC == 1 +// The look-alike connections below stand in for QUICNetVConnection, so it must have the same shape. +static_assert(std::is_convertible_v<QUICNetVConnection *, TLSSNISupport *>); +static_assert(std::is_convertible_v<QUICNetVConnection *, NetVConnection *>); +static_assert(!std::is_base_of_v<SSLNetVConnection, QUICNetVConnection>); +#endif + +namespace +{ +IpEndpoint +make_endpoint(const char *ip) +{ + IpEndpoint ep; + + REQUIRE(ats_ip_pton(ip, &ep) == 0); + return ep; +} + +// A connection that binds TLSSNISupport and ALPNSupport but is not an SSLNetVConnection, the way +// QUICNetVConnection is. +class NonSSLNetVConnection : public UnixNetVConnection, public ALPNSupport, public TLSSNISupport +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + void + bind_to(SSL *ssl) + { + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + } + +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +// The same, but also binding TLSTunnelSupport, so nothing but the connection type stops a tunnel. +class NonSSLTunnelNetVConnection : public NonSSLNetVConnection, public TLSTunnelSupport +{ +public: + void + bind_to(SSL *ssl) + { + NonSSLNetVConnection::bind_to(ssl); + TLSTunnelSupport::bind(ssl, this); + } +}; + +class PeerSSLNetVConnection : public SSLNetVConnection +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + // What _bindSSLObject() binds for the slots the actions read. + void + bind_to(SSL *ssl) + { + this->ssl = ssl; + SSLNetVCAttach(ssl, this); + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + TLSTunnelSupport::bind(ssl, this); + } + + // The SSL object belongs to the test. + ~PeerSSLNetVConnection() override { this->ssl = nullptr; } +}; + +// TLSSNISupport bound on its own, with no connection behind it. +class DetachedSNISupport : public TLSSNISupport +{ +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +struct SSLDeleter { + void + operator()(SSL *ssl) const + { + SSL_free(ssl); + } +}; + +struct SSLCtxDeleter { + void + operator()(SSL_CTX *ctx) const + { + SSL_CTX_free(ctx); + } +}; + +class SSLObject +{ +public: + SSLObject() + { + // The support classes reserve their ex_data slots here, and they assert on a second call. + [[maybe_unused]] static bool const initialized = (SSLInitializeLibrary(), true); + + _ctx.reset(SSL_CTX_new(TLS_server_method())); + REQUIRE(_ctx != nullptr); + _ssl.reset(SSL_new(_ctx.get())); + REQUIRE(_ssl != nullptr); + } + + SSL * + get() const + { + return _ssl.get(); + } + + SSL & + operator*() const + { + return *_ssl; + } + +private: + std::unique_ptr<SSL_CTX, SSLCtxDeleter> _ctx; + std::unique_ptr<SSL, SSLDeleter> _ssl; +}; + +// IpAllow::subjects is process-wide; set it for one test and put it back afterwards. +class ScopedAclSubjects Review Comment: `IpAllow::subjects` is global process-wide state and these tests mutate it. If the test runner (or CI harness) executes Catch2 test cases concurrently, this can cause inter-test races/flakes. Consider guarding `ScopedAclSubjects` with a global mutex for the duration of a scope, or marking these tests as requiring serialized execution. ########## src/iocore/net/unit_tests/test_SNIActionPerformer.cc: ########## @@ -0,0 +1,523 @@ +/** @file + + Catch based unit tests for the SNI actions that act on the connection. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../SNIActionPerformer.h" +#include "../P_SSLNetVConnection.h" +#include "../P_SSLUtils.h" +#include "../P_UnixNetVConnection.h" +#include "iocore/net/TLSALPNSupport.h" +#include "iocore/net/TLSSNISupport.h" +#include "iocore/net/TLSTunnelSupport.h" +#include "proxy/IPAllow.h" +#include "records/RecHttp.h" +#include "tscore/ink_inet.h" + +#if TS_USE_QUIC == 1 +#include "../P_QUICNetVConnection.h" +#endif + +#include <catch2/catch_test_macros.hpp> + +#include <openssl/ssl.h> + +#include <algorithm> +#include <iterator> +#include <memory> +#include <string> +#include <type_traits> +#include <vector> + +#if TS_USE_QUIC == 1 +// The look-alike connections below stand in for QUICNetVConnection, so it must have the same shape. +static_assert(std::is_convertible_v<QUICNetVConnection *, TLSSNISupport *>); +static_assert(std::is_convertible_v<QUICNetVConnection *, NetVConnection *>); +static_assert(!std::is_base_of_v<SSLNetVConnection, QUICNetVConnection>); +#endif + +namespace +{ +IpEndpoint +make_endpoint(const char *ip) +{ + IpEndpoint ep; + + REQUIRE(ats_ip_pton(ip, &ep) == 0); + return ep; +} + +// A connection that binds TLSSNISupport and ALPNSupport but is not an SSLNetVConnection, the way +// QUICNetVConnection is. +class NonSSLNetVConnection : public UnixNetVConnection, public ALPNSupport, public TLSSNISupport +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + void + bind_to(SSL *ssl) + { + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + } + +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +// The same, but also binding TLSTunnelSupport, so nothing but the connection type stops a tunnel. +class NonSSLTunnelNetVConnection : public NonSSLNetVConnection, public TLSTunnelSupport +{ +public: + void + bind_to(SSL *ssl) + { + NonSSLNetVConnection::bind_to(ssl); + TLSTunnelSupport::bind(ssl, this); + } +}; + +class PeerSSLNetVConnection : public SSLNetVConnection +{ +public: + void + set_peer(const char *ip) + { + IpEndpoint ep = make_endpoint(ip); + + set_remote_addr(&ep.sa); + got_remote_addr = true; + } + + // What _bindSSLObject() binds for the slots the actions read. + void + bind_to(SSL *ssl) + { + this->ssl = ssl; + SSLNetVCAttach(ssl, this); + ALPNSupport::bind(ssl, this); + TLSSNISupport::bind(ssl, this); + TLSTunnelSupport::bind(ssl, this); + } + + // The SSL object belongs to the test. + ~PeerSSLNetVConnection() override { this->ssl = nullptr; } +}; + +// TLSSNISupport bound on its own, with no connection behind it. +class DetachedSNISupport : public TLSSNISupport +{ +protected: + in_port_t + _get_local_port() override + { + return 443; + } +}; + +struct SSLDeleter { + void + operator()(SSL *ssl) const + { + SSL_free(ssl); + } +}; + +struct SSLCtxDeleter { + void + operator()(SSL_CTX *ctx) const + { + SSL_CTX_free(ctx); + } +}; + +class SSLObject +{ +public: + SSLObject() + { + // The support classes reserve their ex_data slots here, and they assert on a second call. + [[maybe_unused]] static bool const initialized = (SSLInitializeLibrary(), true); + + _ctx.reset(SSL_CTX_new(TLS_server_method())); + REQUIRE(_ctx != nullptr); + _ssl.reset(SSL_new(_ctx.get())); + REQUIRE(_ssl != nullptr); + } + + SSL * + get() const + { + return _ssl.get(); + } + + SSL & + operator*() const + { + return *_ssl; + } + +private: + std::unique_ptr<SSL_CTX, SSLCtxDeleter> _ctx; + std::unique_ptr<SSL, SSLDeleter> _ssl; +}; + +// IpAllow::subjects is process-wide; set it for one test and put it back afterwards. +class ScopedAclSubjects +{ +public: + ScopedAclSubjects(std::initializer_list<uint8_t> subjects) + { + std::copy(std::begin(IpAllow::subjects), std::end(IpAllow::subjects), std::begin(_saved)); + std::fill(std::begin(IpAllow::subjects), std::end(IpAllow::subjects), IpAllow::Subject::MAX_SUBJECTS); + std::copy(subjects.begin(), subjects.end(), std::begin(IpAllow::subjects)); Review Comment: `std::copy(subjects.begin(), subjects.end(), std::begin(IpAllow::subjects))` can write past the end of `IpAllow::subjects` if an initializer_list larger than `MAX_SUBJECTS` is ever passed (even accidentally in a future test), causing memory corruption. Since this is test code, a simple fix is to add a bound (copy only up to `MAX_SUBJECTS`) and/or `REQUIRE(subjects.size() <= IpAllow::Subject::MAX_SUBJECTS)`. -- 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]
