Backport the http header security patch from envoy 1.9.1#51
Backport the http header security patch from envoy 1.9.1#51duderino merged 2 commits intoistio:release-1.0from
Conversation
http: fixed CVE-2019-9900 by rejecting HTTP/1.x headers with embedded NUL characters. http: fixed CVE-2019-9901 by normalizing HTTP paths prior to routing or L7 data plane processing. This defaults off and is configurable via either HTTP connection manager normalize_path or the runtime. Signed-off-by: Yuchen Dai <silentdai@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@silentdai can you point how how the path normalization is enabled? I couldn't find it in this PR. Thanks for sending this |
|
@duderino I was lost in the context switch... Yes, I forgot to mention that istio/envoy choose to enable the feature. Will update the summary. |
lambdai
left a comment
lambdai
left a comment
There was a problem hiding this comment.
I could rename the maybeNormalizePath to avoid less confusion. But I don't see much benefit here. WDYT?
| } | ||
|
|
||
| // Path sanitization should happen before any path access other than the above sanity check. | ||
| if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_)) { |
There was a problem hiding this comment.
@duderino The idea is that maybeNormalizePath is always executed in this PR.
In envoyproxy/envoy there is a branch to read from runtime as well as HCM config
There was a problem hiding this comment.
envoyproxy corresponding maybeNormalizePath
https://github.com/envoyproxy/envoy/blob/7ed6d2187df94c4cb96f7dccb8643bf764af2ccb/source/common/http/conn_manager_utility.h#L65
There was a problem hiding this comment.
@duderino Are you suggesting rename function as alwaysNormalizePath?
There was a problem hiding this comment.
nope, the function name is fine. I was just commenting that there was a mocked function that I think doesn't actually exist
|
@duderino gentle ping |
While there, revert local changes to reduce the diff with upstream. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
3 participants
Description:
http: fixed CVE-2019-9900 by rejecting HTTP/1.x headers with embedded NUL characters.
http: fixed CVE-2019-9901 by normalizing HTTP paths prior to routing or L7 data plane processing. This defaults off and is configurable via either HTTP connection manager normalize_path or the runtime.
Notes that in this PR the security feature is always enabled and no way to opt out. While in envoyproxy/envoy it is default off.
Risk Level:
MID
Testing:
Unit tests
Integration tests
Docs Changes:
Release Notes:
Bump istio/envoy version to 1.8.1. Notes that no such envoyproxy/envoy version number.
[Optional Fixes #Issue]
[Optional Deprecated:]