-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
router: Validate the header string in header parser. #32559
Conversation
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
…r_parser Signed-off-by: tyxia <tyxia@google.com>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/wait
// Skip any invalid header value. | ||
// The validity of the header key has already been checked as it is generated by | ||
// Http::LowerCaseString. | ||
if (!Envoy::Http::validHeaderString(value)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry in the real code where is this check done? By the codec? If this is purely for fuzzing can you potentially just guard these blocks with a FUZZ ifdef? Seems not great to add this extra runtime check where it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request has been automatically marked as stale because it has not had activity in the last 30 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! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
In production, code/parser level is centralized place for header validation safeguarding against invalid headers. Unified Header Validation is long-term solution for header validation.
However, in the fuzzer, we have seen asserts triggered by invalidate header string and it is before above header validation happens.
It probably doesn't hurt to do header validation/rejection early to avoid invalid header being further evaluated/processed unless overhead of check is a concern. Open to discussion.