codec: reject embedded NUL in headers. (#2)

http-parser doesn't sanitize header values as per RFC 7230 today, so add an
additional check (yielding a CodecProtocolException) for this. See CVE-2019-9900 for further details.

Added CR/LF, in addition to NUL, as prohibited in header strings as per
RFC 7230.

Also added codec_impl_tests for H1 and H2 codecs to validate that
NUL/LF/CR mutations in a request don't violate internal invariants.

Fixes CVE-2019-9900 and oss-fuzz issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13613.

Risk level: Low
Testing: Corpus entry and unit test added.

Signed-off-by: Harvey Tuch <htuch@google.com>

docs/root/intro/version_history.rst

@@ -85,6 +85,10 @@ Version history
 * upstream: added configuration option to select any host when the fallback policy fails.
 * upstream: stopped incrementing upstream_rq_total for HTTP/1 conn pool when request is circuit broken.
 
+1.9.1 (Apr 2, 2019)
+===================
+* http: fixed CVE-2019-9900 by rejecting HTTP/1.x headers with embedded NUL characters.
+
 1.9.0 (Dec 20, 2018)
 ====================
 * access log: added a :ref:`JSON logging mode <config_access_log_format_dictionaries>` to output access logs in JSON format.

include/envoy/http/header_map.h

@@ -20,6 +20,17 @@
 namespace Envoy {
 namespace Http {
 
+// Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
+// never contain embedded NULLs.
+static inline bool validHeaderString(absl::string_view s) {
+  for (const char c : {'\0', '\r', '\n'}) {
+    if (s.find(c) != absl::string_view::npos) {
+      return false;
+    }
+  }
+  return true;
+}
+
 /**
  * Wrapper for a lower case string used in header operations to generally avoid needless case
  * insensitive compares.
@@ -40,9 +51,7 @@ class LowerCaseString {
 
 private:
   void lower() { std::transform(string_.begin(), string_.end(), string_.begin(), tolower); }
-  // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
-  // never contain embedded NULLs.
-  bool valid() const { return string_.find('\0') == std::string::npos; }
+  bool valid() const { return validHeaderString(string_); }
 
   std::string string_;
 };
@@ -183,8 +192,6 @@ class HeaderString {
   };
 
   void freeDynamic();
-  // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
-  // never contain embedded NULLs.
   bool valid() const;
 
   uint32_t string_length_;

source/common/http/header_map_impl.cc

@@ -90,9 +90,7 @@ void HeaderString::freeDynamic() {
   }
 }
 
-bool HeaderString::valid() const {
-  return std::string(c_str(), string_length_).find('\0') == std::string::npos;
-}
+bool HeaderString::valid() const { return validHeaderString(getStringView()); }
 
 void HeaderString::append(const char* data, uint32_t size) {
   switch (type_) {

source/common/http/http1/codec_impl.cc

@@ -418,6 +418,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
     // Ignore trailers.
     return;
   }
+  // http-parser should filter for this
+  // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings
+  // have an invariant that they must not contain embedded zero characters
+  // (NUL, ASCII 0x0).
+  if (absl::string_view(data, length).find('\0') != absl::string_view::npos) {
+    throw CodecProtocolException("http/1.1 protocol error: header value contains NUL");
+  }
 
   header_parsing_state_ = HeaderParsingState::Value;
   current_header_value_.append(data, length);

test/common/http/http1/codec_impl_test.cc

@@ -289,6 +289,67 @@ TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) {
   EXPECT_EQ(0U, buffer.length());
 }
 
+// Regression test for http-parser allowing embedded NULs in header values,
+// verify we reject them.
+TEST_F(Http1ServerConnectionImplTest, HeaderEmbeddedNulRejection) {
+  initialize();
+
+  InSequence sequence;
+
+  Http::MockStreamDecoder decoder;
+  EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+  Buffer::OwnedImpl buffer(
+      absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: bar", std::string(1, '\0'), "baz\r\n"));
+  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException,
+                            "http/1.1 protocol error: header value contains NUL");
+}
+
+// Mutate an HTTP GET with embedded NULs, this should always be rejected in some
+// way (not necessarily with "head value contains NUL" though).
+TEST_F(Http1ServerConnectionImplTest, HeaderMutateEmbeddedNul) {
+  const std::string example_input = "GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: barbaz\r\n";
+
+  for (size_t n = 1; n < example_input.size(); ++n) {
+    initialize();
+
+    InSequence sequence;
+
+    Http::MockStreamDecoder decoder;
+    EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+    Buffer::OwnedImpl buffer(
+        absl::StrCat(example_input.substr(0, n), std::string(1, '\0'), example_input.substr(n)));
+    EXPECT_THROW_WITH_REGEX(codec_->dispatch(buffer), CodecProtocolException,
+                            "http/1.1 protocol error:");
+  }
+}
+
+// Mutate an HTTP GET with CR or LF. These can cause an exception or maybe
+// result in a valid decodeHeaders(). In any case, the validHeaderString()
+// ASSERTs should validate we never have any embedded CR or LF.
+TEST_F(Http1ServerConnectionImplTest, HeaderMutateEmbeddedCRLF) {
+  const std::string example_input = "GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: barbaz\r\n";
+
+  for (const char c : {'\r', '\n'}) {
+    for (size_t n = 1; n < example_input.size(); ++n) {
+      initialize();
+
+      InSequence sequence;
+
+      NiceMock<Http::MockStreamDecoder> decoder;
+      EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+      Buffer::OwnedImpl buffer(
+          absl::StrCat(example_input.substr(0, n), std::string(1, c), example_input.substr(n)));
+      try {
+        codec_->dispatch(buffer);
+      } catch (CodecProtocolException) {
+      }
+    }
+  }
+}
+
 TEST_F(Http1ServerConnectionImplTest, CloseDuringHeadersComplete) {
   initialize();
 

test/common/http/http2/codec_impl_test.cc

@@ -34,7 +34,13 @@ namespace Http2 {
 using Http2SettingsTuple = ::testing::tuple<uint32_t, uint32_t, uint32_t, uint32_t>;
 using Http2SettingsTestParam = ::testing::tuple<Http2SettingsTuple, Http2SettingsTuple>;
 
-class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam> {
+constexpr Http2SettingsTuple
+    DefaultHttp2SettingsTuple(Http2Settings::DEFAULT_HPACK_TABLE_SIZE,
+                              Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS,
+                              Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS,
+                              Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE);
+
+class Http2CodecImplTestFixture {
 public:
   struct ConnectionWrapper {
     void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) {
@@ -52,9 +58,13 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
     Buffer::OwnedImpl buffer_;
   };
 
-  void initialize() {
-    Http2SettingsFromTuple(client_http2settings_, ::testing::get<0>(GetParam()));
-    Http2SettingsFromTuple(server_http2settings_, ::testing::get<1>(GetParam()));
+  Http2CodecImplTestFixture(Http2SettingsTuple client_settings, Http2SettingsTuple server_settings)
+      : client_settings_(client_settings), server_settings_(server_settings) {}
+  virtual ~Http2CodecImplTestFixture() {}
+
+  virtual void initialize() {
+    Http2SettingsFromTuple(client_http2settings_, client_settings_);
+    Http2SettingsFromTuple(server_http2settings_, server_settings_);
     client_ = std::make_unique<TestClientConnectionImpl>(client_connection_, client_callbacks_,
                                                          stats_store_, client_http2settings_,
                                                          max_request_headers_kb_);
@@ -76,8 +86,11 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
   void setupDefaultConnectionMocks() {
     ON_CALL(client_connection_, write(_, _))
         .WillByDefault(Invoke([&](Buffer::Instance& data, bool) -> void {
-          if (corrupt_data_) {
-            corruptFramePayload(data);
+          if (corrupt_metadata_frame_) {
+            corruptMetadataFramePayload(data);
+          }
+          if (corrupt_at_offset_ >= 0) {
+            corruptAtOffset(data, corrupt_at_offset_, corrupt_with_char_);
           }
           server_wrapper_.dispatch(data, *server_);
         }));
@@ -95,22 +108,27 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
     setting.allow_metadata_ = allow_metadata_;
   }
 
-  // corruptFramePayload assumes data contains at least 10 bytes of the beginning of a frame.
-  void corruptFramePayload(Buffer::Instance& data) {
+  // corruptMetadataFramePayload assumes data contains at least 10 bytes of the beginning of a
+  // frame.
+  void corruptMetadataFramePayload(Buffer::Instance& data) {
     const size_t length = data.length();
     const size_t corrupt_start = 10;
     if (length < corrupt_start || length > METADATA_MAX_PAYLOAD_SIZE) {
       ENVOY_LOG_MISC(error, "data size too big or too small");
       return;
     }
-    uint8_t buf[METADATA_MAX_PAYLOAD_SIZE] = {0};
-    data.copyOut(0, length, static_cast<void*>(buf));
-    data.drain(length);
-    // Keeps the frame header (9 bytes) valid, and corrupts the payload.
-    buf[10] |= 0xff;
-    data.add(buf, length);
+    corruptAtOffset(data, corrupt_start, 0xff);
+  }
+
+  void corruptAtOffset(Buffer::Instance& data, size_t index, char new_value) {
+    if (data.length() == 0) {
+      return;
+    }
+    reinterpret_cast<uint8_t*>(data.linearize(data.length()))[index % data.length()] = new_value;
   }
 
+  const Http2SettingsTuple client_settings_;
+  const Http2SettingsTuple server_settings_;
   bool allow_metadata_ = false;
   Stats::IsolatedStoreImpl stats_store_;
   Http2Settings client_http2settings_;
@@ -128,10 +146,22 @@ class Http2CodecImplTest : public testing::TestWithParam<Http2SettingsTestParam>
   MockStreamDecoder request_decoder_;
   StreamEncoder* response_encoder_{};
   MockStreamCallbacks server_stream_callbacks_;
-  bool corrupt_data_ = false;
+  // Corrupt a metadata frame payload.
+  bool corrupt_metadata_frame_ = false;
+  // Corrupt frame at a given offset (if positive).
+  ssize_t corrupt_at_offset_{-1};
+  char corrupt_with_char_{'\0'};
+
   uint32_t max_request_headers_kb_ = Http::DEFAULT_MAX_REQUEST_HEADERS_KB;
 };
 
+class Http2CodecImplTest : public ::testing::TestWithParam<Http2SettingsTestParam>,
+                           protected Http2CodecImplTestFixture {
+public:
+  Http2CodecImplTest()
+      : Http2CodecImplTestFixture(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam())) {}
+};
+
 TEST_P(Http2CodecImplTest, ShutdownNotice) {
   initialize();
 
@@ -191,7 +221,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) {
   client_wrapper_.dispatch(Buffer::OwnedImpl(), *client_);
 
   EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value());
-};
+}
 
 TEST_P(Http2CodecImplTest, InvalidRepeatContinue) {
   initialize();
@@ -242,7 +272,7 @@ TEST_P(Http2CodecImplTest, Invalid103) {
   EXPECT_THROW_WITH_MESSAGE(response_encoder_->encodeHeaders(early_hint_headers, false),
                             CodecProtocolException, "Unexpected 'trailers' with no end stream.");
   EXPECT_EQ(1, stats_store_.counter("http2.too_many_header_frames").value());
-};
+}
 
 TEST_P(Http2CodecImplTest, Invalid204WithContentLength) {
   initialize();
@@ -444,7 +474,7 @@ TEST_P(Http2CodecImplTest, BadMetadataVecReceivedTest) {
   MetadataMapVector metadata_map_vector;
   metadata_map_vector.push_back(std::move(metadata_map_ptr));
 
-  corrupt_data_ = true;
+  corrupt_metadata_frame_ = true;
   EXPECT_THROW_WITH_MESSAGE(request_encoder_->encodeMetadata(metadata_map_vector), EnvoyException,
                             "The user callback function failed");
 }
@@ -1011,6 +1041,50 @@ TEST_P(Http2CodecImplTest, TestCodecHeaderCompression) {
   }
 }
 
+// Validate that nghttp2 rejects NUL/CR/LF as per
+// https://httpwg.org/specs/rfc7540.html#rfc.section.10.3.
+// TEST_P(Http2CodecImplTest, InvalidHeaderChars) {
+// TODO(htuch): Write me. Http2CodecImplMutationTest basically covers this,
+// but we could be a bit more specific and add a captured H2 HEADERS frame
+// here and inject it with mutation of just the header value, ensuring we get
+// the expected codec exception.
+// }
+
+class Http2CodecImplMutationTest : public ::testing::TestWithParam<::testing::tuple<char, int>>,
+                                   protected Http2CodecImplTestFixture {
+public:
+  Http2CodecImplMutationTest()
+      : Http2CodecImplTestFixture(DefaultHttp2SettingsTuple, DefaultHttp2SettingsTuple) {}
+
+  void initialize() override {
+    corrupt_with_char_ = ::testing::get<0>(GetParam());
+    corrupt_at_offset_ = ::testing::get<1>(GetParam());
+    Http2CodecImplTestFixture::initialize();
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(Http2CodecImplMutationTest, Http2CodecImplMutationTest,
+                         ::testing::Combine(::testing::ValuesIn({'\0', '\r', '\n'}),
+                                            ::testing::Range(0, 128)));
+
+// Mutate an arbitrary offset in the HEADERS frame with NUL/CR/LF. This should
+// either throw an exception or continue, but we shouldn't crash due to
+// validHeaderString() ASSERTs.
+TEST_P(Http2CodecImplMutationTest, HandleInvalidChars) {
+  initialize();
+
+  TestHeaderMapImpl request_headers;
+  request_headers.addCopy("foo", "barbaz");
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(AnyNumber());
+  EXPECT_CALL(client_callbacks_, onGoAway()).Times(AnyNumber());
+  try {
+    request_encoder_->encodeHeaders(request_headers, true);
+  } catch (const CodecProtocolException& e) {
+    ENVOY_LOG_MISC(trace, "CodecProtocolException: {}", e.what());
+  }
+}
+
 } // namespace Http2
 } // namespace Http
 } // namespace Envoy

test/extensions/filters/http/gzip/gzip_filter_test.cc

@@ -208,7 +208,7 @@ TEST_F(GzipFilterTest, isAcceptEncodingAllowed) {
   }
   {
     Http::TestHeaderMapImpl headers = {
-        {"accept-encoding", "\tdeflate\t, gzip\t ; q\t =\t 1.0,\t * ;q=0.5\n"}};
+        {"accept-encoding", "\tdeflate\t, gzip\t ; q\t =\t 1.0,\t * ;q=0.5"}};
     EXPECT_TRUE(isAcceptEncodingAllowed(headers));
     EXPECT_EQ(3, stats_.counter("test.gzip.header_gzip").value());
   }
@@ -416,7 +416,7 @@ TEST_F(GzipFilterTest, isContentTypeAllowed) {
     EXPECT_TRUE(isContentTypeAllowed(headers));
   }
   {
-    Http::TestHeaderMapImpl headers = {{"content-type", "\ttext/html\t\n"}};
+    Http::TestHeaderMapImpl headers = {{"content-type", "\ttext/html\t"}};
     EXPECT_TRUE(isContentTypeAllowed(headers));
   }
 
@@ -588,7 +588,7 @@ TEST_F(GzipFilterTest, isTransferEncodingAllowed) {
     EXPECT_FALSE(isTransferEncodingAllowed(headers));
   }
   {
-    Http::TestHeaderMapImpl headers = {{"transfer-encoding", " gzip\t,  chunked\t\n"}};
+    Http::TestHeaderMapImpl headers = {{"transfer-encoding", " gzip\t,  chunked\t"}};
     EXPECT_FALSE(isTransferEncodingAllowed(headers));
   }
 }

test/integration/h1_corpus/embed_null.pb_text

@@ -0,0 +1 @@
+events {   downstream_send_bytes: "POST   /\nntnt:   � \0     " }

tools/spelling_dictionary.txt

@@ -137,6 +137,7 @@ LC
 LDS
 LEV
 LHS
+LF
 MB
 MD
 MGET