fuzz: avoid false positives in HCM fuzzer. (#4262)

htuch · web-flow · commit 0057e22d9028 · 2018-08-29T17:43:11.000-04:00
Previously we were not respecting the encodeHeaders() :status contract or the connection close callbacks. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10025. Risk level: Low Testing: Corpus entries added. Signed-off-by: Harvey Tuch <htuch@google.com>
diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h
@@ -28,7 +28,8 @@ class StreamEncoder {
   virtual void encode100ContinueHeaders(const HeaderMap& headers) PURE;
 
   /**
-   * Encode headers, optionally indicating end of stream.
+   * Encode headers, optionally indicating end of stream. Response headers must
+   * have a valid :status set.
    * @param headers supplies the header map to encode.
    * @param end_stream supplies whether this is a header only request/response.
    */
diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h
@@ -251,6 +251,11 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
     using StreamImpl::StreamImpl;
 
     // StreamImpl
+    void encodeHeaders(const HeaderMap& headers, bool end_stream) override {
+      // The contract is that client codecs must ensure that :status is present.
+      ASSERT(headers.Status() != nullptr);
+      StreamImpl::encodeHeaders(headers, end_stream);
+    }
     void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
                        nghttp2_data_provider* provider) override;
     void transformUpgradeFromH1toH2(HeaderMap& headers) override {
diff --git a/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5638706466652160 b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5638706466652160
@@ -0,0 +1,43 @@
+actions {
+  new_stream {
+    request_headers {
+      headers {
+        key: ":method"
+      }
+      headers {
+        key: "foo"
+        value: "/"
+      }
+      headers {
+        key: "cookie"
+      }
+      headers {
+        key: "/"
+        value: "foo.com"
+      }
+      headers {
+        key: "/"
+        value: "GT"
+      }
+      headers {
+        value: "/"
+      }
+    }
+  }
+}
+actions {
+  stream_action {
+    response {
+      headers {
+      }
+    }
+  }
+}
+actions {
+}
+actions {
+}
+actions {
+}
+actions {
+}
diff --git a/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5679723404328960 b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5679723404328960
@@ -0,0 +1,53 @@
+actions {
+}
+actions {
+  stream_action {
+  }
+}
+actions {
+  new_stream {
+    request_headers {
+      headers {
+        key: ":method"
+        value: "GET"
+      }
+      headers {
+        key: ":path"
+        value: "/"
+      }
+      headers {
+        key: ":scheme"
+        value: "GET"
+      }
+      headers {
+        key: ":authority"
+        value: "foo.com"
+      }
+      headers {
+        value: "nosniff"
+      }
+      headers {
+        key: "GET"
+        value: "foo=bar"
+      }
+      headers {
+        key: "cookie"
+        value: "foo2=bar2"
+      }
+    }
+  }
+}
+actions {
+  stream_action {
+    response {
+      headers {
+      }
+    }
+  }
+}
+actions {
+}
+actions {
+}
+actions {
+}
diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc
@@ -301,9 +301,16 @@ class FuzzStream {
     }
     case test::common::http::ResponseAction::kHeaders: {
       if (state == StreamState::PendingHeaders) {
-        decoder_filter_->callbacks_->encodeHeaders(
-            std::make_unique<TestHeaderMapImpl>(Fuzz::fromHeaders(response_action.headers())),
-            end_stream);
+        auto headers =
+            std::make_unique<TestHeaderMapImpl>(Fuzz::fromHeaders(response_action.headers()));
+        // The client codec will ensure we always have a valid :status.
+        // Similarly, local replies should always contain this.
+        try {
+          Utility::getResponseStatus(*headers);
+        } catch (const CodecClientException&) {
+          headers->setReferenceKey(Headers::get().Status, "200");
+        }
+        decoder_filter_->callbacks_->encodeHeaders(std::move(headers), end_stream);
         state = end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers;
       }
       break;
@@ -368,9 +375,12 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
   NiceMock<Upstream::MockClusterManager> cluster_manager;
   NiceMock<Network::MockReadFilterCallbacks> filter_callbacks;
   std::unique_ptr<Ssl::MockConnection> ssl_connection;
+  bool connection_alive = true;
 
   ON_CALL(filter_callbacks.connection_, ssl()).WillByDefault(Return(ssl_connection.get()));
   ON_CALL(Const(filter_callbacks.connection_), ssl()).WillByDefault(Return(ssl_connection.get()));
+  ON_CALL(filter_callbacks.connection_, close(_))
+      .WillByDefault(InvokeWithoutArgs([&connection_alive] { connection_alive = false; }));
   filter_callbacks.connection_.local_address_ =
       std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1");
   filter_callbacks.connection_.remote_address_ =
@@ -384,6 +394,11 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
 
   for (const auto& action : input.actions()) {
     ENVOY_LOG_MISC(trace, "action {} with {} streams", action.DebugString(), streams.size());
+    if (!connection_alive) {
+      ENVOY_LOG_MISC(trace, "skipping due to dead connection");
+      break;
+    }
+
     switch (action.action_selector_case()) {
     case test::common::http::Action::kNewStream: {
       streams.emplace_back(new FuzzStream(conn_manager, config,
