New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fuzz: fixes oss-fuzz: 9895 #4189

Merged
merged 4 commits into from Aug 20, 2018

Conversation

Projects
None yet
3 participants
@anirudhmurali
Copy link
Member

anirudhmurali commented Aug 16, 2018

Title: Fixes oss-fuzz: 9895

Description: oss-fuzz issue (9895): https://oss-fuzz.com/v2/testcase-detail/6195059702628352
The given input testcase had multiple START_TIME formatters as value, a valid input must contain only one START_TIME, throwing exception if invalid.

Risk Level: Low

Testing: Tested unit tests (bazel test //test/common/router:header_parser_fuzz_test), built and ran fuzzers with oss-fuzz.

Signed-off-by: Anirudh M m.anirudh18@gmail.com

fuzz: fixes oss-fuzz: 9895
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch
Copy link
Member

htuch left a comment

Thanks for taking this one on.

@@ -143,7 +143,10 @@ RequestInfoHeaderFormatter::RequestInfoHeaderFormatter(absl::string_view field_n
}
field_extractor_ = [this, pattern](const Envoy::RequestInfo::RequestInfo& request_info) {
const auto& formatters = start_time_formatters_.at(pattern);
ASSERT(formatters.size() == 1);
if (formatters.size() != 1) {

This comment has been minimized.

@htuch

htuch Aug 16, 2018

Member

@dio does this seem right?

This comment has been minimized.

@dio

dio Aug 17, 2018

Member

I think yes. From the current use-case perspective, this is correct.

This comment has been minimized.

@dio

dio Aug 17, 2018

Member

On the second thought, I think we can support multiple patterns.

@anirudhmurali Do you want to take it?

@@ -0,0 +1,8 @@
headers_to_add {

This comment has been minimized.

@htuch

htuch Aug 16, 2018

Member

@anirudhmurali can you add a test case to the unit tests for header_formatter.cc? Best practice is to check in both corpus and a unit test for the new error handling.

@htuch htuch assigned dio and htuch Aug 16, 2018

@htuch htuch requested a review from dio Aug 16, 2018

ASSERT(formatters.size() == 1);
if (formatters.size() != 1) {
throw EnvoyException(
fmt::format("multiple start_time values found: {}", formatters.size()));

This comment has been minimized.

@dio

dio Aug 17, 2018

Member

@anirudhmurali, thanks for checking on this.

Questions:

1. Do you think "multiple %START_TIME% patterns..." sounds better?
2. Is the input here: https://github.com/envoyproxy/envoy/pull/4189/files#diff-084669b1d67ce3df5fbda83bef9354bbR3 causing this?

This comment has been minimized.

@dio

dio Aug 17, 2018

Member

NVM, I tried to patch the header_formatter to accept this pattern from the corpus, and it seems it makes sense to have multiple patterns inline as the header value.

@anirudhmurali, as I said above, I can work with you to fix this.

(This fuzzer is really good then. This is my mistake, sorry).

added support for multiple START_TIME patterns
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Aug 19, 2018

@dio With regards to the Slack discussion and the GitHub comments, I believe this allows definition of multiple START_TIME patterns. Have added the case to the test file as well. Please let me know if there is anything more to this. Thanks.

fixed failing test
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@dio
Copy link
Member

dio left a comment

Thanks, @anirudhmurali! Looks good. One small request.

@@ -617,6 +617,10 @@ match: { prefix: "/new_endpoint" }
key: "x-request-start"
value: "%START_TIME(%s.%3f)%"
append: true
- header:
key: "x-request-start"

This comment has been minimized.

@dio

dio Aug 19, 2018

Member

Let's rename the key to something else, e.g. "x-request-start-multiple", and add some similar expectations below.

EXPECT_TRUE(headerMap.has("x-request-start-multiple"));
EXPECT_EQ("1522796769.123 2018-04-03T23:06:09.123Z 1522796769", headerMap.get_("x-request-start-multiple"));
added expectations
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch

htuch approved these changes Aug 20, 2018

Copy link
Member

htuch left a comment

LGTM; @dio do you want to take a final pass and merge if it look good to you?

@dio

dio approved these changes Aug 20, 2018

Copy link
Member

dio left a comment

LGTM. Thanks, @anirudhmurali!

@htuch sure! Will do that soon after the CI is done the checking. Thank you!

@htuch htuch merged commit db4783b into envoyproxy:master Aug 20, 2018

12 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details

snowp added a commit to snowp/envoy that referenced this pull request Aug 20, 2018

Merge remote-tracking branch 'origin/master' into retry-filter
* origin/master: (34 commits)
  fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189)
  docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194)
  fix sometimes returns response body to HEAD requests (envoyproxy#3985)
  admin: fix config dump order (envoyproxy#4197)
  thrift: allow translation between downstream and upstream protocols (envoyproxy#4136)
  Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120)
  upstream: allow extension protocol options to be used with http filters (envoyproxy#4165)
  [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169)
  docs: improve gRPC-JSON filter doc (envoyproxy#4184)
  stats: refactoring MetricImpl without strings (envoyproxy#4190)
  fuzz: coverage profile generation instructions. (envoyproxy#4193)
  upstream: rebuild cluster when health check config is changed (envoyproxy#4075)
  build: use clang-6.0. (envoyproxy#4168)
  thrift_proxy: introduce header transport (envoyproxy#4082)
  tcp: allow connection pool callers to store protocol state (envoyproxy#4131)
  configs: match latest API changes (envoyproxy#4185)
  Fix wrong mock function name. (envoyproxy#4187)
  Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182)
  Converting envoy configs to V2 (envoyproxy#2957)
  Add timestamp to HealthCheckEvent definition (envoyproxy#4119)
  ...

Signed-off-by: Snow Pettersen <snowp@squareup.com>

nickrmc83 added a commit to thales-e-security/envoy that referenced this pull request Aug 23, 2018

fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189)
Fixes oss-fuzz issue (9895): https://oss-fuzz.com/v2/testcase-detail/6195059702628352
The given input testcase had multiple START_TIME formatters as value, these are now supported.

Risk Level: Low
Testing: Tested unit tests (bazel test //test/common/router:header_parser_fuzz_test), built and ran fuzzers with oss-fuzz.

Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment