http: rejecting CONNECT requests with bodies (envoyproxy#11530)

Risk Level: Low (connect only)
Testing: fixed up tests
Docs Changes: no
Release Notes: no
Runtime guard: no - connect in Alpha
Part of envoyproxy#1451

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/http/http1/codec_impl.cc

@@ -34,6 +34,7 @@ struct Http1ResponseCodeDetailValues {
  const absl::string_view ConnectionHeaderSanitization = "http1.connection_header_rejected";
  const absl::string_view InvalidUrl = "http1.invalid_url";
  const absl::string_view InvalidTransferEncoding = "http1.invalid_transfer_encoding";
+ const absl::string_view BodyDisallowed = "http1.body_disallowed";
 };
 
 struct Http1HeaderTypesValues {
@@ -660,16 +661,30 @@ int ConnectionImpl::onHeadersCompleteBase() {
  }
  }
  if (parser_.method == HTTP_CONNECT) {
+ if (request_or_response_headers.ContentLength()) {
+ if (request_or_response_headers.getContentLengthValue() == "0") {
+ request_or_response_headers.removeContentLength();
+ } else {
+ // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a
+ // CONNECT request has no defined semantics, and may be rejected.
+ error_code_ = Http::Code::BadRequest;
+ sendProtocolError(Http1ResponseCodeDetails::get().BodyDisallowed);
+ throw CodecProtocolException("http/1.1 protocol error: unsupported content length");
+ }
+ }
  ENVOY_CONN_LOG(trace, "codec entering upgrade mode for CONNECT request.", connection_);
  handling_upgrade_ = true;
  }
 
  // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject
  // transfer-codings it does not understand.
+ // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a
+ // CONNECT request has no defined semantics, and may be rejected.
  if (request_or_response_headers.TransferEncoding()) {
  const absl::string_view encoding = request_or_response_headers.getTransferEncodingValue();
- if (reject_unsupported_transfer_encodings_ &&
- !absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked)) {
+ if ((reject_unsupported_transfer_encodings_ &&
+ !absl::EqualsIgnoreCase(encoding, Headers::get().TransferEncodingValues.Chunked)) ||
+ parser_.method == HTTP_CONNECT) {
  error_code_ = Http::Code::NotImplemented;
  sendProtocolError(Http1ResponseCodeDetails::get().InvalidTransferEncoding);
  throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding");

test/common/http/http1/codec_impl_test.cc

@@ -1717,20 +1717,31 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithTEChunked) {
  NiceMock<MockRequestDecoder> decoder;
  EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
 
- // Connect with body is technically illegal, but Envoy does not inspect the
- // body to see if there is a non-zero byte chunk. It will instead pass it
- // through.
- // TODO(alyssawilk) track connect payload and block if this happens.
- Buffer::OwnedImpl expected_data("12345abcd");
- EXPECT_CALL(decoder, decodeHeaders_(_, false));
- EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false));
+ // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 CONNECT with body has no defined
+ // semantics: Envoy will reject chunked CONNECT requests.
  Buffer::OwnedImpl buffer(
  "CONNECT host:80 HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n12345abcd");
  auto status = codec_->dispatch(buffer);
- EXPECT_TRUE(status.ok());
+ EXPECT_TRUE(isCodecProtocolError(status));
+ EXPECT_EQ(status.message(), "http/1.1 protocol error: unsupported transfer encoding");
+}
+
+TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithNonZeroContentLength) {
+ initialize();
+
+ InSequence sequence;
+ NiceMock<MockRequestDecoder> decoder;
+ EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+ // Make sure we avoid the deferred_end_stream_headers_ optimization for
+ // requests-with-no-body.
+ Buffer::OwnedImpl buffer("CONNECT host:80 HTTP/1.1\r\ncontent-length: 1\r\n\r\nabcd");
+ auto status = codec_->dispatch(buffer);
+ EXPECT_TRUE(isCodecProtocolError(status));
+ EXPECT_EQ(status.message(), "http/1.1 protocol error: unsupported content length");
 }
 
-TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithContentLength) {
+TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithZeroContentLength) {
  initialize();
 
  InSequence sequence;