diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 039b89ee8e8e2..7b7847c0f7f3c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -1,6 +1,11 @@ date: Pending behavior_changes: +- area: response_decoder + change: | + Updated EnvoyQuicClientStream and ResponseDecoderWrapper to use a handle to access the response decoder + to prevent use-after-free errors by ensuring the decoder instance is still live before calling its methods. + This change is guarded by the runtime flag ``envoy.reloadable_features.use_response_decoder_handle``. - area: http change: | A route refresh will now result in a tracing refresh. The trace sampling decision and decoration diff --git a/envoy/http/codec.h b/envoy/http/codec.h index 3c23ea86b2636..bc7c45cacf0c3 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -198,6 +198,23 @@ class ResponseEncoder : public virtual StreamEncoder { StreamInfo::StreamInfo& stream_info) PURE; }; +class ResponseDecoder; + +/** + * A handle to a ResponseDecoder. This handle can be used to check if the underlying decoder is + * still valid and to get a reference to it. + */ +class ResponseDecoderHandle { +public: + virtual ~ResponseDecoderHandle() = default; + + /** + * @return a reference to the underlying decoder if it is still valid. + */ + virtual OptRef get() PURE; +}; +using ResponseDecoderHandlePtr = std::unique_ptr; + /** * Decodes an HTTP stream. These are callbacks fired into a sink. This interface contains methods * common to both the request and response path. @@ -304,9 +321,16 @@ class ResponseDecoder : public virtual StreamDecoder { * @param os the ostream to dump state to * @param indent_level the depth, for pretty-printing. * + * This function is called on Envoy fatal errors so should avoid memory allocation. */ virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE; + + /** + * @return A handle to the response decoder. Caller can check the response decoder's liveness via + * the handle. + */ + virtual ResponseDecoderHandlePtr createResponseDecoderHandle() PURE; }; /** diff --git a/envoy/router/BUILD b/envoy/router/BUILD index 33af45f78fe55..47dc4cbebcbb4 100644 --- a/envoy/router/BUILD +++ b/envoy/router/BUILD @@ -95,6 +95,7 @@ envoy_cc_library( "//envoy/tracing:tracer_interface", "//envoy/upstream:resource_manager_interface", "//envoy/upstream:retry_interface", + "//source/common/http:response_decoder_impl_base", "//source/common/protobuf", "//source/common/protobuf:utility_lib", "@com_google_absl//absl/types:optional", diff --git a/envoy/router/router.h b/envoy/router/router.h index d37a0bd61968d..ef26a58dec6ca 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -27,6 +27,7 @@ #include "envoy/upstream/resource_manager.h" #include "envoy/upstream/retry.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/protobuf/protobuf.h" #include "source/common/protobuf/utility.h" @@ -1490,7 +1491,7 @@ class GenericConnPool { * An API for the interactions the upstream stream needs to have with the downstream stream * and/or router components */ -class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks { +class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { public: /** * @return return the route for the downstream stream. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index f784a9b99609c..7829e37899399 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -132,7 +132,18 @@ envoy_cc_library( envoy_cc_library( name = "codec_wrappers_lib", hdrs = ["codec_wrappers.h"], - deps = ["//envoy/http:codec_interface"], + deps = [ + ":response_decoder_impl_base", + "//envoy/http:codec_interface", + ], +) + +envoy_cc_library( + name = "response_decoder_impl_base", + hdrs = ["response_decoder_impl_base.h"], + deps = [ + "//envoy/http:codec_interface", + ], ) envoy_cc_library( diff --git a/source/common/http/codec_wrappers.h b/source/common/http/codec_wrappers.h index 7b98413e1a5a1..1494df392b06e 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -2,24 +2,35 @@ #include "envoy/http/codec.h" +#include "source/common/http/response_decoder_impl_base.h" +#include "source/common/runtime/runtime_features.h" + namespace Envoy { namespace Http { /** * Wrapper for ResponseDecoder that just forwards to an "inner" decoder. */ -class ResponseDecoderWrapper : public ResponseDecoder { +class ResponseDecoderWrapper : public ResponseDecoderImplBase { public: // ResponseDecoder void decode1xxHeaders(ResponseHeaderMapPtr&& headers) override { - inner_.decode1xxHeaders(std::move(headers)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decode1xxHeaders(std::move(headers)); + } else { + onInnerDecoderDead(); + } } void decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) override { if (end_stream) { onPreDecodeComplete(); } - inner_.decodeHeaders(std::move(headers), end_stream); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeHeaders(std::move(headers), end_stream); + } else { + onInnerDecoderDead(); + } if (end_stream) { onDecodeComplete(); } @@ -29,7 +40,11 @@ class ResponseDecoderWrapper : public ResponseDecoder { if (end_stream) { onPreDecodeComplete(); } - inner_.decodeData(data, end_stream); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeData(data, end_stream); + } else { + onInnerDecoderDead(); + } if (end_stream) { onDecodeComplete(); } @@ -37,20 +52,33 @@ class ResponseDecoderWrapper : public ResponseDecoder { void decodeTrailers(ResponseTrailerMapPtr&& trailers) override { onPreDecodeComplete(); - inner_.decodeTrailers(std::move(trailers)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeTrailers(std::move(trailers)); + } else { + onInnerDecoderDead(); + } onDecodeComplete(); } void decodeMetadata(MetadataMapPtr&& metadata_map) override { - inner_.decodeMetadata(std::move(metadata_map)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeMetadata(std::move(metadata_map)); + } else { + onInnerDecoderDead(); + } } void dumpState(std::ostream& os, int indent_level) const override { - inner_.dumpState(os, indent_level); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->dumpState(os, indent_level); + } else { + onInnerDecoderDead(); + } } protected: - ResponseDecoderWrapper(ResponseDecoder& inner) : inner_(inner) {} + ResponseDecoderWrapper(ResponseDecoder& inner) + : inner_handle_(inner.createResponseDecoderHandle()), inner_(&inner) {} /** * Consumers of the wrapper generally want to know when a decode is complete. This is called @@ -59,7 +87,29 @@ class ResponseDecoderWrapper : public ResponseDecoder { virtual void onPreDecodeComplete() PURE; virtual void onDecodeComplete() PURE; - ResponseDecoder& inner_; + ResponseDecoderHandlePtr inner_handle_; + Http::ResponseDecoder* inner_ = nullptr; + +private: + Http::ResponseDecoder* getInnerDecoder() const { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + return inner_; + } + if (inner_handle_) { + if (OptRef inner = inner_handle_->get(); inner.has_value()) { + return &inner.value().get(); + } + } + return nullptr; + } + + void onInnerDecoderDead() const { + const std::string error_msg = "Wrapped decoder use after free detected."; + IS_ENVOY_BUG(error_msg); + RELEASE_ASSERT(!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.abort_when_accessing_dead_decoder"), + error_msg); + } }; /** diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 99aea9e453b2d..5877a035f041d 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -25,8 +25,8 @@ namespace Http { namespace Http1 { ActiveClient::StreamWrapper::StreamWrapper(ResponseDecoder& response_decoder, ActiveClient& parent) - : RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), - ResponseDecoderWrapper(response_decoder), parent_(parent) { + : ResponseDecoderWrapper(response_decoder), + RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), parent_(parent) { RequestEncoderWrapper::inner_encoder_->getStream().addCallbacks(*this); } diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index 7bdeb288e6aa4..b8b1fa803dfca 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -36,8 +36,8 @@ class ActiveClient : public Envoy::Http::ActiveClient { Envoy::Http::ActiveClient::releaseResources(); } - struct StreamWrapper : public RequestEncoderWrapper, - public ResponseDecoderWrapper, + struct StreamWrapper : public ResponseDecoderWrapper, + public RequestEncoderWrapper, public StreamCallbacks, public Event::DeferredDeletable, protected Logger::Loggable { diff --git a/source/common/http/response_decoder_impl_base.h b/source/common/http/response_decoder_impl_base.h new file mode 100644 index 0000000000000..694c2b7a6af24 --- /dev/null +++ b/source/common/http/response_decoder_impl_base.h @@ -0,0 +1,40 @@ +#pragma once + +#include + +#include "envoy/http/codec.h" + +namespace Envoy { +namespace Http { + +class ResponseDecoderHandleImpl : public ResponseDecoderHandle { +public: + ResponseDecoderHandleImpl(std::weak_ptr live_trackable, ResponseDecoder& decoder) + : live_trackable_(live_trackable), decoder_(decoder) {} + + OptRef get() override { + if (live_trackable_.lock()) { + return decoder_; + } + return {}; + } + +private: + std::weak_ptr live_trackable_; + ResponseDecoder& decoder_; +}; + +class ResponseDecoderImplBase : public ResponseDecoder { +public: + ResponseDecoderImplBase() : live_trackable_(std::make_shared(true)) {} + + ResponseDecoderHandlePtr createResponseDecoderHandle() override { + return std::make_unique(live_trackable_, *this); + } + +private: + std::shared_ptr live_trackable_; +}; + +} // namespace Http +} // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 53773d3e33107..f6506a7fb33e1 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -36,6 +36,11 @@ EnvoyQuicClientStream::EnvoyQuicClientStream( RegisterMetadataVisitor(this); } +void EnvoyQuicClientStream::setResponseDecoder(Http::ResponseDecoder& decoder) { + response_decoder_handle_ = decoder.createResponseDecoderHandle(); + response_decoder_ = &decoder; +} + Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) { ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers); @@ -207,7 +212,11 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, if (!optional_status.has_value()) { // In case the status is invalid or missing, the response_decoder_.decodeHeaders() will fail the // request - response_decoder_->decodeHeaders(std::move(headers), fin); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeHeaders(std::move(headers), fin); + } else { + onResponseDecoderDead(); + } ConsumeHeaderList(); return; } @@ -224,10 +233,18 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, if (is_special_1xx && !decoded_1xx_) { // This is 100 Continue, only decode it once to support Expect:100-Continue header. decoded_1xx_ = true; - response_decoder_->decode1xxHeaders(std::move(headers)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decode1xxHeaders(std::move(headers)); + } else { + onResponseDecoderDead(); + } } else if (!is_special_1xx) { - response_decoder_->decodeHeaders(std::move(headers), - /*end_stream=*/fin); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeHeaders(std::move(headers), + /*end_stream=*/fin); + } else { + onResponseDecoderDead(); + } if (status == enumToInt(Http::Code::NotModified)) { got_304_response_ = true; } @@ -301,7 +318,11 @@ void EnvoyQuicClientStream::OnBodyAvailable() { // A stream error has occurred, stop processing. return; } - response_decoder_->decodeData(*buffer, fin_read_and_no_trailers); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeData(*buffer, fin_read_and_no_trailers); + } else { + onResponseDecoderDead(); + } } if (!sequencer()->IsClosed() || read_side_closed()) { @@ -348,7 +369,11 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { onStreamError(close_connection_upon_invalid_header_, transform_rst); return; } - response_decoder_->decodeTrailers(std::move(trailers)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeTrailers(std::move(trailers)); + } else { + onResponseDecoderDead(); + } MarkTrailersConsumed(); } } @@ -432,10 +457,15 @@ void EnvoyQuicClientStream::OnMetadataComplete(size_t /*frame_len*/, const quic::QuicHeaderList& header_list) { if (mustRejectMetadata(header_list.uncompressed_header_bytes())) { onStreamError(true, quic::QUIC_HEADERS_TOO_LARGE); + return; } if (!header_list.empty()) { - response_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeMetadata(metadataMapFromHeaderList(header_list)); + } else { + onResponseDecoderDead(); + } } } @@ -466,7 +496,7 @@ bool EnvoyQuicClientStream::hasPendingData() { return BufferedDataBytes() > 0; } // connect-udp". void EnvoyQuicClientStream::useCapsuleProtocol() { http_datagram_handler_ = std::make_unique(*this); - http_datagram_handler_->setStreamDecoder(response_decoder_); + http_datagram_handler_->setStreamDecoder(getResponseDecoder()); RegisterHttp3DatagramVisitor(http_datagram_handler_.get()); } #endif @@ -475,5 +505,26 @@ void EnvoyQuicClientStream::OnInvalidHeaders() { onStreamError(absl::nullopt, quic::QUIC_BAD_APPLICATION_PAYLOAD); } +void EnvoyQuicClientStream::onResponseDecoderDead() const { + const std::string error_msg = "response_decoder_ use after free detected."; + IS_ENVOY_BUG(error_msg); + RELEASE_ASSERT(!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.abort_when_accessing_dead_decoder"), + error_msg); +} + +Http::ResponseDecoder* EnvoyQuicClientStream::getResponseDecoder() { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + return response_decoder_; + } + if (response_decoder_handle_) { + if (OptRef decoder = response_decoder_handle_->get(); + decoder.has_value()) { + return &decoder.value().get(); + } + } + return nullptr; +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 67214eb9741f2..5352f2b92bea5 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -3,6 +3,7 @@ #include "envoy/buffer/buffer.h" #include "source/common/quic/envoy_quic_stream.h" +#include "source/common/runtime/runtime_features.h" #ifdef ENVOY_ENABLE_HTTP_DATAGRAMS #include "source/common/quic/http_datagram_handler.h" @@ -25,7 +26,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options); - void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; } + void setResponseDecoder(Http::ResponseDecoder& decoder); // Http::StreamEncoder Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { @@ -92,6 +93,12 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, void useCapsuleProtocol(); #endif + // Returns nullptr if the response decoder has already been destructed. + Http::ResponseDecoder* getResponseDecoder(); + + void onResponseDecoderDead() const; + + Http::ResponseDecoderHandlePtr response_decoder_handle_; Http::ResponseDecoder* response_decoder_{nullptr}; bool decoded_1xx_{false}; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b25a151963428..e56f20700f779 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -79,6 +79,7 @@ RUNTIME_GUARD(envoy_reloadable_features_trace_refresh_after_route_refresh); RUNTIME_GUARD(envoy_reloadable_features_udp_set_do_not_fragment); RUNTIME_GUARD(envoy_reloadable_features_uhv_allow_malformed_url_encoding); RUNTIME_GUARD(envoy_reloadable_features_uri_template_match_on_asterisk); +RUNTIME_GUARD(envoy_reloadable_features_use_response_decoder_handle); RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); RUNTIME_GUARD(envoy_reloadable_features_websocket_allow_4xx_5xx_through_filter_chain); @@ -171,7 +172,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_getaddrinfo_no_ai_flags); FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_new_dns_implementation); // Force a local reply from upstream envoy for reverse connections. FALSE_RUNTIME_GUARD(envoy_reloadable_features_reverse_conn_force_local_reply); - +// RELEASE_ASSERT when upstream stream detects UAF of downstream response decoder instance. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_abort_when_accessing_dead_decoder); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD index c6b30b08d03d1..914382ab15f81 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -29,6 +29,7 @@ envoy_cc_library( "//source/common/http:hash_policy_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/http:utility_lib", "//source/common/network:utility_lib", "//source/common/router:header_parser_lib", diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h index 661797dc65de0..86a770a8517bd 100644 --- a/source/common/tcp_proxy/upstream.h +++ b/source/common/tcp_proxy/upstream.h @@ -17,6 +17,7 @@ #include "source/common/http/codec_client.h" #include "source/common/http/hash_policy.h" #include "source/common/http/null_route_impl.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/network/utility.h" #include "source/common/router/config_impl.h" #include "source/common/router/header_parser.h" @@ -230,8 +231,7 @@ class HttpUpstream : public GenericUpstream, protected Http::StreamCallbacks { std::unique_ptr downstream_headers_; private: - Upstream::ClusterInfoConstSharedPtr cluster_; - class DecoderShim : public Http::ResponseDecoder { + class DecoderShim : public Http::ResponseDecoderImplBase { public: DecoderShim(HttpUpstream& parent) : parent_(parent) {} void decode1xxHeaders(Http::ResponseHeaderMapPtr&&) override {} @@ -351,7 +351,7 @@ class CombinedUpstream : public GenericUpstream, public Envoy::Router::RouterFil private: Http::StreamDecoderFilterCallbacks& decoder_filter_callbacks_; - class DecoderShim : public Http::ResponseDecoder { + class DecoderShim : public Http::ResponseDecoderImplBase { public: DecoderShim(CombinedUpstream& parent) : parent_(parent) {} // Http::ResponseDecoder diff --git a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD index cef2da998723f..62257e221fcc2 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD @@ -77,6 +77,7 @@ envoy_cc_library( "//source/common/common:logger_lib", "//source/common/grpc:typed_async_client_lib", "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/http/http1:codec_lib", "//source/common/http/http1:codec_stats_lib", "//source/common/network:address_lib", diff --git a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h index 68828336a43d7..23dbacb1db6c0 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h @@ -12,6 +12,7 @@ #include "source/common/common/logger.h" #include "source/common/http/http1/codec_impl.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/network/filter_impl.h" namespace Envoy { @@ -49,7 +50,7 @@ class SimpleConnReadFilter : public Network::ReadFilterBaseImpl, class RCConnectionWrapper : public Network::ConnectionCallbacks, public Event::DeferredDeletable, public Logger::Loggable, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::ConnectionCallbacks { friend class SimpleConnReadFilterTest; diff --git a/source/extensions/filters/udp/udp_proxy/BUILD b/source/extensions/filters/udp/udp_proxy/BUILD index a9e79751a3afc..9e60d1f00f3f0 100644 --- a/source/extensions/filters/udp/udp_proxy/BUILD +++ b/source/extensions/filters/udp/udp_proxy/BUILD @@ -42,6 +42,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:linked_object", "//source/common/common:random_generator_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/network:socket_lib", "//source/common/network:socket_option_factory_lib", "//source/common/network:utility_lib", diff --git a/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h b/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h index f2b6f64606fa9..c1c921375f269 100644 --- a/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h +++ b/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h @@ -21,6 +21,7 @@ #include "source/common/http/codes.h" #include "source/common/http/header_map_impl.h" #include "source/common/http/headers.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/http/utility.h" #include "source/common/network/socket_impl.h" #include "source/common/network/socket_interface.h" @@ -311,7 +312,7 @@ class HttpUpstreamImpl : public HttpUpstream, protected Http::StreamCallbacks { } private: - class ResponseDecoder : public Http::ResponseDecoder { + class ResponseDecoder : public Http::ResponseDecoderImplBase { public: ResponseDecoder(HttpUpstreamImpl& parent) : parent_(parent) {} diff --git a/source/extensions/health_checkers/grpc/BUILD b/source/extensions/health_checkers/grpc/BUILD index c06822d4e75c9..b3688169d1f2d 100644 --- a/source/extensions/health_checkers/grpc/BUILD +++ b/source/extensions/health_checkers/grpc/BUILD @@ -20,6 +20,7 @@ envoy_cc_extension( deps = [ "//source/common/grpc:codec_lib", "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/upstream:health_checker_lib", "//source/common/upstream:host_utility_lib", "//source/extensions/health_checkers/common:health_checker_base_lib", diff --git a/source/extensions/health_checkers/grpc/health_checker_impl.h b/source/extensions/health_checkers/grpc/health_checker_impl.h index 326035eac59b1..a3e54bb454792 100644 --- a/source/extensions/health_checkers/grpc/health_checker_impl.h +++ b/source/extensions/health_checkers/grpc/health_checker_impl.h @@ -17,6 +17,7 @@ #include "source/common/common/logger.h" #include "source/common/grpc/codec.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/router/header_parser.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/upstream/health_checker_impl.h" @@ -52,7 +53,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase { private: struct GrpcActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { GrpcActiveHealthCheckSession(GrpcHealthCheckerImpl& parent, const HostSharedPtr& host); ~GrpcActiveHealthCheckSession() override; diff --git a/source/extensions/health_checkers/http/BUILD b/source/extensions/health_checkers/http/BUILD index 6ad5ed7773564..b759a789d7568 100644 --- a/source/extensions/health_checkers/http/BUILD +++ b/source/extensions/health_checkers/http/BUILD @@ -18,6 +18,7 @@ envoy_cc_extension( ], deps = [ "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/upstream:health_checker_lib", "//source/common/upstream:host_utility_lib", "//source/extensions/health_checkers/common:health_checker_base_lib", diff --git a/source/extensions/health_checkers/http/health_checker_impl.h b/source/extensions/health_checkers/http/health_checker_impl.h index 81fae53f73330..68fd6214f9166 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.h +++ b/source/extensions/health_checkers/http/health_checker_impl.h @@ -17,6 +17,7 @@ #include "source/common/common/logger.h" #include "source/common/grpc/codec.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/router/header_parser.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/upstream/health_checker_impl.h" @@ -77,7 +78,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { private: struct HttpActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { HttpActiveHealthCheckSession(HttpHealthCheckerImpl& parent, const HostSharedPtr& host); ~HttpActiveHealthCheckSession() override; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 868182b92b8d8..2aae452aa1dcc 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -162,6 +162,7 @@ envoy_cc_test( deps = [ "//source/common/http:codec_wrappers_lib", "//test/mocks/http:http_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/http/codec_wrappers_test.cc b/test/common/http/codec_wrappers_test.cc index 35bf742a03f02..e5ffb0acdd615 100644 --- a/test/common/http/codec_wrappers_test.cc +++ b/test/common/http/codec_wrappers_test.cc @@ -1,6 +1,7 @@ #include "source/common/http/codec_wrappers.h" #include "test/mocks/http/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" using testing::_; @@ -10,23 +11,54 @@ namespace Http { class MockResponseDecoderWrapper : public ResponseDecoderWrapper { public: - MockResponseDecoderWrapper() : ResponseDecoderWrapper(inner_decoder_) {} - MockResponseDecoder& innerEncoder() { return inner_decoder_; } + explicit MockResponseDecoderWrapper(MockResponseDecoder& inner_decoder) + : ResponseDecoderWrapper(inner_decoder) {} void onDecodeComplete() override {} void onPreDecodeComplete() override {} - -private: - MockResponseDecoder inner_decoder_; }; TEST(MockResponseDecoderWrapper, dumpState) { - MockResponseDecoderWrapper wrapper; + MockResponseDecoder inner_decoder; + MockResponseDecoderWrapper wrapper(inner_decoder); std::stringstream os; - EXPECT_CALL(wrapper.innerEncoder(), dumpState(_, _)); + EXPECT_CALL(inner_decoder, dumpState(_, _)); wrapper.dumpState(os, 0); } +TEST(MockResponseDecoderWrapper, decoderDestroyedBeforeDecoding) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto inner_decoder = std::make_unique(); + MockResponseDecoderWrapper wrapper(*inner_decoder); + + inner_decoder.reset(); + + EXPECT_ENVOY_BUG( + wrapper.decodeHeaders(ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, + true), + "Wrapped decoder use after free detected"); + + EXPECT_ENVOY_BUG(wrapper.decode1xxHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "100"}}}), + "Wrapped decoder use after free detected"); + + Buffer::OwnedImpl data("foo"); + EXPECT_ENVOY_BUG(wrapper.decodeData(data, true), "Wrapped decoder use after free detected"); + + EXPECT_ENVOY_BUG(wrapper.decodeTrailers( + ResponseTrailerMapPtr{new TestResponseTrailerMapImpl{{"key", "value"}}}), + "Wrapped decoder use after free detected"); + + MetadataMapPtr metadata = std::make_unique(); + (*metadata)["key1"] = "value1"; + EXPECT_ENVOY_BUG(wrapper.decodeMetadata(std::move(metadata)), + "Wrapped decoder use after free detected"); + + std::stringstream os; + EXPECT_ENVOY_BUG(wrapper.dumpState(os, 0), "Wrapped decoder use after free detected"); +} + class MockRequestEncoderWrapper : public RequestEncoderWrapper { public: MockRequestEncoderWrapper() : RequestEncoderWrapper(&inner_encoder_) {} diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index bc61e050ff7d6..5a2202a9921a8 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -152,6 +152,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@com_github_google_quiche//:quic_core_http_spdy_session_lib", "@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib", diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 473dd22af5e05..4522e5d788ff0 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_decoder.h" #include "test/mocks/network/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -741,6 +742,115 @@ TEST_F(EnvoyQuicClientStreamTest, EncodeTrailersOnClosedStream) { EXPECT_EQ(0u, quic_session_.bytesToSend()); } +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecoding1xxHeader) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + // Destroy the mock decoder. + stream_decoder.reset(); + + quiche::HttpHeaderBlock continue_header; + continue_header[":status"] = "100"; + std::string headers = spdyHeaderToHttp3StreamPayload(continue_header); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame1), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingHeader) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame1), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingBody) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + EXPECT_CALL(*stream_decoder, decodeHeaders_(_, /*end_stream=*/false)); + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + quic_stream_->OnStreamFrame(frame1); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string body = bodyToHttp3StreamPayload("body"); + quic::QuicStreamFrame frame2(stream_id_, /*fin*/ false, headers.length(), body); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame2), + "response_decoder_ use after free detected"); + + std::string trailers = spdyHeaderToHttp3StreamPayload(spdy_trailers_); + quic::QuicStreamFrame frame3(stream_id_, true, (headers.length() + body.length()), trailers); + quic_stream_->OnStreamFrame(frame3); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingTrailer) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + EXPECT_CALL(*stream_decoder, decodeHeaders_(_, /*end_stream=*/false)); + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + quic_stream_->OnStreamFrame(frame1); + + EXPECT_CALL(*stream_decoder, decodeData(_, /*end_stream=*/false)); + std::string body = bodyToHttp3StreamPayload("body"); + quic::QuicStreamFrame frame2(stream_id_, /*fin*/ false, headers.length(), body); + quic_stream_->OnStreamFrame(frame2); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string trailers = spdyHeaderToHttp3StreamPayload(spdy_trailers_); + quic::QuicStreamFrame frame3(stream_id_, true, (headers.length() + body.length()), trailers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame3), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + #ifdef ENVOY_ENABLE_HTTP_DATAGRAMS TEST_F(EnvoyQuicClientStreamTest, EncodeCapsule) { setUpCapsuleProtocol(false, true); diff --git a/test/integration/integration_stream_decoder.h b/test/integration/integration_stream_decoder.h index 9654ce9e824ee..ce83293aed3a0 100644 --- a/test/integration/integration_stream_decoder.h +++ b/test/integration/integration_stream_decoder.h @@ -10,6 +10,7 @@ #include "envoy/http/metadata_interface.h" #include "source/common/common/dump_state_utils.h" +#include "source/common/http/response_decoder_impl_base.h" #include "test/test_common/utility.h" @@ -21,7 +22,8 @@ namespace Envoy { /** * Stream decoder wrapper used during integration testing. */ -class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::StreamCallbacks { +class IntegrationStreamDecoder : public Http::ResponseDecoderImplBase, + public Http::StreamCallbacks { public: IntegrationStreamDecoder(Event::Dispatcher& dispatcher); ~IntegrationStreamDecoder() override; diff --git a/test/integration/utility.h b/test/integration/utility.h index 39c2c664a3d0a..9db2c3d6d0bea 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -17,6 +17,7 @@ #include "source/common/common/dump_state_utils.h" #include "source/common/common/utility.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/stats/isolated_store_impl.h" #include "test/test_common/printers.h" @@ -29,7 +30,7 @@ namespace Envoy { /** * A buffering response decoder used for testing. */ -class BufferingStreamDecoder : public Http::ResponseDecoder, public Http::StreamCallbacks { +class BufferingStreamDecoder : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { public: BufferingStreamDecoder(std::function on_complete_cb) : on_complete_cb_(on_complete_cb) {} diff --git a/test/mocks/http/BUILD b/test/mocks/http/BUILD index 416d71fe5a2c3..af75f948d3445 100644 --- a/test/mocks/http/BUILD +++ b/test/mocks/http/BUILD @@ -104,6 +104,7 @@ envoy_cc_mock( hdrs = ["stream_decoder.h"], deps = [ "//envoy/http:codec_interface", + "//source/common/http:response_decoder_impl_base", ], ) diff --git a/test/mocks/http/stream_decoder.h b/test/mocks/http/stream_decoder.h index e1c8585a49a3c..2f265b158b82e 100644 --- a/test/mocks/http/stream_decoder.h +++ b/test/mocks/http/stream_decoder.h @@ -1,6 +1,8 @@ #pragma once #include "envoy/http/codec.h" +#include "source/common/http/response_decoder_impl_base.h" + #include "gmock/gmock.h" namespace Envoy { @@ -43,7 +45,7 @@ class MockRequestDecoder : public RequestDecoder { MOCK_METHOD(RequestDecoderHandlePtr, getRequestDecoderHandle, ()); }; -class MockResponseDecoder : public ResponseDecoder { +class MockResponseDecoder : public ResponseDecoderImplBase { public: MockResponseDecoder(); ~MockResponseDecoder() override; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 2fbc45cc57e1c..d2bd26af0b031 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -493,6 +493,7 @@ TTLs TX TXT UA +UAF UBSAN UDP UDS @@ -973,6 +974,7 @@ linkability linkable linux livelock +liveness llvm loc localhost