From b9ca1fa22f924410f79d9b470999422b1d27b76f Mon Sep 17 00:00:00 2001 From: htuch Date: Fri, 24 Apr 2020 19:53:13 -0400 Subject: [PATCH] hcm: avoid invoking 100-continue handling on decode filter. (#10929) The 100-continue state tracking variables were checking in commonContinue() (on both decode/encode paths), conditioning do100ContinueHeaders(). This makes no sense on the decode path, and can lead to crashes as per #10923 when the decode pipeline is resumed, so refactored the logic out to just the encode path. Risk level: Low Testing: Unit and integration regression tests added, as well as corpus entry. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18461 Fixes #10923 Signed-off-by: Harvey Tuch --- source/common/http/conn_manager_impl.cc | 2 +- source/common/http/conn_manager_impl.h | 5 + ...nn_manager_impl_fuzz_test-5674283828772864 | 120 ++++++++++++++++++ test/common/http/conn_manager_impl_test.cc | 65 ++++++++++ test/integration/BUILD | 1 + test/integration/integration_test.cc | 10 ++ 6 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5674283828772864 diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b4dddcb478ff..75c8bed64cd2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -2067,7 +2067,7 @@ void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() { allowIteration(); // Only resume with do100ContinueHeaders() if we've actually seen a 100-Continue. - if (parent_.state_.has_continue_headers_ && !continue_headers_continued_) { + if (has100Continueheaders()) { continue_headers_continued_ = true; do100ContinueHeaders(); // If the response headers have not yet come in, don't continue on with diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 1e3705b57634..705f60e27e4c 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -139,6 +139,7 @@ class ConnectionManagerImpl : Logger::Loggable, virtual Buffer::WatermarkBufferPtr createBuffer() PURE; virtual Buffer::WatermarkBufferPtr& bufferedData() PURE; virtual bool complete() PURE; + virtual bool has100Continueheaders() PURE; virtual void do100ContinueHeaders() PURE; virtual void doHeaders(bool end_stream) PURE; virtual void doData(bool end_stream) PURE; @@ -237,6 +238,7 @@ class ConnectionManagerImpl : Logger::Loggable, Buffer::WatermarkBufferPtr createBuffer() override; Buffer::WatermarkBufferPtr& bufferedData() override { return parent_.buffered_request_data_; } bool complete() override { return parent_.state_.remote_complete_; } + bool has100Continueheaders() override { return false; } void do100ContinueHeaders() override { NOT_REACHED_GCOVR_EXCL_LINE; } void doHeaders(bool end_stream) override { parent_.decodeHeaders(this, *parent_.request_headers_, end_stream); @@ -349,6 +351,9 @@ class ConnectionManagerImpl : Logger::Loggable, Buffer::WatermarkBufferPtr createBuffer() override; Buffer::WatermarkBufferPtr& bufferedData() override { return parent_.buffered_response_data_; } bool complete() override { return parent_.state_.local_complete_; } + bool has100Continueheaders() override { + return parent_.state_.has_continue_headers_ && !continue_headers_continued_; + } void do100ContinueHeaders() override { parent_.encode100ContinueHeaders(this, *parent_.continue_headers_); } diff --git a/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5674283828772864 b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5674283828772864 new file mode 100644 index 000000000000..64081c27010c --- /dev/null +++ b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5674283828772864 @@ -0,0 +1,120 @@ +actions { + stream_action { + request { + trailers { + headers { + headers { + key: "foo" + value: "bar" + } + } + decoder_filter_callback_action { + add_decoded_data { + size: 1000000 + } + } + } + } + } +} +actions { + new_stream { + request_headers { + headers { + key: ":method" + value: "GET" + } + headers { + key: ":path" + value: "/" + } + headers { + key: ":scheme" + value: "http" + } + headers { + key: ":authority" + value: "foo.com" + } + headers { + key: "blah" + value: "nosniff" + } + headers { + key: "cookie" + value: "foo=bar" + } + headers { + key: "cookie" + value: "foo2=bar2" + } + } + } +} +actions { + stream_action { + request { + data { + size: 3000000 + status: DATA_STOP_ITERATION_AND_BUFFER + decoder_filter_callback_action { + add_decoded_data { + size: 1000000 + } + } + } + } + } +} +actions { + stream_action { + response { + trailers { + headers { + key: "foo" + value: "bar" + } + } + } + } +} +actions { + stream_action { + stream_id: 5505024 + } +} +actions { + stream_action { + response { + continue_headers { + } + } + } +} +actions { + stream_action { + request { + continue_decoding { + } + } + } +} +actions { + stream_action { + response { + data: 5 + } + } +} +actions { + stream_action { + response { + headers { + headers { + key: ":status" + value: "200" + } + } + } + } +} diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 38593d73ab3e..4853bb378c7e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -617,6 +617,71 @@ TEST_F(HttpConnectionManagerImplTest, PauseResume100Continue) { decoder_filters_[1]->callbacks_->encodeHeaders(std::move(response_headers), false); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10923. +TEST_F(HttpConnectionManagerImplTest, 100ContinueResponseWithDecoderPause) { + proxy_100_continue_ = true; + setup(false, "envoy-custom-server", false); + + std::shared_ptr filter(new NiceMock()); + + // Allow headers to pass. + EXPECT_CALL(*filter, decodeHeaders(_, false)) + .WillRepeatedly( + InvokeWithoutArgs([]() -> FilterHeadersStatus { return FilterHeadersStatus::Continue; })); + // Pause and then resume the decode pipeline, this is key to triggering #10923. + EXPECT_CALL(*filter, decodeData(_, false)).WillOnce(InvokeWithoutArgs([]() -> FilterDataStatus { + return FilterDataStatus::StopIterationAndBuffer; + })); + EXPECT_CALL(*filter, decodeData(_, true)) + .WillRepeatedly( + InvokeWithoutArgs([]() -> FilterDataStatus { return FilterDataStatus::Continue; })); + + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + })); + + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + RequestDecoder* decoder = nullptr; + NiceMock encoder; + EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + + // Test not charging stats on the second call. + RequestHeaderMapPtr headers{ + new TestRequestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; + decoder->decodeHeaders(std::move(headers), false); + // Allow the decode pipeline to pause. + decoder->decodeData(data, false); + + ResponseHeaderMapPtr continue_headers{new TestResponseHeaderMapImpl{{":status", "100"}}}; + filter->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); + + // Resume decode pipeline after encoding 100 continue headers, we're now ready to trigger + // #10923. + decoder->decodeData(data, true); + + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{{":status", "200"}}}; + filter->callbacks_->encodeHeaders(std::move(response_headers), true); + + data.drain(4); + })); + + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + EXPECT_EQ(1U, stats_.named_.downstream_rq_1xx_.value()); + EXPECT_EQ(1U, listener_stats_.downstream_rq_1xx_.value()); + EXPECT_EQ(1U, stats_.named_.downstream_rq_2xx_.value()); + EXPECT_EQ(1U, listener_stats_.downstream_rq_2xx_.value()); + EXPECT_EQ(2U, stats_.named_.downstream_rq_completed_.value()); + EXPECT_EQ(2U, listener_stats_.downstream_rq_completed_.value()); +} + // By default, Envoy will set the server header to the server name, here "custom-value" TEST_F(HttpConnectionManagerImplTest, ServerHeaderOverwritten) { setup(false, "custom-value", false); diff --git a/test/integration/BUILD b/test/integration/BUILD index ad714ee65167..5c3d08b0ef16 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -596,6 +596,7 @@ envoy_cc_test( "//test/integration/filters:clear_route_cache_filter_lib", "//test/integration/filters:encoder_decoder_buffer_filter_lib", "//test/integration/filters:process_context_lib", + "//test/integration/filters:stop_iteration_and_continue", "//test/mocks/http:http_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 1ae41217b1a0..3cddecf87bfd 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -256,6 +256,16 @@ TEST_P(IntegrationTest, EnvoyProxyingLate100ContinueWithEncoderFilter) { testEnvoyProxying100Continue(false, true); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10923. +TEST_P(IntegrationTest, EnvoyProxying100ContinueWithDecodeDataPause) { + config_helper_.addFilter(R"EOF( + name: stop-iteration-and-continue-filter + typed_config: + "@type": type.googleapis.com/google.protobuf.Empty + )EOF"); + testEnvoyProxying100Continue(true); +} + // This is a regression for https://github.com/envoyproxy/envoy/issues/2715 and validates that a // pending request is not sent on a connection that has been half-closed. TEST_P(IntegrationTest, UpstreamDisconnectWithTwoRequests) {