commit 2a0d1065d4f68cf622db2ffd22dc8022c0239fa4
author Fedor Indutny <fedor@indutny.com> Fri Apr 05 16:49:14 2019 -0400
committer Fedor Indutny <fedor@indutny.com> Mon Apr 08 20:37:55 2019 -0400
tree 689262ef13ac4947e90ce6063c3f1f63e90da1f5
parent 0d0a24e19eb5ba232d2ea8859aba2a7cc6c42bc4

http_parser: revert `memchr()` optimization

An optimization was introduced in c6097e1d and 0097de58. The crux of
optimization was to skip all characters in header value until either
of CR or LF. Unfortunately, this optimization comes at cost of
inconsistency in header value validation, which might lead to security
issue due to violated expectations in the user code.

Partially revert the optimization, and add additional check to make
general header value parsing consistent.

Fix: #468
PR-URL: https://github.com/nodejs/http-parser/pull/469
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Harvey Tuch <htuch@google.com>

http_parser.c

diff --git a/http_parser.c b/http_parser.c
index e2fc5d2..2ea228e 100644
--- a/http_parser.c
+++ b/http_parser.c
@@ -1496,28 +1496,24 @@
 
           switch (h_state) {
             case h_general:
-            {
-              const char* p_cr;
-              const char* p_lf;
-              size_t limit = data + len - p;
+              {
+                const char* limit = p + MIN(data + len - p, max_header_size);
 
-              limit = MIN(limit, max_header_size);
-
-              p_cr = (const char*) memchr(p, CR, limit);
-              p_lf = (const char*) memchr(p, LF, limit);
-              if (p_cr != NULL) {
-                if (p_lf != NULL && p_cr >= p_lf)
-                  p = p_lf;
-                else
-                  p = p_cr;
-              } else if (UNLIKELY(p_lf != NULL)) {
-                p = p_lf;
-              } else {
-                p = data + len;
+                for (; p != limit; p++) {
+                  ch = *p;
+                  if (ch == CR || ch == LF) {
+                    --p;
+                    break;
+                  }
+                  if (!lenient && !IS_HEADER_CHAR(ch)) {
+                    SET_ERRNO(HPE_INVALID_HEADER_TOKEN);
+                    goto error;
+                  }
+                }
+                if (p == data + len)
+                  --p;
+                break;
               }
-              --p;
-              break;
-            }
 
             case h_connection:
             case h_transfer_encoding:

test.c

diff --git a/test.c b/test.c
index c3fddd5..0140a18 100644
--- a/test.c
+++ b/test.c
@@ -4316,6 +4316,9 @@
   test_simple("GET / HTTP/11.1\r\n\r\n", HPE_INVALID_VERSION);
   test_simple("GET / HTTP/1.01\r\n\r\n", HPE_INVALID_VERSION);
 
+  test_simple("GET / HTTP/1.0\r\nHello: w\1rld\r\n\r\n", HPE_INVALID_HEADER_TOKEN);
+  test_simple("GET / HTTP/1.0\r\nHello: woooo\2rld\r\n\r\n", HPE_INVALID_HEADER_TOKEN);
+
   // Extended characters - see nodejs/test/parallel/test-http-headers-obstext.js
   test_simple("GET / HTTP/1.1\r\n"
               "Test: Düsseldorf\r\n",