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 <htuch@google.com>

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());

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<Http::MockStreamDecoder> 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<Http::MockStreamDecoder> 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<Http::MockStreamDecoder> 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<Http::MockStreamDecoder> 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();

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();

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_;
   };
 

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) {

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}

tools/spelling_skip_files.txt

@@ -1 +1 @@
-OWNERS.md
+OWNERS.md corpus