http/1.1 codec: Rejects requests w/ invalid HTTP header values #7306

dcarney · 2019-06-17T22:49:42Z

Description:
This PR addresses the behavior described in #7061 by modifying the HTTP/1.1 codec, such that it verifies the value of each HTTP header when onHeaderValue is called. The codec raises an exception, which results in a 400 response being returned if an invalid header value (per RFC 7230, section 3.2) is found. The actual validation is done via the nghttp2_check_header_value function, which is wrapped in Envoy::Http::HeaderUtility::headerIsValid.

(NOTE: this has been discussed as something that nghttp2 could do itself, but the issue seems to have languished. Also note that Go handles this: Go uses the httpguts.ValidHeaderFieldValue function (which is analogous to nghttp2_check_header_value) to ensure that all the HTTP header values conform to the relevant RFC specs before an http.Transport instance will round-trip the request.

Risk Level:
Low/medium

This stricter validation semantics are controlled with the envoy.reloadable_features.validate_header_values runtime-guarded feature. Originally, the PR used a new boolean on the HTTP connection manager proto to toggle the behavior, but during the course of PR review, it was decided that this would be more appropriate for a runtime guard.

Testing:
Unit tests were added to cover both the HeaderUtility changes as well as the HTTP codec changes. Manual testing against an Envoy build made from this PR's branch was performed using the repro steps decribed in #7061. During the manual testing, the "client Envoy" (or "envoy-one") returned a 400 as expected, rather than exhibiting the behavior described in the issue:

# run a newly-compiled envoy with an HTTP/1.1 listener 
# (on port :10001) and an H/2 upstream cluster

$ ./envoy-static -c config.yaml -l trace 2>&1                                               

# make the request with the invalid HTTP header(s):

$ cat invalid_request.txt | nc -n -i 1 127.0.0.1 10001      
HTTP/1.1 503 Service Unavailable
content-length: 85
content-type: text/plain
date: Thu, 11 Jul 2019 02:25:36 GMT
server: envoy

upstream connect error or disconnect/reset before headers. reset reason: remote reset%

# enable the new runtime feature to cause stricter header validation

$ curl --silent -X POST http://localhost:9901/runtime_modify\?envoy.reloadable_features.validate_header_values\=true
OK

# make the request with the invalid HTTP header(s) again:

$ cat invalid_request.txt | nc -n -i 1 127.0.0.1 10001 
HTTP/1.1 400 Bad Request
content-length: 0
connection: close

Success!

Docs Changes:

Release Notes: Updated
Fixes #7061

snowp

Thanks, this seems reasonable to me. CI seems broken, mind taking a look?

snowp · 2019-06-18T14:36:08Z

source/common/http/header_utility.cc

+      [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate {
+        const absl::string_view header_value = header.value().getStringView();
+        if (nghttp2_check_header_value(
+                reinterpret_cast<const uint8_t*>(std::string(header_value).data()),


Can you call .data on the string_view directly to avoid a temporary std::string?

snowp · 2019-06-18T14:49:49Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // header, returning a 400 response if any of the headers contain an invalid
+  // value. See `Header Fields `<https://tools.ietf.org/html/rfc7230#section-3.2>`_
+  // for details.
+  google.protobuf.BoolValue validate_headers = 33;


Normally we use the POD boolean type when the default is false

Sure, sounds good to me. I think I had been looking at some other fields on the conn manager like add_user_agent, which uses the proto wrapper type, but defaults to false.

rgs1 · 2019-06-18T20:17:21Z

source/common/http/conn_manager_impl.cc

@@ -598,6 +599,17 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
  // called with end_stream=true.
  maybeEndDecode(end_stream);

+  if (connection_manager_.config_.validateHeaders()) {
+    ENVOY_STREAM_LOG(trace, "validating header values\n{}", *this, *request_headers_);
+    if (!Http::HeaderUtility::validateHeaders(*request_headers_)) {


should we bump a stat here? might be useful

dcarney · 2019-06-18T23:12:49Z

@snowp Thanks for the feedback! I addressed your comments, and CI is sorted out now.

snowp · 2019-06-19T16:32:20Z

LGTM, though a stat would be nice. @alyssawilk do you wanna take a look?

alyssawilk

This is great - I think it makes Envoy much safer to do consistent header validation. I've got a couple of thoughts for today, and I'll do a closer pass at tests tomorrow.

alyssawilk · 2019-06-19T19:46:41Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // header, returning a 400 response if any of the headers contain an invalid
+  // value. See `Header Fields <https://tools.ietf.org/html/rfc7230#section-3.2>`
+  // for more details regarding the validity of HTTP headers.
+  bool validate_headers = 33;


We can sanity check with @mattklein123 , but if this is making Envoy more standards compliant I'd lean towards making it a permanent change (off by default, and then see if on by default works for people)

If so I think instead of making changes to the Envoy API, we should just runtime guard per
https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#runtime-guarding

runtime guard for this SGTM 👍

Okay. Just so I understand correctly, we'd be making this new behavior:

a runtime-guarded option only, and removing the option it from the connection manager proto, as I have it currently.

off by default

I'm basing that on the comments in the original issue about making it opt-in.

I was a little confused by this verbiage from the runtime guarding docs:

Generally all runtime guarded features will be set true when a release is cut)

alyssawilk · 2019-06-19T19:47:15Z

docs/root/intro/version_history.rst

@@ -30,6 +30,7 @@ Version history
 * http: mitigated a race condition with the :ref:`delayed_close_timeout<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.delayed_close_timeout>` where it could trigger while actively flushing a pending write buffer for a downstream connection.
 * http: added support for :ref:`preserve_external_request_id<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.preserve_external_request_id>` that represents whether the x-request-id should not be reset on edge entry inside mesh
 * http: changed `sendLocalReply` to send percent-encoded `GrpcMessage`.
+* http: added support for rejecting requests containing invalid HTTP header values.


So if we leave this in, we should reference the config field. If we runtime guard let's mention the string we guard it with.

Oops, I think we now have two release notes - how about removing this one since I think the upper one is correct?

actually this file looks odd in general - there seem to be a bunch of deltas which aren't yours. Bad merge?

alyssawilk · 2019-06-19T19:49:00Z

include/envoy/stream_info/stream_info.h

@@ -128,6 +128,8 @@ struct ResponseCodeDetailValues {
  // indicates that original "success" headers may have been sent downstream
  // despite the subsequent failure.
  const std::string LateUpstreamReset = "upstream_reset_after_response_started";
+  // The request was rejected due to invalid HTTP headers


Specifically, the headers had invalid characters, yes? If so let's be as specific as possible here and below

alyssawilk · 2019-06-19T19:51:35Z

source/common/http/conn_manager_impl.cc

@@ -598,6 +599,17 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
  // called with end_stream=true.
  maybeEndDecode(end_stream);

+  if (connection_manager_.config_.validateHeaders()) {


I do just want to call out this solves your problem because your client is using HTTP/1. If they use HTTP/2 with invalid characters, nghttp2 would reject the request before we got to the HCM decodeHeaders call.
I still think it's a good change, I just want to make sure the limitations are on your radar :-)

alyssawilk · 2019-06-19T19:55:51Z

source/common/http/conn_manager_impl.cc

@@ -598,6 +599,17 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
  // called with end_stream=true.
  maybeEndDecode(end_stream);

+  if (connection_manager_.config_.validateHeaders()) {
+    ENVOY_STREAM_LOG(trace, "validating header values\n{}", *this, *request_headers_);
+    if (!Http::HeaderUtility::validateHeaders(*request_headers_)) {


From a perf perspective, this ends up being a full redundant header scan for HTTP/2 as nghttp2 has already validated headers at this point. It might be worth moving this check in the HTTP/1 codec (maybe even doing onHeaderValue? rather than once at the end) and we could ASSERT that all characters are valid so that as we add more transports (QUIC etc.) we have consistent behavior.

@alyssawilk If I move this to the http/1.1 codec, it seems to make sense to handle it similarly to this code, which is doing similar albeit more limited, header value checking (it even references the same RFC section!). WDYT?

Yeah, I think with the runtime guard false, it does the old thing, and with the runtime guard true, it does your new thing.
runtime starts out false, we flip it with the release, and then remove old code the following release.
If there's any docs clean up which would make that more clear in future feel free to include it in this PR (with my thanks)

dcarney · 2019-06-27T17:12:33Z

Update on this PR: I talked with @alyssawilk on Slack about some of the integration test failures I'd been seeing with the runtime guarding (thanks a ton Alyssa!).

The TL;DR is that the runtime guarding has issues in certain integration tests due to its reliance on thread-local storage (which isn't available in all integration test threads, depending on the objects which are trying to use the runtime impl).

We came up with a workaround approach which will allow this PR to make forward progress, and the runtime/thread-local stuff (and some docs improvements) can be sorted out in another PR. I've got the implementation working correctly using the runtime guarding and incorporating the other PR feedback. Now I just need to thread things through correctly in the integration tests. 👍

stale · 2019-07-04T18:09:01Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

alyssawilk

Looking great! A few more thoughts on little ways we can clean this up before merging

alyssawilk · 2019-07-09T14:11:48Z

source/common/http/codec_client.cc

@@ -136,11 +136,12 @@ void CodecClient::onData(Buffer::Instance& data) {

 CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& connection,
                                 Upstream::HostDescriptionConstSharedPtr host,
-                                 Event::Dispatcher& dispatcher)
+                                 Event::Dispatcher& dispatcher, bool validate_header_values)


maybe strict_header_validation?
since we were validating a bit before, just not much

Hm, or maybe I'm pinging when you're not ready for me to take another pass. Let me know when I should take a look? :-)

Sure, I think strict_header_validation is a slightly more accurate description of what's going on.

validate_header_values is consistent with the runtime feature name, and all the other parameters/contructor args however. @alyssawilk Do you think I should find/replace them all, so we standardize on strict_header_validation?

That'd be great, if you're up for it.

Did a PR-wide s/validate_header_values/strict_header_validation/

alyssawilk · 2019-07-09T14:26:59Z

source/common/http/http1/codec_impl.cc

    throw CodecProtocolException("http/1.1 protocol error: header value contains NUL");
  }

+  if (validate_headers_) {


I think this can be an if-else with the above clause, since I think headerIsValid should catch the '\0'
Can you check? It'd mean that when we remove the old code path we can remove the extra header scan without worrying about a change in functionality

Yep, the HeaderUtility::headerIsValid function will catch the \0 char, along with any other ASCII char with decimal value [0-31] (with the exception of decimal value 9, which is allowed per the RFC).

alyssawilk · 2019-07-09T14:28:45Z

source/common/http/http1/codec_impl.cc

@@ -664,8 +688,13 @@ void ServerConnectionImpl::onBelowLowWatermark() {
  }
 }

-ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&)
-    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
+ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection,


Any reason to not just remove the old constructor and default validate_header_values to false?

Though now that I think about it, I think we'd be better off only having the new constructor and not having a default value. If we force all callers to pick a boolean we won't miss any code sites using the new code path.

It will probably mean tweaking a bunch of unit tests (you can set them all true) but hopefully you can search and replace there

The main reason I hadn't done that was the pervasiveness of the change: it involves a bunch of find/replace that really blew up the diff for the PR. (Due to the fact that it involves changing ServerConnectionImpl, ClientConnectionImpl, CodecClient/CodecClientProd, ConnectionManagerConfig, and associated mocks, unit tests, and call sites.

If you think that much code churn is warranted though, I can certainly do that, and make all the callers default to false (and default the unit test callers to true). I know originally we had talked about an overloaded constructor like the one included here, which didn't lead to nearly as much churn.

Yeah, that's actually a fair point about code churn given we'd want to go back to the single argument once we flip this to true and deprecate.

Do you feel confident from grepping around that you have all the call sites under source/...? If not, how about a hacked local version with a RELEASE_ASSERT in the old constructors to make sure we didn't miss any, and we'll call it a day?

Yep, I added RELEASE_ASSERT(false, ""); to the {Client,Server}ConnectionImpl constructors and ran Envoy to perform the steps outlined in the PR description (in the "testing" section). Worked great.

alyssawilk · 2019-07-09T14:29:00Z

source/common/http/http1/codec_impl.h

@@ -292,6 +296,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
  ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
                       Http1Settings settings, uint32_t max_request_headers_kb);

+  ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,


Ditto here, regarding the new constructor

alyssawilk · 2019-07-09T14:30:19Z

source/common/upstream/health_checker_impl.cc

@@ -336,7 +336,7 @@ Http::CodecClient::Type HttpHealthCheckerImpl::codecClientType(bool use_http2) {
 Http::CodecClient*
 ProdHttpHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {


I'd be inclined to snag the runtime value here too, so when we've flipped the default we can remove the old code path and always do full sanitization

I think you did this in the wrong block, for the gRPC health checker (always H2) rather than than here where it can be 1/1

Other than that, LGTM.
@snowp any other thoughts on your end?

@alyssawilk Good catch! Just fixed that.

alyssawilk · 2019-07-11T20:44:08Z

Oh also, can you do a master merge, and move your release note to 1.12.0? Thanks!

…alues Signed-off-by: Dylan Carney <dcarney@gmail.com>