HCM: protect against removal of critical response headers by a filter chain. by mathetake · Pull Request #16745 · envoyproxy/envoy

mathetake · 2021-06-01T08:25:47Z

This is the rework of the previous PR (#15658).

Commit Message: HCM: protect against removal of critical response headers by a filter chain.
Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves #13756 and ref: #15487.

Notes

For the request path, Envoy is already guarded by HeaderUtility::checkRequiredHeaders and its error is gracefully handled in the onPoolReady.
The following is the current crash path with uncaught exception caused by removed :status header after filter chains:

envoy/source/common/http/conn_manager_impl.cc

Line 1481 in 5c51bf1

chargeStats(headers);

envoy/source/common/http/conn_manager_impl.cc

Line 777 in 5c51bf1

uint64_t response_code = Utility::getResponseStatus(headers);

envoy/source/common/http/utility.cc

Line 455 in 5c51bf1

    
           throw CodecClientException(":status must be specified and a valid unsigned long");

In the previous PR, I implemented this guard in the codec and make the codec return the status, but it's going to make the implementation much more complex than I expected (You can see in the size of the diff in HCM: guard Envoy from removal of critical response headers. #15658), plus introduce a lot of complexity in the code base (there were even unresolved comment about its direction). So I came back to the first solution there as opposed to the comment by @asraa HCM: guard Envoy from removal of critical response headers. #15658 (comment)).

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

asraa

i think my vote for making encodeHeaders (although larger change) return a status is that we won't need to check elsewhere in case some other extension point besides filters break status (i think that was the comment on my request side PR). but maybe that should change when that will be a problem

source/common/http/header_utility.cc

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-06-01T12:50:58Z

we won't need to check elsewhere in case some other extension point besides filters break status

Thanks, that makes sense.. but yeah we could expand this check when actually we see other extension points break the status header.

asraa

The code otherwise looks good to me, deferring to a senior maintainer for any opinions on the approach
@envoyproxy/senior-maintainers

docs/root/configuration/http/http_conn_man/response_code_details.rst

test/common/http/utility_test.cc

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

alyssawilk

thanks for fixing this.

LGTM modulo comment request.

Non-blocking comments, fixing this crash is good, but if we want to allow semi-trusted wasm, I think we should do the full suite of header checks that we do in the codec after wasm runs, ensure all headers only have valid characters, we don't allow smuggling by having transfer encoding and content length, relative urls don't start with ../ etc? Is there a plan and/or tracking issue for that?

source/common/http/header_utility.cc

alyssawilk · 2021-06-01T18:35:35Z

source/common/http/header_utility.h

   * missing.
   */
-  static Http::Status checkRequiredHeaders(const Http::RequestHeaderMap& headers);
+  static Http::Status checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers);


I hear your point about calling this from the codec being really messy.
optional!: Could we at least as part of this PR make the codecs not throw/crash if they get bad data a well? I think we'd prefer to serialize 0 if status code is not present to crashing though Matt may disagree =P

In order to do so, we should eliminate all the usage of getResponseStatus and use getResponseStatusNoThrow instead? e.g.

envoy/source/common/http/http2/codec_impl.cc

Line 336 in 5c51bf1

const uint64_t status = Http::Utility::getResponseStatus(*headers);

envoy/source/common/http/http1/codec_impl.cc

Lines 373 to 375 in 5c51bf1

// The contract is that client codecs must ensure that :status is present.

ASSERT(headers.Status() != nullptr);

uint64_t numeric_status = Utility::getResponseStatus(headers);

I think fixing this is worth a separate PR and discussion, so can I open an issue for now and discuss there?

SGTM, and as it's optional don't feel obliged to tackle it (though it would be nice!)
I think it's largely a part of removing exceptions from the data plane, which I think @chaoqin-li1123 is looking at

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-06-01T23:25:50Z

but if we want to allow semi-trusted wasm, I think we should do the full suite of header checks that we do in the codec after wasm runs, ensure all headers only have valid characters, we don't allow smuggling by having transfer encoding and content length, relative urls don't start with ../ etc? Is there a plan and/or tracking issue for that?

yeah that's true and this is somewhat related to the comment by @PiotrSikora #15487 (comment), and yes, we should do Wasm specific check in addition to this Envoy-wide protection. I will open an issue for tracking this. Thanks!

… chain. (envoyproxy#16745) This is the rework of the previous PR (envoyproxy#15658). Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves envoyproxy#13756 and ref: envoyproxy#15487. Testing: new integration tests Risk: low Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

HCM: guard Envoy from removal of critical response headers.

9da66e0

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake changed the title ~~HCM: protect against removal of critical response headers.~~ HCM: protect against removal of critical response headers by a filter chain. Jun 1, 2021

This was referenced Jun 1, 2021

Gracefully handle removal of critical headers by filters #13756

Closed

wasm: crashes when removing ":status" header in the response. #15487

Closed

Fix docs.

6ad8f4b

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake marked this pull request as ready for review June 1, 2021 09:11

mathetake added 2 commits June 1, 2021 18:25

Fix router test.

fd491bf

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Fix format.

b09c2d7

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

asraa reviewed Jun 1, 2021

View reviewed changes

source/common/http/header_utility.cc Outdated Show resolved Hide resolved

review: use getResponseStatusNoThrow in checkRequiredResponseHeaders.

937bc2c

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake requested a review from asraa June 1, 2021 12:48

asraa previously approved these changes Jun 1, 2021

View reviewed changes

docs/root/configuration/http/http_conn_man/response_code_details.rst Outdated Show resolved Hide resolved

test/common/http/utility_test.cc Outdated Show resolved Hide resolved

review: simplify tests, fix doc.

7953e30

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake dismissed asraa’s stale review via 7953e30 June 1, 2021 13:06

Fix test comment.

1430cd8

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

junr03 assigned asraa and alyssawilk Jun 1, 2021

alyssawilk reviewed Jun 1, 2021

View reviewed changes

review: add comment about why this check is here.

46d285a

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake requested a review from alyssawilk June 2, 2021 00:43

alyssawilk approved these changes Jun 2, 2021

View reviewed changes

alyssawilk merged commit 13f1860 into envoyproxy:main Jun 2, 2021

mathetake deleted the missing-response-headers branch June 2, 2021 23:08

KBaichoo mentioned this pull request Apr 2, 2025

lua filter: ensure active_request_ is verified before deletion Fixes #38017 #38863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCM: protect against removal of critical response headers by a filter chain.#16745

HCM: protect against removal of critical response headers by a filter chain.#16745
alyssawilk merged 8 commits intoenvoyproxy:mainfrom
mathetake:missing-response-headers

mathetake commented Jun 1, 2021 •

edited

Loading

Uh oh!

asraa left a comment

Uh oh!

Uh oh!

mathetake commented Jun 1, 2021

Uh oh!

asraa left a comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Uh oh!

Uh oh!

alyssawilk Jun 1, 2021

Uh oh!

mathetake Jun 1, 2021

Uh oh!

alyssawilk Jun 2, 2021

Uh oh!

mathetake commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// The contract is that client codecs must ensure that :status is present.
	ASSERT(headers.Status() != nullptr);
	uint64_t numeric_status = Utility::getResponseStatus(headers);

Conversation

mathetake commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathetake commented Jun 1, 2021

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

mathetake Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

mathetake commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mathetake commented Jun 1, 2021 •

edited

Loading