[fuzz] fix invalid header related issues in fuzz tests

[asraa]

Commit Message: Cleanup fuzzed headers that are fed into methods whose incoming headers have already had checks performed by codecs or other callsites.
Additional Description:

• conn_manager_impl_fuzz_test trips on a sanity check that the codecs validated the host header. Since we feed random fuzzed inputs to this method, we have to clean the input to be valid. The previous validation only removed whitespace, now it uses the same nghttp2_check_authority method.
• codec_impl_fuzz_test trips on the use a host header in encodeHeaders from an http/1 input that has a missing host. At the point where the function is called, the request headers should already have a host header.

Testing: Added corpus entry for codec_impl that includes a request header without a host, and for conn_manager_impl that includes an invalid authority header.
Risk level: Low
Fixes:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21627
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21816

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

[adisuissa]

Looks good. A small comment about the comment :)

[adisuissa]

This interface is for both HTTP/1.x and HTTP/2.
Do you think it makes sense to add the specific comment only to the HTTP/1.x method (maybe with an ASSERT(host != nullptr) as this is an invariant)?

[alyssawilk]

I think there is an assertion on status still?
Also can you remind me where the host assertion comes from? We could weigh removing it - HTTP/0.9 doesn't require a host for example and we have to serialize raw HTTP to integration test it.

[asraa]

Hmm yeah, I'll need to remove that comment (see @alyssawilk review). I think this is only relevant to the CONNECT without host header issue, and I'll need to know if CONNECT without host is allowed (if it is I'll remove the comment and I think the resolution here is to fix this line

envoy/source/common/http/http1/codec_impl.cc

Line 380 in bad35c8

connection_.copyToBuffer(host->value().getStringView().data(), host->value().size());

[alyssawilk]

Thanks for cleaning up the fuzzers!

[alyssawilk]

I think there is an assertion on status still?
Also can you remind me where the host assertion comes from? We could weigh removing it - HTTP/0.9 doesn't require a host for example and we have to serialize raw HTTP to integration test it.

[asraa]

Also can you remind me where the host assertion comes from? We could weigh removing it - HTTP/0.9 doesn't require a host for example and we have to serialize raw HTTP to integration test it.

It trips on a missing host header for connect here:

envoy/source/common/http/http1/codec_impl.cc

Line 380 in bad35c8

connection_.copyToBuffer(host->value().getStringView().data(), host->value().size());

. And I don't think we have a host assertion before that. Are CONNECT's without a host header allowed?

Edit: The removal of the :valid status is because this is for Request headers -- I think that was a copy/paste snafu!

[alyssawilk]

Connect isn't allowed without host - on ingress we would fail parsing. I would like to gracefully handle the case where some filter screws up the host header - I'd added in throwing an exception like we do for missing method, but per Matt's point that doesn't work and you're removing it anyway.
I guess my thought is I'd like them all handled the same way, with graceful connection teardown "you did something wrong" rather than insist they be present even though intermediate filters could screw that up.
I'm cool with fuzzer fixes in the interim if it reduces noise, but could proper error handling for this and other expectations be part of the exception removal path?

[asraa]

Gotcha, thanks for the clarification!

even though intermediate filters could screw that up.
I'm cool with fuzzer fixes in the interim if it reduces noise, but could proper error handling for this and other expectations be part of the exception removal path?

I added proper error handling to inputs that may come from filters to the design overview of #10878. It seemed like I will tackle it in two PRs: callback-only exceptions, and other exceptions like this that need to be replaced with proper error handling if they're hit. In the interim, I would at least like to add a comment/TODO reference for tracking in this PR. I feel like "TODO(10878): Add error handling for missing Host header" without any current handling at all is strange, wdyt?

[alyssawilk]

Again I'm fine with interim fuzzer fixes, I was only really objecting to the API comments - if what we want long term is error handling rather than required field X (and we think it can't happen today apart from badly behaved filters) I'd be inclined to leave the API as-is and maybe TODO unwinding the fuzzer to fuzz once we have error handling for unexpected codec errors?

[adisuissa]

It seems that test/fuzz/utility.h is missing an #include "nghttp2/nghttp2.h".
Would also suggest to verify that Host header exists if connect is being used (so it will also be verified in after exceptions are removed) to

envoy/source/common/http/http1/codec_impl.cc

Line 364 in 941477a

if (!method || (!path && !is_connect)) {

Other than that LGTM.

[asraa]

API comments fixed, added TODOs to add proper error handling for the missing connect/host header, since adding a throw would just be crashing