From f1bec6f05c7cd44ec40b2f19e385bb3b1226f5ca Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Tue, 20 Jan 2015 22:55:01 +0900
Subject: [PATCH] nghttpx: Return 400 for multiple CLs for HTTP/1 upstream

---
 integration-tests/nghttpx_test.go | 23 +++++++++++++++++++++
 src/shrpx_downstream.cc           | 34 +++++++++++++++++++++++++------
 src/shrpx_downstream.h            |  6 ++++--
 src/shrpx_https_upstream.cc       |  4 +++-
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/integration-tests/nghttpx_test.go b/integration-tests/nghttpx_test.go
index c9157473..4c41ed6d 100644
--- a/integration-tests/nghttpx_test.go
+++ b/integration-tests/nghttpx_test.go
@@ -1,9 +1,11 @@
 package nghttp2
 
 import (
+	"bufio"
 	"fmt"
 	"github.com/bradfitz/http2"
 	"github.com/bradfitz/http2/hpack"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"testing"
@@ -46,6 +48,27 @@ func TestH1H1PlainGETClose(t *testing.T) {
 	}
 }
 
+func TestH1H1DuplicateRequestCL(t *testing.T) {
+	st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) {
+		t.Errorf("server should not forward bad request")
+	})
+	defer st.Close()
+
+	if _, err := io.WriteString(st.conn, fmt.Sprintf("GET / HTTP/1.1\r\nHost: %v\r\nTest-Case: TestH1H1DuplicateRequestCL\r\nContent-Length: 0\r\nContent-Length: 1\r\n\r\n", st.authority)); err != nil {
+		t.Fatalf("Error io.WriteString() = %v", err)
+	}
+
+	resp, err := http.ReadResponse(bufio.NewReader(st.conn), nil)
+	if err != nil {
+		t.Fatalf("Error http.ReadResponse() = %v", err)
+	}
+
+	want := 400
+	if got := resp.StatusCode; got != want {
+		t.Errorf("status: %v; want %v", got, want)
+	}
+}
+
 func TestH2H1PlainGET(t *testing.T) {
 	st := newServerTester(nil, t, noopHandler)
 	defer st.Close()
diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc
index 9103e6b6..1e6d5ed4 100644
--- a/src/shrpx_downstream.cc
+++ b/src/shrpx_downstream.cc
@@ -286,11 +286,31 @@ const std::string &Downstream::get_assembled_request_cookie() const {
   return assembled_request_cookie_;
 }
 
-void Downstream::index_request_headers() {
-  for (auto &kv : request_headers_) {
+int Downstream::index_request_headers() {
+  for (size_t i = 0; i < request_headers_.size(); ++i) {
+    auto &kv = request_headers_[i];
     util::inp_strlower(kv.name);
+
+    auto token = http2::lookup_token(
+        reinterpret_cast<const uint8_t *>(kv.name.c_str()), kv.name.size());
+    if (token < 0) {
+      continue;
+    }
+
+    http2::index_header(request_hdidx_, token, i);
+
+    if (token == http2::HD_CONTENT_LENGTH) {
+      auto len = util::parse_uint(kv.value);
+      if (len == -1) {
+        return -1;
+      }
+      if (request_content_length_ != -1 && request_content_length_ != len) {
+        return -1;
+      }
+      request_content_length_ = len;
+    }
   }
-  http2::index_headers(request_hdidx_, request_headers_);
+  return 0;
 }
 
 const Headers::value_type *Downstream::get_request_header(int token) const {
@@ -758,9 +778,11 @@ void Downstream::inspect_http1_request() {
     }
   }
   auto idx = request_hdidx_[http2::HD_TRANSFER_ENCODING];
-  if (idx != -1 &&
-      util::strifind(request_headers_[idx].value.c_str(), "chunked")) {
-    chunked_request_ = true;
+  if (idx != -1) {
+    request_content_length_ = -1;
+    if (util::strifind(request_headers_[idx].value.c_str(), "chunked")) {
+      chunked_request_ = true;
+    }
   }
 }
 
diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h
index f8bfcb5c..be8c856c 100644
--- a/src/shrpx_downstream.h
+++ b/src/shrpx_downstream.h
@@ -100,8 +100,10 @@ public:
   Headers crumble_request_cookie();
   void assemble_request_cookie();
   const std::string &get_assembled_request_cookie() const;
-  // Lower the request header field names and indexes request headers
-  void index_request_headers();
+  // Lower the request header field names and indexes request headers.
+  // If there is any invalid headers (e.g., multiple Content-Length
+  // having different values), returns -1.
+  int index_request_headers();
   // Returns pointer to the request header with the name |name|.  If
   // multiple header have |name| as name, return last occurrence from
   // the beginning.  If no such header is found, returns nullptr.
diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc
index a51df58e..a04e83c8 100644
--- a/src/shrpx_https_upstream.cc
+++ b/src/shrpx_https_upstream.cc
@@ -157,7 +157,9 @@ int htp_hdrs_completecb(http_parser *htp) {
     ULOG(INFO, upstream) << "HTTP request headers\n" << ss.str();
   }
 
-  downstream->index_request_headers();
+  if (downstream->index_request_headers() != 0) {
+    return -1;
+  }
 
   downstream->inspect_http1_request();