Skip to content

[fuzz] fix bad inputs and config bugs#12504

Merged
ggreenway merged 5 commits intoenvoyproxy:masterfrom
asraa:hcm-1xx-bug
Aug 10, 2020
Merged

[fuzz] fix bad inputs and config bugs#12504
ggreenway merged 5 commits intoenvoyproxy:masterfrom
asraa:hcm-1xx-bug

Conversation

@asraa
Copy link
Contributor

@asraa asraa commented Aug 5, 2020

Signed-off-by: Asra Ali asraa@google.com

Commit Message:

  • Unset metadata matcher causes crash, since no matcher is set
  • encodeHeaders in HCM requires that the only 1xx header is a 101 upgrade
  • stream info destructor issue -- this one no longer reproduces

Risk Level: Low
Testing: Regression tests added
Fixes:
https://oss-fuzz.com/testcase?key=4863844862918656
https://oss-fuzz.com/testcase-detail/5656400764862464
https://oss-fuzz.com/testcase-detail/5631179290836992

Signed-off-by: Asra Ali <asraa@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12504 was opened by asraa.

see: more, trace.

Signed-off-by: Asra Ali <asraa@google.com>
ggreenway
ggreenway previously approved these changes Aug 6, 2020
Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @envoyproxy/api-shepherds PTAL.

lizan
lizan previously approved these changes Aug 7, 2020
@repokitteh-read-only repokitteh-read-only bot removed the api label Aug 7, 2020
// access_log_hint metadata, set the filter to "envoy.common" and the path to
// "access_log_hint", and the value to "true".
type.matcher.v3.MetadataMatcher matcher = 1;
type.matcher.v3.MetadataMatcher matcher = 1 [(validate.rules).message = {required: true}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/envoyproxy/envoy/blob/master/api/API_VERSIONING.md#backwards-compatibility

Increasing the strictness of protoc-gen-validate annotations. Exceptions may be granted for scenarios in which these stricter conditions model behavior already implied structurally or by documentation.

Assuming this change falls into this exception, because MetadataFilter can be null, @asraa ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be able to be solved in code too. MetadataFilter only needs an actual MetadataMatcher matcher so that it can have a value, otherwise this line crashes in code

value_matcher_ = Matchers::ValueMatcher::create(val);

because of
NOT_REACHED_GCOVR_EXCL_LINE;

Thinking about this more, it is possible to solve in code by only setting value_matcher_ if there's a matcher, and evaluating the match by preconditioning that there's a value_matcher_.
I think code may be better (at least for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted for the code change.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa dismissed stale reviews from lizan and ggreenway via 3d4d7c6 August 7, 2020 13:28
filter_(filter_config.matcher().filter()) {

auto& matcher_config = filter_config.matcher();
if (filter_config.has_matcher()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an explicit test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely -- done

asraa added 2 commits August 7, 2020 16:03
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@ggreenway ggreenway merged commit 67c5b43 into envoyproxy:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants