Sanitize headers nominated by the Connection header by abaptiste · Pull Request #8862 · envoyproxy/envoy

abaptiste · 2019-11-02T04:01:32Z

Description: Sanitize headers nominated by the Connection header

Risk Level: Medium
Testing: Added several test cases to verify the header manipulation. Also executed bazel test //test/...
Docs Changes: N/A
Release Notes: N/A
Fixes: #8623

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov · 2019-11-04T16:07:57Z

source/common/http/http1/codec_impl.h

  const uint32_t max_headers_count_;

  bool strict_header_validation_;
+  bool connection_header_sanitization_;


Missing const

Would that mean that strict_header_validation_ is also missing a const?

yes, thanks for fixing this as well.

yanavlasov · 2019-11-04T16:10:56Z

source/common/http/utility.cc

+      absl::StrSplit(connection_header_value.getStringView(), ',');
+
+  // If we have 10 or more nominated headers, fail this request
+  if (connection_header_tokens.size() >= 10) {


Nit: I would create a named constant for 10

yanavlasov · 2019-11-04T16:15:59Z

source/common/http/utility.cc

+    }
+
+    // Build the LowerCaseString for header lookup
+    const std::string header_to_remove = std::string(token_sv);


Note that the 'header_to_remove' is not used anywhere other than to initialize lcs_header_to_remove. To eliminate memory allocation here, you can just directly use std::string(token_sv) in the lcs_header_to_remove initialization.

Yep. With the multiple refactors I missed this.

yanavlasov · 2019-11-04T16:21:42Z

source/common/http/utility.cc

+    // Determine whether the nominated header contains invalid values
+    HeaderEntry* nominated_header = NULL;
+
+    if (StringUtil::CaseInsensitiveCompare()(token_sv, Http::Headers::get().Connection.get())) {


The header names are lowercase already. You can simplify this by making it lcs_header_to_remove == Http::Headers::get().Connection

yanavlasov · 2019-11-04T16:22:07Z

source/common/http/utility.cc

+      keep_header = true;
+      headers_to_remove.emplace(token_sv);
+
+    } else if (StringUtil::CaseInsensitiveCompare()(token_sv,


Same as above and other instances of header name comparison below.

yanavlasov · 2019-11-04T16:25:03Z

source/common/http/utility.cc

+    }
+
+    if (nominated_header) {
+      const HeaderString& nominated_header_value = nominated_header->value();


You can skip this step and just use nominated_header->value().getStringView() below.

yanavlasov · 2019-11-04T16:25:49Z

source/common/http/utility.cc

+
+    if (nominated_header) {
+      const HeaderString& nominated_header_value = nominated_header->value();
+      auto nominated_heaader_value_sv = nominated_header_value.getStringView();


Typo in variable name

I thought I fixed this. I will double check.

yanavlasov · 2019-11-04T16:29:32Z

source/common/http/utility.cc

+
+      // reject the request if the TE header is too large
+      if (is_te_header && (nominated_heaader_value_sv.size() >= 256)) {
+        return false;


This may need a log message. Because form the log message printed by the caller it would be unclear why sanitizeConnectionHeader failed.

yanavlasov · 2019-11-04T17:34:34Z

source/common/http/utility.cc

+      // If the TE header contains "trailers" we will reset the header to this value since it's
+      // the only permitted value
+      if (is_te_header && (nominated_heaader_value_sv.find(
+                               Http::Headers::get().TEValues.Trailers) != std::string::npos)) {


I'll defer to other reviewers to see this check is enough. It does not account for a weird edge case where a header name contains the "trailers" keyword. I thought about it more and I think your original version using StrSplit in the for range loop was correct. The StrSplit in the for range loop does not cause memory allocation and with the length check it would be safe. If the find check is deemed to be insufficient you should use check for the trailers keyword using your original method. Sorry abot flip-flopping on this one.

This check should only execute if the header itself is "TE". I removed the tokenization since it was expensive (per a comment on the old PR). In this change, if the header is "TE" and we find "trailers" and other values (there should be a check for whether a comma exists in the header value), then we would replace the existing value of TE: whatever, and, trailers with just TE: trailers.

There was a second commit where I fixed the variable spelling error and added this check. In any case, I'm open to what others would suggest here.

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov

Thanks, the change looks good. Just one last little thing you missed.

yanavlasov · 2019-11-05T15:50:47Z

source/common/http/utility.cc

+
+    // If the Connection token value is not a nominated header, ignore it here since
+    // the connection header is removed elsewhere when the H1 request is upgraded to H2
+    if (StringUtil::CaseInsensitiveCompare()(token_sv, cv.Close) ||


One more instance of header comparison.

OK, let me work on this

- Remove CaseInsensitiveCompare in favor of == operator Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov

Look awesome, thanks for bearing with me. Just a couple of minor changes, please.

yanavlasov · 2019-11-07T01:01:34Z

source/common/http/utility.cc

+
+    // If the Connection token value is not a nominated header, ignore it here since
+    // the connection header is removed elsewhere when the H1 request is upgraded to H2
+    if ((lcs_header_to_remove == LowerCaseString(cv.Close)) ||


Ah, yes ConnectionValues are std::string not LowerCaseString. Conversion from std::string to LowerCaseString may cause heap allocations. For the maximum of 10 tokens this part will do 40 allocations, copies and lower case conversions. We know that ConnectionValues are lowercase. I think a better approach would be to use (lcs_header_to_remove.get() == cv.Close) pattern, in this way a per iteration work is avoided.

yanavlasov · 2019-11-07T01:02:25Z

source/common/http/utility.cc

+                           std::string::npos)) {
+
+        // Reset the TE header to just trailers only if other values exist
+        if (nominated_header_value_sv.find(',') != std::string::npos) {


Excellent way to avoid needless clear and copy.

yanavlasov · 2019-11-07T01:11:25Z

source/common/http/utility.cc

+
+      // If the TE header contains "trailers" we will reset the header to this value since it's
+      // the only permitted value
+      if (is_te_header && (nominated_header_value_sv.find(Http::Headers::get().TEValues.Trailers) !=


The more I think about it, the more I agree that your original approach of looking for the "trailers" keyword was the correct one. This will fail if request has the "TE: TRAILERS" header which is allowed by the standard (values are case insensitive) and TE would be removed. It will also keep the header if request has the "TE: myabusivetrailersencoding", which I'm certain someone would figure out how to abuse. I think this should go back to the range for loop over the split nominated_header_value_sv and do the trim and lower case conversion before comparing to the "trailers" keyword. Sorry about taking your time with going back and forth on this one.

Your example is valid. I updated the code and added a test case that exercises this condition.

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov · 2019-11-08T01:10:16Z

source/common/http/utility.cc

+          const absl::string_view header_sv = StringUtil::trim(header_value);
+
+          // Remove everything from TE header except "trailers"
+          if (!StringUtil::CaseInsensitiveCompare()(header_sv,


It is very close. It can be cleaned up a bit more by removing the need for the tokens_to_remove map. Note that the condition for keeping the TE header is the presence of the "trailers" token. You can do the following:

leave the keep_header unchanged at the beginning of the loop

reverse the if statement condition inside the loop, so it it is true when the header_sv == "trailers"

inside the if statement set the keep_header and "break" out of the loop, since we do not care to look at the rest of the tokens
After the loop, check if TE header has to be kept and if so set its value to "trailers". With this change you can remove the if statement below at line 484 entirely.
This avoids the overhead of the building the hash map and calling the "removeTokens" method.

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov

Thank you for making this change.

alyssawilk

Looking great! here's some comments from my side

alyssawilk · 2019-11-12T18:27:57Z

source/common/http/http1/codec_impl.cc

      ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_);
      handling_upgrade_ = true;
    }
+  } else if (connection_header_sanitization_ && current_header_map_->Connection()) {


can you add release notes at docs/root/intro/version_history.rst including the flag to flip to revert to prior behavior?

Sure thing!

alyssawilk · 2019-11-12T18:31:01Z

source/common/http/utility.cc

+
+  StringUtil::CaseUnorderedSet headers_to_remove{};
+  std::vector<std::string> connection_header_tokens =
+      absl::StrSplit(connection_header_value.getStringView(), ',');


can we use StringUtil::splitToken, which auto-trims, and also allows removing empty headers?

alyssawilk · 2019-11-12T18:33:15Z

source/common/http/utility.cc

+               (lcs_header_to_remove == Http::Headers::get().ForwardedProto) ||
+               !token_sv.find(':')) {
+      // If pseudo headers or X-Forwarded* headers are nominated, this could be
+      // an invalid request. Reject the request


can we comment for posterity why we disallow these? If it's in the spec, we can link. If we just think it's high risk we can say that too.

Ping on this one?

alyssawilk · 2019-11-12T18:33:48Z

source/common/http/utility.cc

+               !token_sv.find(':')) {
+      // If pseudo headers or X-Forwarded* headers are nominated, this could be
+      // an invalid request. Reject the request
+      return false;


should we trace log this as well for consistency?

Yep. Added a trace log here

alyssawilk · 2019-11-12T18:38:24Z

source/common/http/utility.cc

+
+          const absl::string_view header_sv = StringUtil::trim(header_value);
+
+          // If trailers exist in the TE value tokens, keep the header, removing any other values


here I assume we don't want to use StringUtil::caseFindToken because it doesn't allow the limitation.
I think that's the right call just because while we could refactor to pass in MAX_ALLOWED_TE_VALUE_SIZE it'd result in complicated return values?
I'd still use StringUtil::splitToken though.

alyssawilk · 2019-11-12T18:39:17Z

source/common/http/utility.cc

+
+        if (keep_header) {
+          nominated_header->value().clear();
+          nominated_header->value().setCopy(Http::Headers::get().TEValues.Trailers.data(),


I believe setCopy implicitly clears

I will remove the clear call

alyssawilk · 2019-11-12T18:41:56Z

source/common/http/utility.cc

+    if (new_value.empty()) {
+      headers.removeConnection();
+    } else {
+      headers.Connection()->value(new_value);


does removeTokens remove empty fields?
Maybe a unit test for "foo, ,bar" and removing foo and bar?

alyssawilk · 2019-11-12T18:42:26Z

test/integration/http_integration.cc

 absl::optional<uint64_t>
 HttpIntegrationTest::waitForNextUpstreamRequest(const std::vector<uint64_t>& upstream_indices,
                                                std::chrono::milliseconds connection_wait_timeout) {
+


I think you can revert this change?

Yep, will clean this up.

abaptiste · 2019-11-12T19:50:00Z

/azp run envoy-macos

azure-pipelines · 2019-11-12T19:50:05Z

Commenter does not have sufficient privileges for PR 8862 in repo envoyproxy/envoy

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

alyssawilk

Two quick thoughts, and I owe you a final pass. As I'm out tomorrow and at EnvoyCon next week it may be laggy - sorry in advance!

alyssawilk · 2019-11-13T20:28:47Z

docs/root/intro/version_history.rst


 1.13.0 (pending)
 ================
+* http: added the ability to sanitize headers nominated by the Connection header. This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true.


mind moving this down? we try for alphabetic order here

Regarding the X-Forwarded for headers. I added a comment below the existing one. I will replace the lines on L445-446 with the updated comment.

See you at EnvoyCon! :)

alyssawilk · 2019-11-13T20:29:34Z

source/common/http/utility.cc

+               (lcs_header_to_remove == Http::Headers::get().ForwardedProto) ||
+               !token_sv.find(':')) {
+      // If pseudo headers or X-Forwarded* headers are nominated, this could be
+      // an invalid request. Reject the request


Ping on this one?

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

alyssawilk

one final nit, and one optional, and you're good to go!

alyssawilk · 2019-11-14T16:19:23Z

docs/root/intro/version_history.rst

 * health check: gRPC health checker sets the gRPC deadline to the configured timeout duration.
 * http: support :ref:`auto_host_rewrite_header<envoy_api_field_config.filter.http.dynamic_forward_proxy.v2alpha.PerRouteConfig.auto_host_rewrite_header>` in the dynamic forward proxy.
 * logger: added :ref:`--log-format-escaped <operations_cli>` command line option to escape newline characters in application logs.
+* http: added the ability to sanitize headers nominated by the Connection header. This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true.


Sorry, but can you move this to above line 12 (http: support")?
Also I think backticks around the runtime name is standard

ugh, vim sort failed me :( Will fix ... again.

alyssawilk · 2019-11-14T18:46:52Z

test/integration/header_integration_test.cc

 }
+
+// Validates TE header is forwarded if it contains a supported value
+TEST_P(HeaderIntegrationTest, TestTeHeaderPassthrough) {


You know, it only occurs to me now, but we should probably leave one of these tests here to verify connection sanitization works end to end, and do the rest as unit tests of the new function in test/common/http/utility_test.cc
I'm going to say this is 100% optional, since you've done all the work and I forgot to call it out like 8 reviews ago, but if you want to earn alyssawilk brownie points (probably not actually redeemable for brownies) it would be cheaper/faster for CI.

lol :) It's a deal!!

- Move tests to utility_test.cc Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov · 2019-11-15T02:49:18Z

test/common/http/utility_test.cc

+  };
+  // Expect that the set of headers is valid and can be sanitized
+  EXPECT_TRUE(Utility::sanitizeConnectionHeader(request_headers));
+}


My brownie points are not nearly as valuable as Alyssa's. But here a last suggestion I have. I think it will make these tests more informative by also checking the expected header map after sanitization. You can add something like this:

TestHeaderMapImpl sanitized_headers = { {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "no-headers.com"}, {"x-request-foo", "downstram"}, {"connection", "te,close"}, {"te", "trailers"}, }; EXPECT_EQ(sanitized_headers, request_headers);

Note that multivalue headers are compared as single value, so no space between "te,close".
I really appreciate your patience here.

Likewise. I will update. Thanks for the suggestion.

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

abaptiste · 2019-11-15T20:44:54Z

/retest

repokitteh-read-only · 2019-11-15T20:44:58Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8862 (comment) was created by @abaptiste.

see: more, trace.

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

alyssawilk

Fantastic work, thanks especially for the extra test work at the end :-)

Sanitize headers nominated by the Connection header

29b01f1

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

mattklein123 assigned alyssawilk and PiotrSikora Nov 3, 2019

Reset the TE header only if other values exist

06726ef

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov requested changes Nov 4, 2019

View reviewed changes

Address PR Comments

2d3bcca

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov requested changes Nov 5, 2019

View reviewed changes

Address PR Comments

00f8e5e

- Remove CaseInsensitiveCompare in favor of == operator Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

abaptiste requested a review from yanavlasov November 6, 2019 16:48

abaptiste added 2 commits November 6, 2019 16:50

Merge branch 'master' into te_header_sanitization

8484cd5

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Merge branch 'master' into te_header_sanitzation

571e2f8

yanavlasov requested changes Nov 7, 2019

View reviewed changes

abaptiste added 2 commits November 7, 2019 18:37

Address PR Comments

a35b697

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Merge branch 'master' into te_header_sanitzation

1ab0d61

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov requested changes Nov 8, 2019

View reviewed changes

abaptiste added 2 commits November 8, 2019 17:38

Merge branch 'master' into te_header_sanitzation

6174482

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Address PR Comments

26bfd63

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov previously approved these changes Nov 9, 2019

View reviewed changes

alyssawilk reviewed Nov 12, 2019

View reviewed changes

alyssawilk added the waiting label Nov 12, 2019

Address PR Comments

e8cd453

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

abaptiste dismissed yanavlasov’s stale review via e8cd453 November 12, 2019 23:31

repokitteh-read-only bot removed the waiting label Nov 12, 2019

abaptiste added 2 commits November 12, 2019 23:49

Add an comment regarding the X-Forwarded* checks

beb9efd

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Remove extra space after a period.

3f095c6

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

abaptiste requested review from alyssawilk and yanavlasov November 13, 2019 20:23

alyssawilk reviewed Nov 13, 2019

View reviewed changes

abaptiste added 2 commits November 13, 2019 21:56

Address PR Comments

a20cbe6

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Merge branch 'master' into te_header_sanitzation

7f367f9

alyssawilk reviewed Nov 14, 2019

View reviewed changes

Address PR Comments

a09025e

- Move tests to utility_test.cc Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov requested changes Nov 15, 2019

View reviewed changes

Update test validation

7667971

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

abaptiste added 2 commits November 15, 2019 20:58

Kick CI

3a5c2fa

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

Merge branch 'master' into te_header_sanitzation

c111350

Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>

yanavlasov approved these changes Nov 16, 2019

View reviewed changes

alyssawilk approved these changes Nov 18, 2019

View reviewed changes

alyssawilk merged commit b0cbc87 into envoyproxy:master Nov 18, 2019

abaptiste deleted the te_header_sanitzation branch November 20, 2019 19:28

alyssawilk mentioned this pull request Jul 7, 2020

envoy.reloadable_features.connection_header_sanitization deprecation #11933

Closed


		const absl::string_view header_sv = StringUtil::trim(header_value);

		// If trailers exist in the TE value tokens, keep the header, removing any other values

Conversation

abaptiste commented Nov 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment