nghttpx: HttpUpstream: Check required request headers strictly

If multiple required headers (e.g., :path) found, return HTTP 400
error.
Fix util::strieq(a,b,n) where boundary of b is not checked in the
loop.

src/shrpx_http2_upstream.cc

@@ -192,33 +192,104 @@ void on_frame_recv_callback
     downstream->init_response_body_buf();
 
     auto nva = frame->headers.nva;
-    std::string path, scheme, host, method;
-    for(size_t i = 0; i < frame->headers.nvlen; ++i) {
-      if(util::strieq(":path", nva[i].name, nva[i].namelen)) {
-        path.assign(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
-      } else if(util::strieq(":scheme", nva[i].name, nva[i].namelen)) {
-        scheme.assign(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
-      } else if(util::strieq(":method", nva[i].name, nva[i].namelen)) {
-        method.assign(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
-        downstream->set_request_method(method);
-      } else if(util::strieq(":host", nva[i].name, nva[i].namelen)) {
-        host.assign(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
-      } else if(nva[i].namelen > 0 && nva[i].name[0] != ':') {
-        downstream->add_request_header
-          (std::string(reinterpret_cast<char*>(nva[i].name), nva[i].namelen),
-           std::string(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen));
+
+    if(LOG_ENABLED(INFO)) {
+      std::stringstream ss;
+      for(size_t i = 0; i < frame->headers.nvlen; ++i) {
+        ss << TTY_HTTP_HD;
+        ss.write(reinterpret_cast<char*>(nva[i].name), nva[i].namelen);
+        ss << TTY_RST << ": ";
+        ss.write(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
+        ss << "\n";
+      }
+      ULOG(INFO, upstream) << "HTTP request headers. stream_id="
+                           << downstream->get_stream_id()
+                           << "\n" << ss.str();
+    }
+
+    // Assuming that nva is sorted by name.
+    const char *req_headers[] = {":host", ":method", ":path", ":scheme" };
+    const size_t req_hdlen = sizeof(req_headers)/sizeof(req_headers[0]);
+    int req_hdidx[req_hdlen];
+    memset(req_hdidx, -1, sizeof(req_hdidx));
+    bool bad_req = false;
+    {
+      size_t i, j;
+      for(i = 0, j = 0; i < frame->headers.nvlen && j < req_hdlen;) {
+        int rv = util::strcompare(req_headers[j], nva[i].name, nva[i].namelen);
+        if(rv > 0) {
+          if(nva[i].namelen > 0 && nva[i].name[0] != ':') {
+            downstream->add_request_header
+              (std::string(reinterpret_cast<char*>(nva[i].name),
+                           nva[i].namelen),
+               std::string(reinterpret_cast<char*>(nva[i].value),
+                           nva[i].valuelen));
+          }
+          ++i;
+        } else if(rv < 0) {
+          ++j;
+        } else {
+          if(req_hdidx[j] != -1) {
+            if(LOG_ENABLED(INFO)) {
+              ULOG(INFO, upstream) << "multiple " << req_headers[j]
+                                   << " found in the request";
+            }
+            bad_req = true;
+            break;
+          }
+          req_hdidx[j] = i;
+          ++i;
+        }
+      }
+      if(!bad_req) {
+        // Here :scheme is optional, because with CONNECT method, it
+        // is omitted.
+        for(j = 0; j < 3; ++j) {
+          if(req_hdidx[j] == -1) {
+            bad_req = true;
+            break;
+          }
+        }
+      }
+      if(!bad_req) {
+        for(; i < frame->headers.nvlen; ++i) {
+          if(nva[i].namelen > 0 && nva[i].name[0] != ':') {
+            downstream->add_request_header
+              (std::string(reinterpret_cast<char*>(nva[i].name),
+                           nva[i].namelen),
+               std::string(reinterpret_cast<char*>(nva[i].value),
+                           nva[i].valuelen));
+          }
+        }
       }
     }
-    if(path.empty() || host.empty() || method.empty()) {
-      upstream->rst_stream(downstream, NGHTTP2_PROTOCOL_ERROR);
+    if(bad_req) {
+      downstream->set_request_state(Downstream::HEADER_COMPLETE);
+      if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
+        downstream->set_request_state(Downstream::MSG_COMPLETE);
+      }
+      if(upstream->error_reply(downstream, 400) != 0) {
+        upstream->rst_stream(downstream, NGHTTP2_PROTOCOL_ERROR);
+      }
       return;
     }
+
+    std::string host(reinterpret_cast<char*>(nva[req_hdidx[0]].value),
+                     nva[req_hdidx[0]].valuelen);
+    std::string method(reinterpret_cast<char*>(nva[req_hdidx[1]].value),
+                       nva[req_hdidx[1]].valuelen);
+    std::string path(reinterpret_cast<char*>(nva[req_hdidx[2]].value),
+                     nva[req_hdidx[2]].valuelen);
+
+    downstream->set_request_method(method);
+
     // SpdyDownstreamConnection examines request path to find
     // scheme. We construct abs URI for spdy_bridge mode as well as
     // spdy_proxy mode.
     if((get_config()->spdy_proxy || get_config()->spdy_bridge) &&
-       !scheme.empty() && path[0] == '/') {
-      std::string reqpath = scheme;
+       req_hdidx[3] != -1 && path[0] == '/') {
+      std::string reqpath(reinterpret_cast<char*>(nva[req_hdidx[3]].value),
+                          nva[req_hdidx[3]].valuelen);
       reqpath += "://";
       reqpath += host;
       reqpath += path;
@@ -230,20 +301,6 @@ void on_frame_recv_callback
     downstream->add_request_header("host", host);
     downstream->check_upgrade_request();
 
-    if(LOG_ENABLED(INFO)) {
-      std::stringstream ss;
-      for(size_t i = 0; i < frame->headers.nvlen; ++i) {
-        ss << TTY_HTTP_HD;
-        ss.write(reinterpret_cast<char*>(nva[i].name), nva[i].namelen);
-        ss << TTY_RST << ": ";
-        ss.write(reinterpret_cast<char*>(nva[i].value), nva[i].valuelen);
-        ss << "\n";
-      }
-      ULOG(INFO, upstream) << "HTTP request headers. stream_id="
-                           << downstream->get_stream_id()
-                           << "\n" << ss.str();
-    }
-
     auto dconn = upstream->get_client_handler()->get_downstream_connection();
     int rv = dconn->attach_downstream(downstream);
     if(rv != 0) {

src/util.cc

@@ -26,6 +26,7 @@
 
 #include <time.h>
 
+#include <cassert>
 #include <cstdio>
 #include <cstring>
 
@@ -166,10 +167,30 @@ bool strieq(const char *a, const uint8_t *b, size_t bn)
     return false;
   }
   const uint8_t *blast = b + bn;
-  for(; *a && lowcase(*a) == lowcase(*b); ++a, ++b);
+  for(; *a && b != blast && lowcase(*a) == lowcase(*b); ++a, ++b);
   return !*a && b == blast;
 }
 
+int strcompare(const char *a, const uint8_t *b, size_t bn)
+{
+  assert(a && b);
+  const uint8_t *blast = b + bn;
+  for(; *a && b != blast; ++a, ++b) {
+    if(*a < *b) {
+      return -1;
+    } else if(*a > *b) {
+      return 1;
+    }
+  }
+  if(!*a && b == blast) {
+    return 0;
+  } else if(b == blast) {
+    return 1;
+  } else {
+    return -1;
+  }
+}
+
 bool strifind(const char *a, const char *b)
 {
   if(!a || !b) {

src/util.h

@@ -291,6 +291,8 @@ bool iendsWith
 
 bool endsWith(const std::string& a, const std::string& b);
 
+int strcompare(const char *a, const uint8_t *b, size_t n);
+
 bool strieq(const std::string& a, const std::string& b);
 
 bool strieq(const char *a, const char *b);