Skip to content
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

[http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process #10886

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

antoniovicente
Copy link
Contributor

Description: Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.
Risk Level: low
Testing: unit and integration
Docs Changes: n/a
Release Notes: n/a
Fixes #10270

…read into different buffer slices.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
asraa
asraa previously approved these changes Apr 28, 2020
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the positive/negative test coverage!

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks. Just small comment typo from a quick skim.

/wait

source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved
Signed-off-by: Antonio Vicente <avd@google.com>
@antoniovicente
Copy link
Contributor Author

/backport

@repokitteh-read-only repokitteh-read-only bot added the backport/review Request to backport to stable releases label Jul 22, 2020
@lambdai lambdai added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Jul 22, 2020
@PiotrSikora
Copy link
Contributor

@lambdai Someone has to backport those changes to older branches.

cc @antoniovicente for visibility and/or volunteering to backport.
cc @lizan to see if he can volunteer someone to help.

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 28, 2020
…ire multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 28, 2020
…ire multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: antonio <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 28, 2020
…ire multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Signed-off-by: antonio <avd@google.com>
lambdai added a commit that referenced this pull request Jul 29, 2020
…at requ… (#12320)

[http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai added a commit that referenced this pull request Jul 29, 2020
…ire multiple dispatch calls to process (#12323)

Cherry-pick [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (#10886) 
Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai added a commit that referenced this pull request Jul 31, 2020
…at requ… (#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
brian-avery pushed a commit to istio/envoy that referenced this pull request Oct 7, 2020
* doc: version 1.12.7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix wasm compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix typo

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Oct 8, 2020
* docs: kick-off 1.13.5 release. (envoyproxy#12164)

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12320)

[http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#203)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fixed compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix test

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Co-authored-by: Piotr Sikora <piotrsikora@google.com>

Co-authored-by: Yuchen Dai <silentdai@gmail.com>
Co-authored-by: Piotr Sikora <piotrsikora@google.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Oct 8, 2020
* doc: version 1.12.7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix wasm compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix typo

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Co-authored-by: Yuchen Dai <silentdai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/approved Approved backports to stable releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LWS in H1 header values may be stripped out when proxying.
5 participants