From c7de58d865020e1484677fa79c48e99459fe647d Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 16 Jan 2016 15:11:48 +0900 Subject: [PATCH 1/2] Validate :authroity, host, and :scheme value more strictly --- lib/nghttp2_http.c | 102 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) diff --git a/lib/nghttp2_http.c b/lib/nghttp2_http.c index 2abcd50f..6aee90f0 100644 --- a/lib/nghttp2_http.c +++ b/lib/nghttp2_http.c @@ -284,9 +284,101 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_nv *nv, return 0; } +/* Generated by genauthroitychartbl.py */ +static char VALID_AUTHORITY_CHARS[] = { + 0 /* NUL */, 0 /* SOH */, 0 /* STX */, 0 /* ETX */, 0 /* EOT */, + 0 /* ENQ */, 0 /* ACK */, 0 /* BEL */, 0 /* BS */, 0 /* HT */, + 0 /* LF */, 0 /* VT */, 0 /* FF */, 0 /* CR */, 0 /* SO */, + 0 /* SI */, 0 /* DLE */, 0 /* DC1 */, 0 /* DC2 */, 0 /* DC3 */, + 0 /* DC4 */, 0 /* NAK */, 0 /* SYN */, 0 /* ETB */, 0 /* CAN */, + 0 /* EM */, 0 /* SUB */, 0 /* ESC */, 0 /* FS */, 0 /* GS */, + 0 /* RS */, 0 /* US */, 0 /* SPC */, 1 /* ! */, 0 /* " */, + 0 /* # */, 1 /* $ */, 1 /* % */, 1 /* & */, 1 /* ' */, + 1 /* ( */, 1 /* ) */, 1 /* * */, 1 /* + */, 1 /* , */, + 1 /* - */, 1 /* . */, 0 /* / */, 1 /* 0 */, 1 /* 1 */, + 1 /* 2 */, 1 /* 3 */, 1 /* 4 */, 1 /* 5 */, 1 /* 6 */, + 1 /* 7 */, 1 /* 8 */, 1 /* 9 */, 1 /* : */, 1 /* ; */, + 0 /* < */, 1 /* = */, 0 /* > */, 0 /* ? */, 1 /* @ */, + 1 /* A */, 1 /* B */, 1 /* C */, 1 /* D */, 1 /* E */, + 1 /* F */, 1 /* G */, 1 /* H */, 1 /* I */, 1 /* J */, + 1 /* K */, 1 /* L */, 1 /* M */, 1 /* N */, 1 /* O */, + 1 /* P */, 1 /* Q */, 1 /* R */, 1 /* S */, 1 /* T */, + 1 /* U */, 1 /* V */, 1 /* W */, 1 /* X */, 1 /* Y */, + 1 /* Z */, 1 /* [ */, 0 /* \ */, 1 /* ] */, 0 /* ^ */, + 1 /* _ */, 0 /* ` */, 1 /* a */, 1 /* b */, 1 /* c */, + 1 /* d */, 1 /* e */, 1 /* f */, 1 /* g */, 1 /* h */, + 1 /* i */, 1 /* j */, 1 /* k */, 1 /* l */, 1 /* m */, + 1 /* n */, 1 /* o */, 1 /* p */, 1 /* q */, 1 /* r */, + 1 /* s */, 1 /* t */, 1 /* u */, 1 /* v */, 1 /* w */, + 1 /* x */, 1 /* y */, 1 /* z */, 0 /* { */, 0 /* | */, + 0 /* } */, 1 /* ~ */, 0 /* DEL */, 0 /* 0x80 */, 0 /* 0x81 */, + 0 /* 0x82 */, 0 /* 0x83 */, 0 /* 0x84 */, 0 /* 0x85 */, 0 /* 0x86 */, + 0 /* 0x87 */, 0 /* 0x88 */, 0 /* 0x89 */, 0 /* 0x8a */, 0 /* 0x8b */, + 0 /* 0x8c */, 0 /* 0x8d */, 0 /* 0x8e */, 0 /* 0x8f */, 0 /* 0x90 */, + 0 /* 0x91 */, 0 /* 0x92 */, 0 /* 0x93 */, 0 /* 0x94 */, 0 /* 0x95 */, + 0 /* 0x96 */, 0 /* 0x97 */, 0 /* 0x98 */, 0 /* 0x99 */, 0 /* 0x9a */, + 0 /* 0x9b */, 0 /* 0x9c */, 0 /* 0x9d */, 0 /* 0x9e */, 0 /* 0x9f */, + 0 /* 0xa0 */, 0 /* 0xa1 */, 0 /* 0xa2 */, 0 /* 0xa3 */, 0 /* 0xa4 */, + 0 /* 0xa5 */, 0 /* 0xa6 */, 0 /* 0xa7 */, 0 /* 0xa8 */, 0 /* 0xa9 */, + 0 /* 0xaa */, 0 /* 0xab */, 0 /* 0xac */, 0 /* 0xad */, 0 /* 0xae */, + 0 /* 0xaf */, 0 /* 0xb0 */, 0 /* 0xb1 */, 0 /* 0xb2 */, 0 /* 0xb3 */, + 0 /* 0xb4 */, 0 /* 0xb5 */, 0 /* 0xb6 */, 0 /* 0xb7 */, 0 /* 0xb8 */, + 0 /* 0xb9 */, 0 /* 0xba */, 0 /* 0xbb */, 0 /* 0xbc */, 0 /* 0xbd */, + 0 /* 0xbe */, 0 /* 0xbf */, 0 /* 0xc0 */, 0 /* 0xc1 */, 0 /* 0xc2 */, + 0 /* 0xc3 */, 0 /* 0xc4 */, 0 /* 0xc5 */, 0 /* 0xc6 */, 0 /* 0xc7 */, + 0 /* 0xc8 */, 0 /* 0xc9 */, 0 /* 0xca */, 0 /* 0xcb */, 0 /* 0xcc */, + 0 /* 0xcd */, 0 /* 0xce */, 0 /* 0xcf */, 0 /* 0xd0 */, 0 /* 0xd1 */, + 0 /* 0xd2 */, 0 /* 0xd3 */, 0 /* 0xd4 */, 0 /* 0xd5 */, 0 /* 0xd6 */, + 0 /* 0xd7 */, 0 /* 0xd8 */, 0 /* 0xd9 */, 0 /* 0xda */, 0 /* 0xdb */, + 0 /* 0xdc */, 0 /* 0xdd */, 0 /* 0xde */, 0 /* 0xdf */, 0 /* 0xe0 */, + 0 /* 0xe1 */, 0 /* 0xe2 */, 0 /* 0xe3 */, 0 /* 0xe4 */, 0 /* 0xe5 */, + 0 /* 0xe6 */, 0 /* 0xe7 */, 0 /* 0xe8 */, 0 /* 0xe9 */, 0 /* 0xea */, + 0 /* 0xeb */, 0 /* 0xec */, 0 /* 0xed */, 0 /* 0xee */, 0 /* 0xef */, + 0 /* 0xf0 */, 0 /* 0xf1 */, 0 /* 0xf2 */, 0 /* 0xf3 */, 0 /* 0xf4 */, + 0 /* 0xf5 */, 0 /* 0xf6 */, 0 /* 0xf7 */, 0 /* 0xf8 */, 0 /* 0xf9 */, + 0 /* 0xfa */, 0 /* 0xfb */, 0 /* 0xfc */, 0 /* 0xfd */, 0 /* 0xfe */, + 0 /* 0xff */ +}; + +static int check_authority(const uint8_t *value, size_t len) { + const uint8_t *last; + for (last = value + len; value != last; ++value) { + if (!VALID_AUTHORITY_CHARS[*value]) { + return 0; + } + } + return 1; +} + +static int check_scheme(const uint8_t *value, size_t len) { + const uint8_t *last; + if (len == 0) { + return 0; + } + + if (!(('A' <= *value && *value <= 'Z') || ('a' <= *value && *value <= 'z'))) { + return 0; + } + + last = value + len; + ++value; + + for (; value != last; ++value) { + if (!(('A' <= *value && *value <= 'Z') || + ('a' <= *value && *value <= 'z') || + ('0' <= *value && *value <= '9') || *value == '+' || *value == '-' || + *value == '.')) { + return 0; + } + } + return 1; +} + int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream, nghttp2_frame *frame, nghttp2_nv *nv, int token, int trailer) { + int rv; + /* We are strict for pseudo header field. One bad character should lead to fail. OTOH, we should be a bit forgiving for regular headers, since existing public internet has so much illegal @@ -313,7 +405,15 @@ int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream, return NGHTTP2_ERR_IGN_HTTP_HEADER; } - if (!nghttp2_check_header_value(nv->value, nv->valuelen)) { + if (token == NGHTTP2_TOKEN__AUTHORITY || token == NGHTTP2_TOKEN_HOST) { + rv = check_authority(nv->value, nv->valuelen); + } else if (token == NGHTTP2_TOKEN__SCHEME) { + rv = check_scheme(nv->value, nv->valuelen); + } else { + rv = nghttp2_check_header_value(nv->value, nv->valuelen); + } + + if (rv == 0) { assert(nv->namelen > 0); if (nv->name[0] == ':') { return NGHTTP2_ERR_HTTP_HEADER; From b202e066fd84722d0ecbbfdf2b6ff511be5b2858 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 16 Jan 2016 15:59:24 +0900 Subject: [PATCH 2/2] 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] != '/' &&