nghttpx: Stricter transfer-encoding checks

integration-tests/nghttpx_http1_test.go

@@ -1490,3 +1490,142 @@ func TestH1H1ChunkedEndsPrematurely(t *testing.T) {
 		t.Fatal("st.http1() should fail")
 	}
 }
+
+// TestH1H1RequestMalformedTransferEncoding tests that server rejects
+// request which contains malformed transfer-encoding.
+func TestH1H1RequestMalformedTransferEncoding(t *testing.T) {
+	opts := options{
+		handler: func(w http.ResponseWriter, r *http.Request) {
+			t.Errorf("server should not forward bad request")
+		},
+	}
+	st := newServerTester(t, opts)
+	defer st.Close()
+
+	if _, err := io.WriteString(st.conn, fmt.Sprintf("GET / HTTP/1.1\r\nHost: %v\r\nTest-Case: TestH1H1RequestMalformedTransferEncoding\r\nTransfer-Encoding: ,chunked\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)
+	}
+
+	defer resp.Body.Close()
+
+	if got, want := resp.StatusCode, http.StatusBadRequest; got != want {
+		t.Errorf("status: %v; want %v", got, want)
+	}
+}
+
+// TestH1H1ResponseMalformedTransferEncoding tests a request fails if
+// its response contains malformed transfer-encoding.
+func TestH1H1ResponseMalformedTransferEncoding(t *testing.T) {
+	opts := options{
+		handler: func(w http.ResponseWriter, r *http.Request) {
+			hj, ok := w.(http.Hijacker)
+			if !ok {
+				http.Error(w, "Could not hijack the connection", http.StatusInternalServerError)
+				return
+			}
+			conn, bufrw, err := hj.Hijack()
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+				return
+			}
+			defer conn.Close()
+			if _, err := bufrw.WriteString("HTTP/1.1 200\r\nTransfer-Encoding: ,chunked\r\n\r\n"); err != nil {
+				t.Fatalf("Error bufrw.WriteString() = %v", err)
+			}
+			bufrw.Flush()
+		},
+	}
+	st := newServerTester(t, opts)
+	defer st.Close()
+
+	res, err := st.http1(requestParam{
+		name: "TestH1H1ResponseMalformedTransferEncoding",
+	})
+	if err != nil {
+		t.Fatalf("Error st.http1() = %v", err)
+	}
+	if got, want := res.status, http.StatusBadGateway; got != want {
+		t.Errorf("res.status: %v; want %v", got, want)
+	}
+}
+
+// TestH1H1ResponseUnknownTransferEncoding tests a request succeeds if
+// its response contains unknown transfer-encoding.
+func TestH1H1ResponseUnknownTransferEncoding(t *testing.T) {
+	opts := options{
+		handler: func(w http.ResponseWriter, r *http.Request) {
+			hj, ok := w.(http.Hijacker)
+			if !ok {
+				http.Error(w, "Could not hijack the connection", http.StatusInternalServerError)
+				return
+			}
+			conn, bufrw, err := hj.Hijack()
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+				return
+			}
+			defer conn.Close()
+			if _, err := bufrw.WriteString("HTTP/1.1 200\r\nTransfer-Encoding: foo\r\n\r\n"); err != nil {
+				t.Fatalf("Error bufrw.WriteString() = %v", err)
+			}
+			bufrw.Flush()
+		},
+	}
+	st := newServerTester(t, opts)
+	defer st.Close()
+
+	if _, err := io.WriteString(st.conn, fmt.Sprintf("GET / HTTP/1.1\r\nHost: %v\r\nTest-Case: TestH1H1ResponseUnknownTransferEncoding\r\n\r\n",
+		st.authority)); err != nil {
+		t.Fatalf("Error: io.WriteString() = %v", err)
+	}
+
+	r := bufio.NewReader(st.conn)
+
+	resp := make([]byte, 4096)
+
+	resplen, err := r.Read(resp)
+	if err != nil {
+		t.Fatalf("Error: r.Read() = %v", err)
+	}
+
+	resp = resp[:resplen]
+
+	const expect = "HTTP/1.1 200 OK\r\nTransfer-Encoding: foo\r\nConnection: close\r\nServer: nghttpx\r\nVia: 1.1 nghttpx\r\n\r\n"
+
+	if got, want := string(resp), expect; got != want {
+		t.Errorf("resp = %v, want %v", got, want)
+	}
+}
+
+// TestH1H1RequestHTTP10TransferEncoding tests that server rejects
+// HTTP/1.0 request which contains transfer-encoding.
+func TestH1H1RequestHTTP10TransferEncoding(t *testing.T) {
+	opts := options{
+		handler: func(w http.ResponseWriter, r *http.Request) {
+			t.Errorf("server should not forward bad request")
+		},
+	}
+	st := newServerTester(t, opts)
+	defer st.Close()
+
+	if _, err := io.WriteString(st.conn, "GET / HTTP/1.0\r\nTest-Case: TestH1H1RequestHTTP10TransferEncoding\r\nTransfer-Encoding: chunked\r\n\r\n"); 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)
+	}
+
+	defer resp.Body.Close()
+
+	if got, want := resp.StatusCode, http.StatusBadRequest; got != want {
+		t.Errorf("status: %v; want %v", got, want)
+	}
+}

