Skip to content

fuzz: added fuzz test for listener filter http_inspector#12411

Merged
asraa merged 35 commits intoenvoyproxy:masterfrom
arthuryan-k:fuzz_generic_listener
Aug 10, 2020
Merged

fuzz: added fuzz test for listener filter http_inspector#12411
asraa merged 35 commits intoenvoyproxy:masterfrom
arthuryan-k:fuzz_generic_listener

Conversation

@arthuryan-k
Copy link
Contributor

@arthuryan-k arthuryan-k commented Jul 31, 2020

Signed-off-by: Arthur Yan arthuryan@google.com

Commit Message: Added fuzz test for listener filter http_inspector
Additional Description:

Extended generic listener filter fuzzer library to support mocked dispatcher and system call behavior
Created http_inspector_corpus and populated with testcases (valid and invalid headers)
Created http_inspector_fuzz_test.cc and updated ListenerFilterFuzzer API

Risk Level: Low
Testing: ran fuzzer, increased function coverage of http_inspector.cc to 100.0% and line coverage to 84.9% (covers all parse states except errors related to socket read failure). coverage for other listener filters (original_src and original_dst) unchanged.
Docs Changes: N/A
Release Notes: N/A

/cc @akonradi
/cc @asraa

Signed-off-by: Arthur Yan <arthuryan@google.com>
Updated original_dst_fuzz_test and original_dst corpus entries to reflect changes

Signed-off-by: Arthur Yan <arthuryan@google.com>
…lect new API

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Builds and passes tests for http_inspector unit test

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
…fuzzer

Signed-off-by: Arthur Yan <arthuryan@google.com>
Changed the API of ListenerFilterFuzzer

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
@asraa
Copy link
Contributor

asraa commented Jul 31, 2020

@jianwen612 Would you like to do a review pass on this?

@jianwen612
Copy link
Contributor

@jianwen612 Would you like to do a review pass on this?

Yes!

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Fixed crashing original_dst_fuzz_test (segmentation fault)

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! Very minor nits, but this looks great.


while (!got_continue) {
if (header.done()) { // End of stream reached but not done
file_event_callback_(Event::FileReadyType::Closed);
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no more data to fuzz, should this be an early return? (Does "not done" mean that the input data has not been read completely, even if the filter will do a no-op?)

Copy link
Contributor Author

@arthuryan-k arthuryan-k Aug 10, 2020

Choose a reason for hiding this comment

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

No, "not done" should handle the case where all of the fuzzed input data has been read (end of stream reached), but the parser fails to determine http. In this case, the file event will fallback to non-http and call continueFilterChain(true), but it needs to receive Event::FileReadyType::Closed in order to trigger that behavior. The file event itself should be able to handle all other possible cases (socket read error, early return after success, waiting for the next event, etc.).

Edit: Actually, great catch! This works as-is for http_inspector due to how done is implemented (continueFilterChain handled separately), but there should be an explicit early return in general (in tls_inspector, continueFilterChain is handled inside done, which causes this while loop to run indefinitely). I'll fix this in the PR for fuzzing tls_inspector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhhhh! Gotcha -- thanks so much for the explanation though. Sounds good for fixing later :)

Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
Signed-off-by: Arthur Yan <arthuryan@google.com>
@asraa asraa merged commit cf85068 into envoyproxy:master Aug 10, 2020
@arthuryan-k arthuryan-k deleted the fuzz_generic_listener branch August 10, 2020 18:05
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.

4 participants