Skip to content

test/fuzz: frame replay style testing and fuzzing for HTTP/2 headers.#6491

Merged
htuch merged 5 commits intoenvoyproxy:masterfrom
htuch:h2-stateless-fuzz
Apr 14, 2019
Merged

test/fuzz: frame replay style testing and fuzzing for HTTP/2 headers.#6491
htuch merged 5 commits intoenvoyproxy:masterfrom
htuch:h2-stateless-fuzz

Conversation

@htuch
Copy link
Member

@htuch htuch commented Apr 5, 2019

In the fix patch for CVE-2019-9900, we introduced some basic HTTP/2
manual fuzzing, where single bytes were corrupted in a HEADERS frame, to
attempt to show that NUL/CR/LF were handled. However, testing that
relies on codec_impl_test, which has nghttp2 as both client and server. This
implies that Huffman coding may be present, and single byte corruptions
of 0x00 don't imply a NUL for example.

In this patch, we take a more principled approach and use artisinal
HEADERS frames that have no Huffman or dynamic table compression to
validate the above single byte corruption property.

A nice side effect of this is that we can derived from this
infrastructure stateless request/response HEADERS fuzzers that can cover
uncompressed (specifically no Huffman) paths, which is more likely to
provide a direct access to nghttp2 codec header sanitization logic.

Risk level: Low
Testing: Unit tests and ran both fuzzers under oss-fuzz Docker image.
Seems reasonably fast and no crashes locally.

Signed-off-by: Harvey Tuch htuch@google.com

In the fix patch for CVE-2019-9900, we introduced some basic HTTP/2
manual fuzzing, where single bytes were corrupted in a HEADERS frame, to
attempt to show that NUL/CR/LF were handled. However, testing that
relies on codec_impl_test has nghttp2 as both client and server. This
implies that Huffman coding may be present, and single byte corruptions
of 0x00 don't imply a NUL for example.

In this patch, we take a more principled approach and use artisinal
HEADERS frames that have no Huffman or dynamic table compression to
validate the above single byte corruption property.

A nice side effect of this is that we can derived from this
infrastructure stateless request/response HEADERS fuzzers that can cover
uncompressed (specifically no Huffman) paths, which is more likely to
provide a direct access to nghttp2 codec header sanitization logic.

Risk level: Low
Testing: Unit tests and ran both fuzzers under oss-fuzz Docker image.
  Seems reasonably fast and no crashes locally.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Apr 9, 2019

Friendly ping.

TEST_F(RequestFrameCommentTest, SingleByteNulCrLfInHeaderField) {
FileFrame header{"request_header_corpus/simple_example_plain"};

for (size_t offset = header.frame().size() - 11 /* foo: offset */; offset < header.frame().size();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not try to corrupt the whole frame?

Copy link
Member Author

Choose a reason for hiding this comment

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

The above test SingleByteNulCrLfInHeaderFrame does this. The idea in this test is that we explicitly expect an exception. If you corrupt some bytes in a frame with NUL, it will have no effect and more or less parse correctly, e.g. those bytes that are already NUL :)

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM from a skim and very neat. Can you add some comments somewhere on how you generated all the magic byte strings that are in this PR? Did you use tcpdump or some other mechanism? Thank you.

/wait

@htuch
Copy link
Member Author

htuch commented Apr 11, 2019

@mattklein123 I basically added hexdump to codec_impl_test.cc in the write ON_CALLs. I'll leave a comment; I can also preserve the trace level ENVOY_LOG_MISC there if you think it's worthwhile?

@mattklein123
Copy link
Member

I can also preserve the trace level ENVOY_LOG_MISC there if you think it's worthwhile?

Probably not, but I think a comment would be very helpful.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch merged commit 1e61a3f into envoyproxy:master Apr 14, 2019
@htuch htuch deleted the h2-stateless-fuzz branch April 14, 2019 00:02
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