[fuzz] fix invalid header related issues in fuzz tests (envoyproxy#10933

) * fix invalid header related issues in fuzz tests Signed-off-by: Asra Ali <asraa@google.com> * add comment Signed-off-by: Asra Ali <asraa@google.com> * Add comments, TODO to remove adding host Signed-off-by: Asra Ali <asraa@google.com> * address comments Signed-off-by: Asra Ali <asraa@google.com>
adisuissa · May 4, 2020 · f2b81c1 · f2b81c1
1 parent fd67664
commit f2b81c1
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 10 deletions.
diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h
@@ -85,8 +85,7 @@ class StreamEncoder {
 class RequestEncoder : public virtual StreamEncoder {
 public:
   /**
-   * Encode headers, optionally indicating end of stream. Response headers must
-   * have a valid :status set.
+   * Encode headers, optionally indicating end of stream.
    * @param headers supplies the header map to encode.
    * @param end_stream supplies whether this is a header only request.
    */

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -362,6 +362,9 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end
   bool is_connect = HeaderUtility::isConnect(headers);
 
   if (!method || (!path && !is_connect)) {
+    // TODO(#10878): This exception does not occur during dispatch and would not be triggered under
+    // normal circumstances since inputs would fail parsing at ingress. Replace with proper error
+    // handling when exceptions are removed. Include missing host header for CONNECT.
     throw CodecClientException(":method and :path must be specified");
   }
   if (method->value() == Headers::get().MethodValues.Head) {

diff --git a/test/common/http/codec_impl_corpus/method_connect b/test/common/http/codec_impl_corpus/method_connect
diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc
@@ -42,6 +42,17 @@ template <class T> T fromSanitizedHeaders(const test::fuzz::Headers& headers) {
   return Fuzz::fromHeaders<T>(headers, {"transfer-encoding"});
 }
 
+// Template specialization for TestRequestHeaderMapImpl to include a Host header. This guards
+// against missing host headers in CONNECT requests that would have failed parsing on ingress.
+// TODO(#10878): When proper error handling is introduced for non-dispatching codec calls, remove
+// this and fail gracefully.
+template <>
+TestRequestHeaderMapImpl
+fromSanitizedHeaders<TestRequestHeaderMapImpl>(const test::fuzz::Headers& headers) {
+  return Fuzz::fromHeaders<TestRequestHeaderMapImpl>(headers, {"transfer-encoding"},
+                                                     {":authority"});
+}
+
 // Convert from test proto Http1ServerSettings to Http1Settings.
 Http1Settings fromHttp1Settings(const test::common::http::Http1ServerSettings& settings) {
   Http1Settings h1_settings;

diff --git a/test/common/http/conn_manager_impl_corpus/invalid_host b/test/common/http/conn_manager_impl_corpus/invalid_host
diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc
@@ -523,7 +523,8 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
     case test::common::http::Action::kNewStream: {
       streams.emplace_back(new FuzzStream(
           conn_manager, config,
-          Fuzz::fromHeaders<TestRequestHeaderMapImpl>(action.new_stream().request_headers()),
+          Fuzz::fromHeaders<TestRequestHeaderMapImpl>(action.new_stream().request_headers(),
+                                                      /* ignore_headers =*/{}, {":authority"}),
           action.new_stream().status(), action.new_stream().end_stream()));
       break;
     }

diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h
@@ -12,6 +12,8 @@
 #include "test/mocks/upstream/host.h"
 #include "test/test_common/utility.h"
 
+#include "nghttp2/nghttp2.h"
+
 // Strong assertion that applies across all compilation modes and doesn't rely
 // on gtest, which only provides soft fails that don't trip oss-fuzz failures.
 #define FUZZ_ASSERT(x) RELEASE_ASSERT(x, "")
@@ -49,13 +51,11 @@ inline std::string replaceInvalidCharacters(absl::string_view string) {
 inline std::string replaceInvalidHostCharacters(absl::string_view string) {
   std::string filtered;
   filtered.reserve(string.length());
-  for (const char& c : string) {
-    switch (c) {
-    case ' ':
+  for (const uint8_t* c = reinterpret_cast<const uint8_t*>(string.data()); *c; ++c) {
+    if (nghttp2_check_authority(c, 1)) {
+      filtered.push_back(*c);
+    } else {
       filtered.push_back('0');
-      break;
-    default:
-      filtered.push_back(c);
     }
   }
   return filtered;
@@ -83,12 +83,18 @@ replaceInvalidStringValues(const envoy::config::core::v3::Metadata& upstream_met
 template <class T>
 inline T fromHeaders(
     const test::fuzz::Headers& headers,
-    const std::unordered_set<std::string>& ignore_headers = std::unordered_set<std::string>()) {
+    const std::unordered_set<std::string>& ignore_headers = std::unordered_set<std::string>(),
+    std::unordered_set<std::string> include_headers = std::unordered_set<std::string>()) {
   T header_map;
   for (const auto& header : headers.headers()) {
     if (ignore_headers.find(absl::AsciiStrToLower(header.key())) == ignore_headers.end()) {
       header_map.addCopy(header.key(), header.value());
     }
+    include_headers.erase(absl::AsciiStrToLower(header.key()));
+  }
+  // Add dummy headers for non-present headers that must be included.
+  for (const auto& header : include_headers) {
+    header_map.addCopy(header, "dummy");
   }
   return header_map;
 }