From b3f42a4ebbd51e816cfde63ee672ce31e420602a Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Tue, 10 Dec 2019 06:34:19 -0500 Subject: [PATCH] http: fix heap overflow vulnerability (CVE-2019-18801). (#75) Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18431. Risk level: Low (no functional change). Testing: Additional unit and integration tests added that cover the :method header overflow case and adjacent behaviors. Corpus entry added. Signed-off-by: Harvey Tuch --- source/common/http/http1/codec_impl.cc | 2 +- ...ized-codec_impl_fuzz_test-5726642969772032 | 12 +++ test/common/http/http1/codec_impl_test.cc | 93 +++++++++++++++++++ test/common/http/http2/codec_impl_test.cc | 19 ++++ test/integration/fake_upstream.h | 17 +++- test/integration/protocol_integration_test.cc | 49 ++++++++++ tools/check_spelling.sh | 2 +- tools/spelling_skip_files.txt | 2 +- 8 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 8f2d33d29d37..6e0ea8f1ea86 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -305,7 +305,7 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ head_request_ = true; } connection_.onEncodeHeaders(headers); - connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); + connection_.reserveBuffer(path->value().size() + method->value().size() + 4096); connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); diff --git a/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 new file mode 100644 index 000000000000..f8d96b4c26af --- /dev/null +++ b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 @@ -0,0 +1,12 @@ +actions { new_stream { request_headers { headers { key: ":method" value: " _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_s ke n key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _h new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _h new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke he ke new_st e new_st kr ke st e new_st t_he ket_he ke new_st e new_st kr e new_st t_he ke new_st e new_st kr ke n key:e ke newstrey:_]E]u___ }\n}," + } + headers { + key: ":method" + value: "GETactions {\n muta{\n ketruest_he key: ctions {\n ers {\n headers {\n key: ctions {\n new_streamTnrtasfTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amrtasfer-headers {headers {\n headers {u new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n headers headers {u new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti headers {u new_stream {asfer-e key: ctioew: r-e oew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headest_heade headers {u key: cti headers {u new_stream {asfer-e key: ctioew: r-e oew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headers {@ headers {u new_stream {asfer-e key: ctioew: \"Tnrtasfer-e key: ction: cti new_stream {reew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headers {@ headers {u new_stream {asfer-e key: ctioew: \"Tnrtasfer-e key: ction: cti new_stream {\n new_streame: s {\n new_streamTnrtasfe " + } + headers { + key: ":path" + } + } + } +} diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index f6fde28db2a2..5a71a953efaa 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -358,6 +358,22 @@ TEST_F(Http1ServerConnectionImplTest, BadRequestNoStream) { EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); } +// This behavior was observed during CVE-2019-18801 and helped to limit the +// scope of affected Envoy configurations. +TEST_F(Http1ServerConnectionImplTest, RejectInvalidMethod) { + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + Buffer::OwnedImpl buffer("BAD / HTTP/1.1\r\nHost: foo\r\n"); + EXPECT_THROW(codec_->dispatch(buffer), CodecProtocolException); + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); +} + TEST_F(Http1ServerConnectionImplTest, BadRequestStartedStream) { initialize(); @@ -559,6 +575,33 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponse) { EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); } +// As with Http1ClientConnectionImplTest.LargeHeaderRequestEncode but validate +// the response encoder instead of request encoder. +TEST_F(Http1ServerConnectionImplTest, LargeHeaderResponseEncode) { + initialize(); + + NiceMock decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + const std::string long_header_value = std::string(79 * 1024, 'a'); + TestHeaderMapImpl headers{{":status", "200"}, {"foo", long_header_value}}; + response_encoder->encodeHeaders(headers, true); + EXPECT_EQ("HTTP/1.1 200 OK\r\nfoo: " + long_header_value + "\r\ncontent-length: 0\r\n\r\n", + output); +} + TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseTrainProperHeaders) { codec_settings_.header_key_format_ = Http1Settings::HeaderKeyFormat::ProperCase; initialize(); @@ -1428,6 +1471,56 @@ TEST_F(Http1ClientConnectionImplTest, LargeResponseHeadersAccepted) { codec_->dispatch(buffer); } +// Regression test for CVE-2019-18801. Large method headers should not trigger +// ASSERTs or ASAN, which they previously did. +TEST_F(Http1ClientConnectionImplTest, LargeMethodRequestEncode) { + initialize(); + + NiceMock response_decoder; + const std::string long_method = std::string(79 * 1024, 'a'); + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + TestHeaderMapImpl headers{{":method", long_method}, {":path", "/"}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ(long_method + " / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); +} + +// As with LargeMethodEncode, but for the path header. This was not an issue +// in CVE-2019-18801, but the related code does explicit size calculations on +// both path and method (these are the two distinguished headers). So, +// belt-and-braces. +TEST_F(Http1ClientConnectionImplTest, LargePathRequestEncode) { + initialize(); + + NiceMock response_decoder; + const std::string long_path = std::string(79 * 1024, '/'); + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + TestHeaderMapImpl headers{{":method", "GET"}, {":path", long_path}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ("GET " + long_path + " HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); +} + +// As with LargeMethodEncode, but for an arbitrary header. This was not an issue +// in CVE-2019-18801. +TEST_F(Http1ClientConnectionImplTest, LargeHeaderRequestEncode) { + initialize(); + + NiceMock response_decoder; + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + const std::string long_header_value = std::string(79 * 1024, 'a'); + TestHeaderMapImpl headers{ + {":method", "GET"}, {"foo", long_header_value}, {":path", "/"}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\nfoo: " + long_header_value + + "\r\ncontent-length: 0\r\n\r\n", + output); +} + // Exception called when the number of response headers exceeds the default value of 100. TEST_F(Http1ClientConnectionImplTest, ManyResponseHeadersRejected) { initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c2784c8dcf19..be111b5a8a19 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1090,6 +1090,25 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAccepted) { request_encoder_->encodeHeaders(request_headers, false); } +// This is the HTTP/2 variant of the HTTP/1 regression test for CVE-2019-18801. +// Large method headers should not trigger ASSERTs or ASAN. The underlying issue +// in CVE-2019-18801 only affected the HTTP/1 encoder, but we include a test +// here for belt-and-braces. This also demonstrates that the HTTP/2 codec will +// accept arbitrary :method headers, unlike the HTTP/1 codec (see +// Http1ServerConnectionImplTest.RejectInvalidMethod for comparison). +TEST_P(Http2CodecImplTest, LargeMethodRequestEncode) { + max_request_headers_kb_ = 80; + initialize(); + + const std::string long_method = std::string(79 * 1024, 'a'); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_headers.setReferenceKey(Headers::get().Method, long_method); + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&request_headers), false)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); + request_encoder_->encodeHeaders(request_headers, false); +} + // Tests stream reset when the number of request headers exceeds the default maximum of 100. TEST_P(Http2CodecImplTest, ManyRequestHeadersInvokeResetStream) { initialize(); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 0405b820d1af..0c044bcb46cf 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -24,6 +24,7 @@ #include "common/common/thread.h" #include "common/grpc/codec.h" #include "common/grpc/common.h" +#include "common/http/exception.h" #include "common/network/connection_balancer_impl.h" #include "common/network/filter_impl.h" #include "common/network/listen_socket_impl.h" @@ -437,10 +438,24 @@ class FakeHttpConnection : public Http::ServerConnectionCallbacks, public FakeCo // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool) override { - parent_.codec_->dispatch(data); + try { + parent_.codec_->dispatch(data); + } catch (const Http::CodecProtocolException& e) { + ENVOY_LOG(debug, "FakeUpstream dispatch error: {}", e.what()); + // We don't do a full stream shutdown like HCM, but just shutdown the + // connection for now. + read_filter_callbacks_->connection().close( + Network::ConnectionCloseType::FlushWriteAndDelay); + } return Network::FilterStatus::StopIteration; } + void + initializeReadFilterCallbacks(Network::ReadFilterCallbacks& read_filter_callbacks) override { + read_filter_callbacks_ = &read_filter_callbacks; + } + + Network::ReadFilterCallbacks* read_filter_callbacks_{}; FakeHttpConnection& parent_; }; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e2bc8f8d3cd6..31093c8b0e24 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1065,6 +1065,55 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { EXPECT_EQ("200", response->headers().Status()->value().getStringView()); } +// Regression tests for CVE-2019-18801. We only validate the behavior of large +// :method request headers, since the case of other large headers is +// covered in the various testLargeRequest-based integration tests here. +// +// The table below describes the expected behaviors (in addition we should never +// see an ASSERT or ASAN failure trigger). +// +// Downstream Upstream Behavior expected +// ------------------------------------------ +// H1 H1 Envoy will reject (HTTP/1 codec behavior) +// H1 H2 Envoy will reject (HTTP/1 codec behavior) +// H2 H1 Envoy will forward but backend will reject (HTTP/1 +// codec behavior) +// H2 H2 Success +TEST_P(ProtocolIntegrationTest, LargeRequestMethod) { + const std::string long_method = std::string(48 * 1024, 'a'); + const Http::TestHeaderMapImpl request_headers{{":method", long_method}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + auto encoder_decoder = codec_client_->startRequest(request_headers); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->waitForDisconnect(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT(downstreamProtocol() == Http::CodecClient::Type::HTTP2); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + ASSERT_TRUE( + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT(upstreamProtocol() == FakeHttpConnection::Type::HTTP2); + auto response = + sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + EXPECT_TRUE(response->complete()); + } + } +} + // Tests StopAllIterationAndBuffer. Verifies decode-headers-return-stop-all-filter calls decodeData // once after iteration is resumed. TEST_P(DownstreamProtocolIntegrationTest, testDecodeHeadersReturnsStopAll) { diff --git a/tools/check_spelling.sh b/tools/check_spelling.sh index 7fa31c5b314d..7597e80e7777 100755 --- a/tools/check_spelling.sh +++ b/tools/check_spelling.sh @@ -78,5 +78,5 @@ SPELLING_WHITELIST_WORDS_FILE="${ROOTDIR}/tools/spelling_whitelist_words.txt" WHITELIST_WORDS=$(echo -n $(cat "${SPELLING_WHITELIST_WORDS_FILE}" | \ grep -v "^#"|grep -v "^$") | tr ' ' ',') SKIP_FILES=$(echo $(cat "${SPELLING_SKIP_FILES}") | sed "s| | -e |g") -git ls-files | grep -v -e "${SKIP_FILES}" | xargs "${TMP_DIR}/misspell" -i \ +git ls-files | grep -v -e ${SKIP_FILES} | xargs "${TMP_DIR}/misspell" -i \ "${WHITELIST_WORDS}" ${MISSPELL_ARGS} diff --git a/tools/spelling_skip_files.txt b/tools/spelling_skip_files.txt index 6ddcc10646b8..8351c72345a4 100644 --- a/tools/spelling_skip_files.txt +++ b/tools/spelling_skip_files.txt @@ -1 +1 @@ -OWNERS.md +OWNERS.md corpus