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

@@ -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

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

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;
       }

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