From b202e066fd84722d0ecbbfdf2b6ff511be5b2858 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 16 Jan 2016 15:59:24 +0900 Subject: [PATCH] nghttpx: Don't allow certain characters in host and :scheme header field For HTTP/2, we do this validation in libnghttp2. http-parser does this partially, when it parses URI, but it does not do anything for Host header field. libspdylay does not perform anything. So do some additional validation for HTTP/1 and SPDY cases. integration tests were also added to make sure they work. --- integration-tests/nghttpx_http1_test.go | 60 +++++++++++++++++++++++++ integration-tests/nghttpx_http2_test.go | 40 +++++++++++++++++ integration-tests/nghttpx_spdy_test.go | 42 ++++++++++++++++- src/shrpx_https_upstream.cc | 18 ++++++-- src/shrpx_spdy_upstream.cc | 21 +++++++++ 5 files changed, 177 insertions(+), 4 deletions(-) diff --git a/integration-tests/nghttpx_http1_test.go b/integration-tests/nghttpx_http1_test.go index 145848fa..b47e7399 100644 --- a/integration-tests/nghttpx_http1_test.go +++ b/integration-tests/nghttpx_http1_test.go @@ -185,6 +185,66 @@ func TestH1H1HostRewrite(t *testing.T) { } } +// TestH1H1BadHost tests that server rejects request including bad +// characters in host header field. +func TestH1H1BadHost(t *testing.T) { + st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + if _, err := io.WriteString(st.conn, "GET / HTTP/1.1\r\nTest-Case: TestH1H1HBadHost\r\nHost: foo\"bar\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) + } + if got, want := resp.StatusCode, 400; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + +// TestH1H1BadAuthority tests that server rejects request including +// bad characters in authority component of requset URI. +func TestH1H1BadAuthority(t *testing.T) { + st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + if _, err := io.WriteString(st.conn, "GET http://foo\"bar/ HTTP/1.1\r\nTest-Case: TestH1H1HBadAuthority\r\nHost: foobar\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) + } + if got, want := resp.StatusCode, 400; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + +// TestH1H1BadScheme tests that server rejects request including +// bad characters in scheme component of requset URI. +func TestH1H1BadScheme(t *testing.T) { + st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + if _, err := io.WriteString(st.conn, "GET http*://example.com/ HTTP/1.1\r\nTest-Case: TestH1H1HBadScheme\r\nHost: example.com\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) + } + if got, want := resp.StatusCode, 400; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + // TestH1H1HTTP10 tests that server can accept HTTP/1.0 request // without Host header field func TestH1H1HTTP10(t *testing.T) { diff --git a/integration-tests/nghttpx_http2_test.go b/integration-tests/nghttpx_http2_test.go index 8871c775..c0a19a3f 100644 --- a/integration-tests/nghttpx_http2_test.go +++ b/integration-tests/nghttpx_http2_test.go @@ -606,6 +606,46 @@ func TestH2H1InvalidMethod(t *testing.T) { } } +// TestH2H1BadAuthority tests that server rejects request including +// bad characters in :authority header field. +func TestH2H1BadAuthority(t *testing.T) { + st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + res, err := st.http2(requestParam{ + name: "TestH2H1BadAuthority", + authority: `foo\bar`, + }) + if err != nil { + t.Fatalf("Error st.http2() = %v", err) + } + if got, want := res.errCode, http2.ErrCodeProtocol; got != want { + t.Errorf("res.errCode: %v; want %v", got, want) + } +} + +// TestH2H1BadScheme tests that server rejects request including +// bad characters in :scheme header field. +func TestH2H1BadScheme(t *testing.T) { + st := newServerTester(nil, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + res, err := st.http2(requestParam{ + name: "TestH2H1BadScheme", + scheme: "http*", + }) + if err != nil { + t.Fatalf("Error st.http2() = %v", err) + } + if got, want := res.errCode, http2.ErrCodeProtocol; got != want { + t.Errorf("res.errCode: %v; want %v", got, want) + } +} + // TestH2H1AssembleCookies tests that crumbled cookies in HTTP/2 // request is assembled into 1 when forwarding to HTTP/1 backend link. func TestH2H1AssembleCookies(t *testing.T) { diff --git a/integration-tests/nghttpx_spdy_test.go b/integration-tests/nghttpx_spdy_test.go index e9bf1afc..2931dc31 100644 --- a/integration-tests/nghttpx_spdy_test.go +++ b/integration-tests/nghttpx_spdy_test.go @@ -1,8 +1,8 @@ package nghttp2 import ( - "golang.org/x/net/http2/hpack" "github.com/tatsuhiro-t/spdy" + "golang.org/x/net/http2/hpack" "net/http" "testing" ) @@ -230,6 +230,46 @@ func TestS3H1InvalidMethod(t *testing.T) { } } +// TestS3H1BadHost tests that server rejects request including bad +// character in :host header field. +func TestS3H1BadHost(t *testing.T) { + st := newServerTesterTLS([]string{"--npn-list=spdy/3.1"}, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + res, err := st.spdy(requestParam{ + name: "TestS3H1BadHost", + authority: `foo\bar`, + }) + if err != nil { + t.Fatalf("Error st.spdy() = %v", err) + } + if got, want := res.status, 400; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + +// TestS3H1BadScheme tests that server rejects request including bad +// character in :scheme header field. +func TestS3H1BadScheme(t *testing.T) { + st := newServerTesterTLS([]string{"--npn-list=spdy/3.1"}, t, func(w http.ResponseWriter, r *http.Request) { + t.Errorf("server should not forward this request") + }) + defer st.Close() + + res, err := st.spdy(requestParam{ + name: "TestS3H1BadScheme", + scheme: `http*`, + }) + if err != nil { + t.Fatalf("Error st.spdy() = %v", err) + } + if got, want := res.status, 400; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + // TestS3H1ReqPhaseSetHeader tests mruby request phase hook // modifies request header fields. func TestS3H1ReqPhaseSetHeader(t *testing.T) { diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index a39cd319..59dfcdaf 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -271,11 +271,24 @@ int htp_hdrs_completecb(http_parser *htp) { return -1; } - if (req.http_major == 1 && req.http_minor == 1 && - !req.fs.header(http2::HD_HOST)) { + auto host = req.fs.header(http2::HD_HOST); + + if (req.http_major == 1 && req.http_minor == 1 && !host) { return -1; } + if (host) { + const auto &value = host->value; + // Not allow at least '"' or '\' in host. They are illegal in + // authority component, also they cause headaches when we put them + // in quoted-string. + if (std::find_if(std::begin(value), std::end(value), [](char c) { + return c == '"' || c == '\\'; + }) != std::end(value)) { + return -1; + } + } + downstream->inspect_http1_request(); if (method != HTTP_CONNECT) { @@ -301,7 +314,6 @@ int htp_hdrs_completecb(http_parser *htp) { req.path = http2::rewrite_clean_path(std::begin(path), std::end(path)); } - auto host = req.fs.header(http2::HD_HOST); if (host) { req.authority = host->value; } diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 8c89f6d7..b41bee1d 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -218,6 +218,27 @@ void on_ctrl_recv_callback(spdylay_session *session, spdylay_frame_type type, return; } + if (std::find_if(std::begin(host->value), std::end(host->value), + [](char c) { return c == '"' || c == '\\'; }) != + std::end(host->value)) { + if (upstream->error_reply(downstream, 400) != 0) { + ULOG(FATAL, upstream) << "error_reply failed"; + } + return; + } + + if (scheme) { + for (auto c : scheme->value) { + if (!(util::is_alpha(c) || util::is_digit(c) || c == '+' || c == '-' || + c == '.')) { + if (upstream->error_reply(downstream, 400) != 0) { + ULOG(FATAL, upstream) << "error_reply failed"; + } + return; + } + } + } + // For other than CONNECT method, path must start with "/", except // for OPTIONS method, which can take "*" as path. if (!is_connect && path->value[0] != '/' &&