From 0117e7f6fc1d40e9e97b4766791e42bac441e07d Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 15 Oct 2023 19:44:53 +0900 Subject: [PATCH] nghttpx: Stricter transfer-encoding checks --- integration-tests/nghttpx_http1_test.go | 139 ++++++++++++++++++++++++ src/http2.cc | 129 ++++++++++++++++++++++ src/http2.h | 5 + src/http2_test.cc | 57 ++++++++++ src/http2_test.h | 1 + src/shrpx-unittest.cc | 2 + src/shrpx_downstream.cc | 6 - src/shrpx_http_downstream_connection.cc | 15 +++ src/shrpx_https_upstream.cc | 15 +++ 9 files changed, 363 insertions(+), 6 deletions(-) diff --git a/integration-tests/nghttpx_http1_test.go b/integration-tests/nghttpx_http1_test.go index ca74a2e3..740396d8 100644 --- a/integration-tests/nghttpx_http1_test.go +++ b/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) + } +} diff --git a/src/http2.cc b/src/http2.cc index 7685879a..07d01448 100644 --- a/src/http2.cc +++ b/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 diff --git a/src/http2.h b/src/http2.h index fc88e4dd..e5b4f1b1 100644 --- a/src/http2.h +++ b/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 diff --git a/src/http2_test.cc b/src/http2_test.cc index 5760726b..f8be9f45 100644 --- a/src/http2_test.cc +++ b/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 diff --git a/src/http2_test.h b/src/http2_test.h index 0d63020e..382470d4 100644 --- a/src/http2_test.h +++ b/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 diff --git a/src/shrpx-unittest.cc b/src/shrpx-unittest.cc index 417ed54a..07373d57 100644 --- a/src/shrpx-unittest.cc +++ b/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", diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index 245e00b8..9ea52b4a 100644 --- a/src/shrpx_downstream.cc +++ b/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; - } } } diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index e464471e..bdb42943 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/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; diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 79cc22a9..49d20883 100644 --- a/src/shrpx_https_upstream.cc +++ b/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();