nghttpx: Robust PROXY protocol implementation

integration-tests/nghttpx_http2_test.go

@@ -885,6 +885,23 @@ func TestH2H1ProxyProtocolV1JustUnknown(t *testing.T) {
 	}
 }
 
+// TestH2H1ProxyProtocolV1TooLongLine tests PROXY protocol version 1
+// line longer than 107 bytes must be rejected
+func TestH2H1ProxyProtocolV1TooLongLine(t *testing.T) {
+	st := newServerTester([]string{"--accept-proxy-protocol", "--add-x-forwarded-for"}, t, noopHandler)
+	defer st.Close()
+
+	st.conn.Write([]byte("PROXY UNKNOWN ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff 65535 655350\r\n"))
+
+	_, err := st.http2(requestParam{
+		name: "TestH2H1ProxyProtocolV1TooLongLine",
+	})
+
+	if err == nil {
+		t.Fatalf("connection was not terminated")
+	}
+}
+
 // TestH2H1ProxyProtocolV1BadLineEnd tests that PROXY protocol version
 // 1 line ending without \r\n should be rejected.
 func TestH2H1ProxyProtocolV1BadLineEnd(t *testing.T) {
@@ -1006,6 +1023,24 @@ func TestH2H1ProxyProtocolV1InvalidDstPort(t *testing.T) {
 	}
 }
 
+// TestH2H1ProxyProtocolV1LeadingZeroPort tests that PROXY protocol
+// version 1 line with non zero port with leading zero should be
+// rejected.
+func TestH2H1ProxyProtocolV1LeadingZeroPort(t *testing.T) {
+	st := newServerTester([]string{"--accept-proxy-protocol"}, t, noopHandler)
+	defer st.Close()
+
+	st.conn.Write([]byte("PROXY TCP6 ::1 ::1 03000 8080\r\n"))
+
+	_, err := st.http2(requestParam{
+		name: "TestH2H1ProxyProtocolV1LeadingZeroPort",
+	})
+
+	if err == nil {
+		t.Fatalf("connection was not terminated")
+	}
+}
+
 // TestH2H1ProxyProtocolV1TooLargeSrcPort tests that PROXY protocol
 // containing too large src port should be rejected.
 func TestH2H1ProxyProtocolV1TooLargeSrcPort(t *testing.T) {

src/shrpx_client_handler.cc

@@ -104,6 +104,8 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) {
 }
 } // namespace
 
+int ClientHandler::noop() { return 0; }
+
 int ClientHandler::read_clear() {
   ev_timer_again(conn_.loop, &conn_.rt);
 
@@ -383,7 +385,8 @@ ClientHandler::ClientHandler(Worker *worker, int fd, SSL *ssl,
   ev_timer_again(conn_.loop, &conn_.rt);
 
   if (get_config()->accept_proxy_protocol) {
-    read_ = write_ = &ClientHandler::read_clear;
+    read_ = &ClientHandler::read_clear;
+    write_ = &ClientHandler::noop;
     on_read_ = &ClientHandler::proxy_protocol_read;
     on_write_ = &ClientHandler::upstream_noop;
   } else {
@@ -844,6 +847,17 @@ ssize_t parse_proxy_line_port(const uint8_t *first, const uint8_t *last) {
   auto p = first;
   int32_t port = 0;
 
+  if (p == last) {
+    return -1;
+  }
+
+  if (*p == '0') {
+    if (p + 1 != last && util::isDigit(*(p + 1))) {
+      return -1;
+    }
+    return 1;
+  }
+
   for (; p != last && util::isDigit(*p); ++p) {
     port *= 10;
     port += *p - '0';
@@ -869,6 +883,7 @@ int ClientHandler::on_proxy_protocol_finish() {
   return 0;
 }
 
+// http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt
 int ClientHandler::proxy_protocol_read() {
   if (LOG_ENABLED(INFO)) {
     CLOG(INFO, this) << "PROXY-protocol: Started";
@@ -876,18 +891,37 @@ int ClientHandler::proxy_protocol_read() {
 
   auto first = rb_.pos;
 
-  // NULL character really destroys getaddrinfo function.  We won't
-  // expect it in PROXY protocol line, so find it here.
-  auto chrs = std::array<char, 2>{{'\r', '\0'}};
-  auto end = std::find_first_of(rb_.pos, rb_.pos + rb_.rleft(),
-                                std::begin(chrs), std::end(chrs));
-  if (end + 2 > rb_.pos + rb_.rleft() || *end == '\0' || end[1] != '\n') {
+  // NULL character really destroys functions which expects NULL
+  // terminated string.  We won't expect it in PROXY protocol line, so
+  // find it here.
+  auto chrs = std::array<char, 2>{{'\n', '\0'}};
+
+  constexpr size_t MAX_PROXY_LINELEN = 107;
+
+  auto bufend = rb_.pos + std::min(MAX_PROXY_LINELEN, rb_.rleft());
+
+  auto end =
+      std::find_first_of(rb_.pos, bufend, std::begin(chrs), std::end(chrs));
+
+  if (end == bufend) {
+    if (rb_.rleft() >= MAX_PROXY_LINELEN) {
+      if (LOG_ENABLED(INFO)) {
+        CLOG(INFO, this) << "PROXY-protocol-v1: No ending CR LF sequence found";
+      }
+      return -1;
+    }
+    return 0;
+  }
+
+  if (*end == '\0' || end == rb_.pos || *(end - 1) != '\r') {
     if (LOG_ENABLED(INFO)) {
       CLOG(INFO, this) << "PROXY-protocol-v1: No ending CR LF sequence found";
     }
     return -1;
   }
 
+  --end;
+
   constexpr const char HEADER[] = "PROXY ";
 
   if (end - rb_.pos < str_size(HEADER)) {
@@ -916,11 +950,21 @@ int ClientHandler::proxy_protocol_read() {
       return -1;
     }
 
-    if (util::streq_l("TCP4 ", rb_.pos, 5)) {
+    if (rb_.pos[1] != 'C' || rb_.pos[2] != 'P') {
+      if (LOG_ENABLED(INFO)) {
+        CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family";
+      }
+      return -1;
+    }
+
+    switch (rb_.pos[3]) {
+    case '4':
       family = AF_INET;
-    } else if (util::streq_l("TCP6 ", rb_.pos, 5)) {
+      break;
+    case '6':
       family = AF_INET6;
-    } else {
+      break;
+    default:
       if (LOG_ENABLED(INFO)) {
         CLOG(INFO, this) << "PROXY-protocol-v1: Unknown INET protocol family";
       }

src/shrpx_client_handler.h

@@ -56,6 +56,7 @@ public:
                 const char *port);
   ~ClientHandler();
 
+  int noop();
   // Performs clear text I/O
   int read_clear();
   int write_clear();