src/http2.cc

@@ -1044,12 +1044,39 @@ InputIt skip_to_right_dquote(InputIt first, InputIt last) {
     switch (*first) {
     case '"':
       return first;
+      // quoted-pair
     case '\\':
       ++first;
       if (first == last) {
         return first;
       }
+
+      switch (*first) {
+      case '\t':
+      case ' ':
+        break;
+      default:
+        if ((0x21 <= *first && *first <= 0x7e) /* VCHAR */ ||
+            (0x80 <= *first && *first <= 0xff) /* obs-text */) {
+          break;
+        }
+
+        return last;
+      }
+
       break;
+      // qdtext
+    case '\t':
+    case ' ':
+    case '!':
+      break;
+    default:
+      if ((0x23 <= *first && *first <= 0x5b) ||
+          (0x5d <= *first && *first <= 0x7e)) {
+        break;
+      }
+
+      return last;
     }
     ++first;
   }
@@ -1957,6 +1984,108 @@ bool legacy_http1(int major, int minor) {
   return major <= 0 || (major == 1 && minor == 0);
 }
 
+bool check_transfer_encoding(const StringRef &s) {
+  if (s.empty()) {
+    return false;
+  }
+
+  auto it = std::begin(s);
+
+  for (;;) {
+    // token
+    if (!util::in_token(*it)) {
+      return false;
+    }
+
+    ++it;
+
+    for (; it != std::end(s) && util::in_token(*it); ++it)
+      ;
+
+    if (it == std::end(s)) {
+      return true;
+    }
+
+    for (;;) {
+      // OWS
+      it = skip_lws(it, std::end(s));
+      if (it == std::end(s)) {
+        return false;
+      }
+
+      if (*it == ',') {
+        ++it;
+
+        it = skip_lws(it, std::end(s));
+        if (it == std::end(s)) {
+          return false;
+        }
+
+        break;
+      }
+
+      if (*it != ';') {
+        return false;
+      }
+
+      ++it;
+
+      // transfer-parameter follows
+
+      // OWS
+      it = skip_lws(it, std::end(s));
+      if (it == std::end(s)) {
+        return false;
+      }
+
+      // token
+      if (!util::in_token(*it)) {
+        return false;
+      }
+
+      ++it;
+
+      for (; it != std::end(s) && util::in_token(*it); ++it)
+        ;
+
+      if (it == std::end(s)) {
+        return false;
+      }
+
+      // No BWS allowed
+      if (*it != '=') {
+        return false;
+      }
+
+      ++it;
+
+      if (util::in_token(*it)) {
+        // token
+        ++it;
+
+        for (; it != std::end(s) && util::in_token(*it); ++it)
+          ;
+      } else if (*it == '"') {
+        // quoted-string
+        ++it;
+
+        it = skip_to_right_dquote(it, std::end(s));
+        if (it == std::end(s)) {
+          return false;
+        }
+
+        ++it;
+      } else {
+        return false;
+      }
+
+      if (it == std::end(s)) {
+        return true;
+      }
+    }
+  }
+}
+
 } // namespace http2
 
 } // namespace nghttp2

src/http2.h

@@ -444,6 +444,11 @@ StringRef make_websocket_accept_token(uint8_t *dest, const StringRef &key);
 // HTTP/0.9 or HTTP/1.0).
 bool legacy_http1(int major, int minor);
 
+// Returns true if transfer-encoding field value |s| conforms RFC
+// strictly.  This function does not allow empty value, BWS, and empty
+// list elements.
+bool check_transfer_encoding(const StringRef &s);
+
 } // namespace http2
 
 } // namespace nghttp2

src/http2_test.cc

@@ -1189,4 +1189,61 @@ void test_http2_contains_trailers(void) {
   CU_ASSERT(http2::contains_trailers(StringRef::from_lit(",trailers")));
 }
 
+void test_http2_check_transfer_encoding(void) {
+  CU_ASSERT(http2::check_transfer_encoding(StringRef::from_lit("chunked")));
+  CU_ASSERT(http2::check_transfer_encoding(StringRef::from_lit("foo,chunked")));
+  CU_ASSERT(
+      http2::check_transfer_encoding(StringRef::from_lit("foo,  chunked")));
+  CU_ASSERT(
+      http2::check_transfer_encoding(StringRef::from_lit("foo   ,  chunked")));
+  CU_ASSERT(
+      http2::check_transfer_encoding(StringRef::from_lit("chunked;foo=bar")));
+  CU_ASSERT(
+      http2::check_transfer_encoding(StringRef::from_lit("chunked ; foo=bar")));
+  CU_ASSERT(http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="bar")")));
+  CU_ASSERT(http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="\bar\"";FOO=BAR)")));
+  CU_ASSERT(
+      http2::check_transfer_encoding(StringRef::from_lit(R"(chunked;foo="")")));
+  CU_ASSERT(http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="bar" , gzip)")));
+
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef{}));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit(",chunked")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("chunked,")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("chunked, ")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit("foo,,chunked")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit("chunked;foo")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("chunked;")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit("chunked;foo=bar;")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit("chunked;?=bar")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit("chunked;=bar")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("chunked;;")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("chunked?")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit(",")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit(" ")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit(";")));
+  CU_ASSERT(!http2::check_transfer_encoding(StringRef::from_lit("\"")));
+  CU_ASSERT(!http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="bar)")));
+  CU_ASSERT(!http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="bar\)")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit(R"(chunked;foo="bar\)"
+                                                          "\x0a"
+                                                          R"(")")));
+  CU_ASSERT(
+      !http2::check_transfer_encoding(StringRef::from_lit(R"(chunked;foo=")"
+                                                          "\x0a"
+                                                          R"(")")));
+  CU_ASSERT(!http2::check_transfer_encoding(
+      StringRef::from_lit(R"(chunked;foo="bar",,gzip)")));
+}
+
 } // namespace shrpx

