fuzz: replace invalid characters in upstream metadata for header parser#7436
fuzz: replace invalid characters in upstream metadata for header parser#7436mattklein123 merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
|
@venilnoronha can you give this a first pass? |
|
@junr03 will do. |
|
@asraa I don't seem to have access to https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15555. Can you grant me/fill me about what exactly this PR is fixing? |
Yes! Sorry about that. The added testcase causes a crash because of the null character here. Because we are trying to add an "%UPSTREAM_METADATA(...)" header, we move the values of the metadata into headers, and this crashes since headers must be "valid()" (not contain null / newline characters). Here's the stack trace: [2019-06-08 14:53:26.859][1][critical][assert] [source/common/http/header_map_impl.cc:209] assert failure: valid(). |
| envoy::api::v2::core::Metadata processed = upstream_metadata; | ||
| for (auto& metadata_struct : *processed.mutable_filter_metadata()) { | ||
| for (auto& field : *metadata_struct.second.mutable_fields()) { | ||
| if (field.second.kind_case() == ProtobufWkt::Value::kStringValue) { |
There was a problem hiding this comment.
Do you think we need to handle other recursive types (struct, list, etc.) here?
There was a problem hiding this comment.
I wasn't too concerned with this as an issue to fuzz over since metadata usually comes from the config (e.g. listenerMetadata(), or routeEntry metadata) or from the connection's streaminfo, so it'd be on the onus of the one configuring Envoy to make sure there's no NUL characters. I would have handled them only if the fuzzer gets blocked on them, but I'm not sure how complicated the fuzzer will get for this case. I could handle them, though, if you think it'd be easier to get done in one PR
There was a problem hiding this comment.
I think we can skip this for now then. @junr03 thoughts?
There was a problem hiding this comment.
hey! just a ping, would love to move forward.
thanks!
There was a problem hiding this comment.
@asraa could you please leave a note related to the discussion above? Thanks!
| fields { | ||
| key: "" | ||
| value { | ||
| string_value: "c\000\000\000\000\000\000\000" |
There was a problem hiding this comment.
Not sure if it's required. Do you think we need test cases for recursive (struct, list, etc.) types?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Asra Ali <asraa@google.com>
test/fuzz/utility.h
Outdated
| for (auto& metadata_struct : *processed.mutable_filter_metadata()) { | ||
| // Metadata fields consist of keyed Structs, which is a map of dynamically typed values. These | ||
| // values can be null, a number, a string, a boolean, a list of values, or a recursive struct. | ||
| // This clears any invalid characters in string values. It may not be likely a coverage-driven |
There was a problem hiding this comment.
s/It may/It may (two space characters between It and may)
|
/retest |
|
🔨 rebuilding |
Replace invalid header characters in filter metadata strings.
Risk Level: Low
Testing: Add corpus entry
Fixes OSS-Fuzz issue:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15555