From e2cd2cf4ed07341e89386d39064e7de323d2e0e8 Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 22 Sep 2020 16:46:55 +0000 Subject: [PATCH 1/3] Fixed crashing fuzz test Signed-off-by: Zach --- ...ed-health_check_fuzz_test-5678121129607168 | 28 +++++++++++++++++++ test/common/upstream/health_check_fuzz.cc | 12 +++++++- 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 diff --git a/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 b/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 new file mode 100644 index 000000000000..4a542c0ca9b0 --- /dev/null +++ b/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 @@ -0,0 +1,28 @@ +health_check_config { + timeout { + seconds: 8960 + } + interval { + seconds: 26624 + } + unhealthy_threshold { + value: 524288 + } + healthy_threshold { + value: 2147483652 + } + http_health_check { + path: "\003" + } + event_log_path: "(" + interval_jitter_percent: 654311422 +} +actions { + respond { + http_respond { + status: "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + } + } +} +http_verify_cluster: true +start_failed: true diff --git a/test/common/upstream/health_check_fuzz.cc b/test/common/upstream/health_check_fuzz.cc index 4c38253aed84..233cc2707087 100644 --- a/test/common/upstream/health_check_fuzz.cc +++ b/test/common/upstream/health_check_fuzz.cc @@ -48,7 +48,6 @@ void HealthCheckFuzz::initializeAndReplay(test::common::upstream::HealthCheckTes } void HealthCheckFuzz::respondHttp(const test::fuzz::Headers& headers, absl::string_view status) { - // Timeout timer needs to be explicitly enabled, usually by onIntervalBase() (Callback on interval // timer). if (!test_sessions_[0]->timeout_timer_->enabled_) { @@ -62,6 +61,17 @@ void HealthCheckFuzz::respondHttp(const test::fuzz::Headers& headers, absl::stri response_headers->setStatus(status); + // This utility gets called in codec impl which handles any exceptions thrown, so check here to + // constrain state space to what can pass through codec. + // TODO(zasweq): However, this utility function gets called in a lot of places around the codebase + // that may take in untrusted inputs. + try { + Http::Utility::getResponseStatus(*response_headers.get()); + } catch (EnvoyException e) { + ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what()); + return; + } + // Responding with http can cause client to close, if so create a new one. bool client_will_close = false; if (response_headers->Connection()) { From 73c33c82c131923293dddbf5780e729c10eed6d7 Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 23 Sep 2020 16:38:47 +0000 Subject: [PATCH 2/3] Switched status to uint64 Signed-off-by: Zach --- .../upstream/health_check_corpus/ConnectionClose | 2 +- test/common/upstream/health_check_corpus/Degraded | 4 ++-- test/common/upstream/health_check_corpus/LargeNanos | 2 +- .../health_check_corpus/RemoteCloseBetweenChecks | 4 ++-- test/common/upstream/health_check_corpus/Success | 2 +- .../SuccessStartFailedSuccessFirst | 2 +- .../upstream/health_check_corpus/TimeoutThenSuccess | 2 +- .../upstream/health_check_corpus/ZeroRetryInterval | 2 +- ...inimized-health_check_fuzz_test-5678121129607168 | 2 +- .../crash-daebc8c8bcb985b777d6fa462a265ba5cdd8b06e | 2 +- test/common/upstream/health_check_corpus/crash_1 | 2 +- test/common/upstream/health_check_corpus/crash_3 | 4 ++-- test/common/upstream/health_check_corpus/crash_4 | 2 +- test/common/upstream/health_check_corpus/crash_5 | 2 +- test/common/upstream/health_check_fuzz.cc | 13 +------------ test/common/upstream/health_check_fuzz.h | 4 +++- test/common/upstream/health_check_fuzz.proto | 3 +-- 17 files changed, 22 insertions(+), 32 deletions(-) diff --git a/test/common/upstream/health_check_corpus/ConnectionClose b/test/common/upstream/health_check_corpus/ConnectionClose index 6bbe0e9a4d4f..ea9a56eaec00 100644 --- a/test/common/upstream/health_check_corpus/ConnectionClose +++ b/test/common/upstream/health_check_corpus/ConnectionClose @@ -33,7 +33,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/Degraded b/test/common/upstream/health_check_corpus/Degraded index f06170c16581..6b14f2ea5011 100644 --- a/test/common/upstream/health_check_corpus/Degraded +++ b/test/common/upstream/health_check_corpus/Degraded @@ -37,7 +37,7 @@ actions { value: "true" } } - status: "200" + status: 200 } } } @@ -55,7 +55,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/LargeNanos b/test/common/upstream/health_check_corpus/LargeNanos index a2bb7b689dfe..b42bf8b69935 100644 --- a/test/common/upstream/health_check_corpus/LargeNanos +++ b/test/common/upstream/health_check_corpus/LargeNanos @@ -38,7 +38,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/RemoteCloseBetweenChecks b/test/common/upstream/health_check_corpus/RemoteCloseBetweenChecks index f658d3dcb54c..f2451f4ca664 100644 --- a/test/common/upstream/health_check_corpus/RemoteCloseBetweenChecks +++ b/test/common/upstream/health_check_corpus/RemoteCloseBetweenChecks @@ -33,7 +33,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } @@ -49,7 +49,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/Success b/test/common/upstream/health_check_corpus/Success index 598b61c9d1b1..6fd5e4e4aa42 100644 --- a/test/common/upstream/health_check_corpus/Success +++ b/test/common/upstream/health_check_corpus/Success @@ -33,7 +33,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/SuccessStartFailedSuccessFirst b/test/common/upstream/health_check_corpus/SuccessStartFailedSuccessFirst index 4a2622fba177..0d15e0234748 100644 --- a/test/common/upstream/health_check_corpus/SuccessStartFailedSuccessFirst +++ b/test/common/upstream/health_check_corpus/SuccessStartFailedSuccessFirst @@ -33,7 +33,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/TimeoutThenSuccess b/test/common/upstream/health_check_corpus/TimeoutThenSuccess index 1253b7f282ca..3232d919d94b 100644 --- a/test/common/upstream/health_check_corpus/TimeoutThenSuccess +++ b/test/common/upstream/health_check_corpus/TimeoutThenSuccess @@ -38,7 +38,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/ZeroRetryInterval b/test/common/upstream/health_check_corpus/ZeroRetryInterval index 27a3fd923dbf..31122ee9c05f 100644 --- a/test/common/upstream/health_check_corpus/ZeroRetryInterval +++ b/test/common/upstream/health_check_corpus/ZeroRetryInterval @@ -34,7 +34,7 @@ actions { value: "locations-production-iad" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 b/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 index 4a542c0ca9b0..b1530f2bc1d2 100644 --- a/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 +++ b/test/common/upstream/health_check_corpus/clusterfuzz-testcase-minimized-health_check_fuzz_test-5678121129607168 @@ -20,7 +20,7 @@ health_check_config { actions { respond { http_respond { - status: "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + status: 11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 } } } diff --git a/test/common/upstream/health_check_corpus/crash-daebc8c8bcb985b777d6fa462a265ba5cdd8b06e b/test/common/upstream/health_check_corpus/crash-daebc8c8bcb985b777d6fa462a265ba5cdd8b06e index d3274aee1ec4..84db13fb3b45 100644 --- a/test/common/upstream/health_check_corpus/crash-daebc8c8bcb985b777d6fa462a265ba5cdd8b06e +++ b/test/common/upstream/health_check_corpus/crash-daebc8c8bcb985b777d6fa462a265ba5cdd8b06e @@ -33,7 +33,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/crash_1 b/test/common/upstream/health_check_corpus/crash_1 index a207f92d479f..7028ea0a18d3 100644 --- a/test/common/upstream/health_check_corpus/crash_1 +++ b/test/common/upstream/health_check_corpus/crash_1 @@ -37,7 +37,7 @@ actions { value: "locations-production-iad" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/crash_3 b/test/common/upstream/health_check_corpus/crash_3 index 48232fd55b06..fd31f02d81a4 100644 --- a/test/common/upstream/health_check_corpus/crash_3 +++ b/test/common/upstream/health_check_corpus/crash_3 @@ -45,7 +45,7 @@ actions { value: "true" } } - status: "200" + status: 200 } } } @@ -62,7 +62,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/crash_4 b/test/common/upstream/health_check_corpus/crash_4 index c1b9b7522451..85c5c4516cb5 100644 --- a/test/common/upstream/health_check_corpus/crash_4 +++ b/test/common/upstream/health_check_corpus/crash_4 @@ -37,7 +37,7 @@ actions { value: "locations-production-iad" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_corpus/crash_5 b/test/common/upstream/health_check_corpus/crash_5 index 4d1bf5ff7aa5..d7c57bf11143 100644 --- a/test/common/upstream/health_check_corpus/crash_5 +++ b/test/common/upstream/health_check_corpus/crash_5 @@ -44,7 +44,7 @@ actions { value: "200" } } - status: "200" + status: 200 } } } diff --git a/test/common/upstream/health_check_fuzz.cc b/test/common/upstream/health_check_fuzz.cc index 233cc2707087..21c822396199 100644 --- a/test/common/upstream/health_check_fuzz.cc +++ b/test/common/upstream/health_check_fuzz.cc @@ -47,7 +47,7 @@ void HealthCheckFuzz::initializeAndReplay(test::common::upstream::HealthCheckTes replay(input); } -void HealthCheckFuzz::respondHttp(const test::fuzz::Headers& headers, absl::string_view status) { +void HealthCheckFuzz::respondHttp(const test::fuzz::Headers& headers, uint64_t status) { // Timeout timer needs to be explicitly enabled, usually by onIntervalBase() (Callback on interval // timer). if (!test_sessions_[0]->timeout_timer_->enabled_) { @@ -61,17 +61,6 @@ void HealthCheckFuzz::respondHttp(const test::fuzz::Headers& headers, absl::stri response_headers->setStatus(status); - // This utility gets called in codec impl which handles any exceptions thrown, so check here to - // constrain state space to what can pass through codec. - // TODO(zasweq): However, this utility function gets called in a lot of places around the codebase - // that may take in untrusted inputs. - try { - Http::Utility::getResponseStatus(*response_headers.get()); - } catch (EnvoyException e) { - ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what()); - return; - } - // Responding with http can cause client to close, if so create a new one. bool client_will_close = false; if (response_headers->Connection()) { diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index d91700fca4e0..53f6e50ef202 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -1,4 +1,6 @@ #pragma once +#include + #include "test/common/upstream/health_check_fuzz.pb.validate.h" #include "test/common/upstream/health_checker_impl_test_utils.h" #include "test/fuzz/common.pb.h" @@ -22,7 +24,7 @@ class HealthCheckFuzz Type type_; private: - void respondHttp(const test::fuzz::Headers& headers, absl::string_view status); + void respondHttp(const test::fuzz::Headers& headers, uint64_t status); void triggerIntervalTimer(bool expect_client_create); void triggerTimeoutTimer(bool last_action); void allocHealthCheckerFromProto(const envoy::config::core::v3::HealthCheck& config); diff --git a/test/common/upstream/health_check_fuzz.proto b/test/common/upstream/health_check_fuzz.proto index 786b7c65c17f..979f01d7115c 100644 --- a/test/common/upstream/health_check_fuzz.proto +++ b/test/common/upstream/health_check_fuzz.proto @@ -9,8 +9,7 @@ import "google/protobuf/empty.proto"; message HttpRespond { //TODO: Split this across headers, body, and trailers? test.fuzz.Headers headers = 1; - //https://github.com/nodejs/http-parser/blob/master/http_parser.c#L852 - string status = 2 [(validate.rules).string = {pattern: "[0-9]+$*", min_len: 1}]; + uint64 status = 2; } message Respond { From d27821b089eb0ac4becbdeab4864ba04895b7e50 Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 23 Sep 2020 19:08:05 +0000 Subject: [PATCH 3/3] Got rid of unused headers Signed-off-by: Zach --- test/common/upstream/health_check_fuzz.h | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/upstream/health_check_fuzz.h b/test/common/upstream/health_check_fuzz.h index 53f6e50ef202..07689d7f5797 100644 --- a/test/common/upstream/health_check_fuzz.h +++ b/test/common/upstream/health_check_fuzz.h @@ -1,5 +1,4 @@ #pragma once -#include #include "test/common/upstream/health_check_fuzz.pb.validate.h" #include "test/common/upstream/health_checker_impl_test_utils.h"