header_map/fuzz: improve robustness to embedded NULL in headers.#6170
Merged
htuch merged 2 commits intoenvoyproxy:masterfrom Mar 5, 2019
Merged
header_map/fuzz: improve robustness to embedded NULL in headers.#6170htuch merged 2 commits intoenvoyproxy:masterfrom
htuch merged 2 commits intoenvoyproxy:masterfrom
Conversation
As discovered back in envoyproxy#5867, we have some situations where we expect the codecs to reject embedded NULLs in header key/values. This PR improves codec_impl_fuzz_test by having it ignore such invalid values and also adds a bunch of ASSERTs to HeaderMapImpl to document/guard against any potential NULL creep, since its correctness is predicated on this. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13447. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
requested changes
Mar 5, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks looks good with one request for some comments.
/wait
|
|
||
| private: | ||
| void lower() { std::transform(string_.begin(), string_.end(), string_.begin(), tolower); } | ||
| bool valid() const { return string_.find('\0') == std::string::npos; } |
Member
There was a problem hiding this comment.
Can you add some kind of comment of why this indicates valid, why we are doing this, it's only used for asserts, etc.? (Same for the other valid function, or just have a comment there to point to here or vice versa)
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
…oyproxy#6170) As discovered back in envoyproxy#5867, we have some situations where we expect the codecs to reject embedded NULLs in header key/values. This PR improves codec_impl_fuzz_test by having it ignore such invalid values and also adds a bunch of ASSERTs to HeaderMapImpl to document/guard against any potential NULL creep, since its correctness is predicated on this. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13447. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As discovered back in #5867, we have some situations where
we expect the codecs to reject embedded NULLs in header key/values. This PR improves
codec_impl_fuzz_test by having it ignore such invalid values and also adds a bunch of ASSERTs to
HeaderMapImpl to document/guard against any potential NULL creep, since its correctness is
predicated on this.
Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13447.
Risk level: Low
Testing: Corpus entry added.
Signed-off-by: Harvey Tuch htuch@google.com