router: fix null-terminator issue in path replacement.

[htuch]

Fixes oss-fuzz issue
https://oss-fuzz.com/testcase-detail/5137346677178368.

Risk level: Low
Testing: Corpus entry added.

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

[htuch]

@mattklein123 not sure if this situation can happen in reality. I.e. we have a Path that is "\0001\000". Will the HTTP parsers reject or should we scrub?

[mattklein123]

@mattklein123 not sure if this situation can happen in reality. I.e. we have a Path that is "\0001\000". Will the HTTP parsers reject or should we scrub?

I looked at the spec and the parser implementations and no, this is not a valid header value that would pass parsing. I have 2 questions:

1. How does this actually crash? It's not clear to me from the diff.
2. Is there any way to make the fuzzer only generate values that the parser will accept (VCHAR in the HTTP/1.1 BNF)

[htuch]

1. It crashes in

   envoy/source/common/router/config_impl.cc

   Line 500 in 88fe0c9

   ASSERT(case_sensitive_ ? absl::StartsWith(path, matched_path)

   .
2. Probably. I'd argue that the fix in this PR is actually harmless and also resolves the issue, so we can put this restriction in next time we get something that shouldn't happen by the spec. It can only be a good thing not to rely on scanning a C string for nullptr :)

[mattklein123]

For 1 I'm probably being dense, but isn't the string null terminated?

[htuch]

The matched_path, from caller, uses data+size to form the string, whereas path is using the C nullptr search for length. Since we have an embedded null, the ASSERT fires.

[mattklein123]

Ah I see. OK thanks. Sure, we can make the fuzzer smarter in a follow up if we need to.