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: 8363 #3905

Merged
merged 4 commits into from Jul 25, 2018

Conversation

Projects
None yet
4 participants
@anirudhmurali
Copy link
Member

anirudhmurali commented Jul 19, 2018

Title: Fixes oss-fuzz: 8363

Description: oss-fuzz issue (8363): https://oss-fuzz.com/v2/testcase-detail/5988544525893632
The crash was because of passing nan to Envoy::ProtobufPercentHelper::convertPercent, it asserts since it is not in the numeric range. Instead of adding a check in this function, have added a check in the preprocessor so that it goes to checkAndReturnDefault and the default value is used.
Have also added the crashing testcase to the corpus.

Risk Level: Low

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

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

fuzz: fixes oss-fuzz: 8363
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@@ -65,7 +65,7 @@ uint64_t fractionalPercentDenominatorToInt(const envoy::type::FractionalPercent&
// @param default_value supplies the default if the field is not present.
#define PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT(message, field_name, max_value, \
default_value) \
((message).has_##field_name() \
((message).has_##field_name() && !std::isnan((message).field_name().value()) \
? ProtobufPercentHelper::convertPercent((message).field_name().value(), max_value) \

This comment has been minimized.

@htuch

htuch Jul 19, 2018

Member

I think this is fine to mitigate the issue today. The clean way to do this is to catch NaN in PGV annotations (i.e. https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/percent.proto#L14, https://github.com/lyft/protoc-gen-validate/blob/master/validate/validate.proto#L98), the same way we validate the [0..100] range.

Can you file an issue for NaN validation at https://github.com/lyft/protoc-gen-validate and add a TODO comment here?

@rodaine @akonradi does this make sense to you?

This comment has been minimized.

@akonradi

akonradi Jul 19, 2018

Contributor

Agreed, this would be a good validation to have in PGV - it's simple, self-contained, and generally useful.

This comment has been minimized.

@anirudhmurali

anirudhmurali Jul 23, 2018

Member

Done both 👍

This comment has been minimized.

@htuch

htuch Jul 24, 2018

Member

Thanks for filing the issue. I think you need a small change of behavior here. Instead of returning the default in NaN, it should throw EnvoyException(), to allow the NaN to be reflected back to the config pipeline.

@htuch htuch self-assigned this Jul 19, 2018

anirudhmurali added some commits Jul 23, 2018

added todo
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
addressed comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
((message).has_##field_name() && !std::isnan((message).field_name().value()) \
? ProtobufPercentHelper::convertPercent((message).field_name().value(), max_value) \
((message).has_##field_name() \
? !std::isnan((message).field_name().value()) \

This comment has been minimized.

@htuch

htuch Jul 24, 2018

Member

I would flip the logic here to have the isnan case first.

((message).has_##field_name() \
? !std::isnan((message).field_name().value()) \
? ProtobufPercentHelper::convertPercent((message).field_name().value(), max_value) \
: throw EnvoyException(fmt::format("Value not in the range of 0..100 range.")) \

This comment has been minimized.

@htuch

htuch Jul 24, 2018

Member

Can you add a test? Thanks.

This comment has been minimized.

@anirudhmurali
added tests and addressed comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch

htuch approved these changes Jul 25, 2018

Copy link
Member

htuch left a comment

Thanks!

@zuercher zuercher merged commit 8459237 into envoyproxy:master Jul 25, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment