Merge branch 'validate-authority-scheme'

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) {

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) {

integration-tests/nghttpx_spdy_test.go

 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) {

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;

src/shrpx_https_upstream.cc

@@ -271,10 +271,23 @@ 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();
 
@@ -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;
       }

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] != '/' &&