src/http2_test.h

@@ -47,6 +47,7 @@ void test_http2_rewrite_clean_path(void);
 void test_http2_get_pure_path_component(void);
 void test_http2_construct_push_component(void);
 void test_http2_contains_trailers(void);
+void test_http2_check_transfer_encoding(void);
 
 } // namespace shrpx
 

src/shrpx-unittest.cc

@@ -109,6 +109,8 @@ int main(int argc, char *argv[]) {
                    shrpx::test_http2_construct_push_component) ||
       !CU_add_test(pSuite, "http2_contains_trailers",
                    shrpx::test_http2_contains_trailers) ||
+      !CU_add_test(pSuite, "http2_check_transfer_encoding",
+                   shrpx::test_http2_check_transfer_encoding) ||
       !CU_add_test(pSuite, "downstream_field_store_append_last_header",
                    shrpx::test_downstream_field_store_append_last_header) ||
       !CU_add_test(pSuite, "downstream_field_store_header",

src/shrpx_downstream.cc

@@ -864,9 +864,6 @@ void Downstream::inspect_http1_request() {
   auto transfer_encoding = req_.fs.header(http2::HD_TRANSFER_ENCODING);
   if (transfer_encoding) {
     req_.fs.content_length = -1;
-    if (util::iends_with_l(transfer_encoding->value, "chunked")) {
-      chunked_request_ = true;
-    }
   }
 
   auto expect = req_.fs.header(http2::HD_EXPECT);
@@ -879,9 +876,6 @@ void Downstream::inspect_http1_response() {
   auto transfer_encoding = resp_.fs.header(http2::HD_TRANSFER_ENCODING);
   if (transfer_encoding) {
     resp_.fs.content_length = -1;
-    if (util::iends_with_l(transfer_encoding->value, "chunked")) {
-      chunked_response_ = true;
-    }
   }
 }
 

src/shrpx_http_downstream_connection.cc

@@ -929,6 +929,11 @@ int htp_hdrs_completecb(llhttp_t *htp) {
 
   for (auto &kv : resp.fs.headers()) {
     kv.value = util::rstrip(balloc, kv.value);
+
+    if (kv.token == http2::HD_TRANSFER_ENCODING &&
+        !http2::check_transfer_encoding(kv.value)) {
+      return -1;
+    }
   }
 
   auto config = get_config();
@@ -1004,6 +1009,16 @@ int htp_hdrs_completecb(llhttp_t *htp) {
   resp.connection_close = !llhttp_should_keep_alive(htp);
   downstream->set_response_state(DownstreamState::HEADER_COMPLETE);
   downstream->inspect_http1_response();
+
+  if (htp->flags & F_CHUNKED) {
+    downstream->set_chunked_response(true);
+  }
+
+  auto transfer_encoding = resp.fs.header(http2::HD_TRANSFER_ENCODING);
+  if (transfer_encoding && !downstream->get_chunked_response()) {
+    resp.connection_close = true;
+  }
+
   if (downstream->get_upgraded()) {
     // content-length must be ignored for upgraded connection.
     resp.fs.content_length = -1;

src/shrpx_https_upstream.cc

@@ -345,6 +345,11 @@ int htp_hdrs_completecb(llhttp_t *htp) {
 
   for (auto &kv : req.fs.headers()) {
     kv.value = util::rstrip(balloc, kv.value);
+
+    if (kv.token == http2::HD_TRANSFER_ENCODING &&
+        !http2::check_transfer_encoding(kv.value)) {
+      return -1;
+    }
   }
 
   auto lgconf = log_config();
@@ -414,6 +419,16 @@ int htp_hdrs_completecb(llhttp_t *htp) {
 
   downstream->inspect_http1_request();
 
+  if (htp->flags & F_CHUNKED) {
+    downstream->set_chunked_request(true);
+  }
+
+  auto transfer_encoding = req.fs.header(http2::HD_TRANSFER_ENCODING);
+  if (transfer_encoding &&
+      http2::legacy_http1(req.http_major, req.http_minor)) {
+    return -1;
+  }
+
   auto faddr = handler->get_upstream_addr();
   auto config = get_config();