deps: update http-parser to version 2.5.1

includes parsing improvements to ensure closer HTTP spec conformance

PR-URL: https://github.com/nodejs/node-private/pull/20

deps/http_parser/Makefile

@@ -19,7 +19,7 @@
 # IN THE SOFTWARE.
 
 PLATFORM ?= $(shell sh -c 'uname -s | tr "[A-Z]" "[a-z]"')
-SONAME ?= libhttp_parser.so.2.5.0
+SONAME ?= libhttp_parser.so.2.5.1
 
 CC?=gcc
 AR?=ar

deps/http_parser/http_parser.c

@@ -433,6 +433,12 @@ enum http_host_state
   (IS_ALPHANUM(c) || (c) == '.' || (c) == '-' || (c) == '_')
 #endif
 
+/**
+ * Verify that a char is a valid visible (printable) US-ASCII
+ * character or %x80-FF
+ **/
+#define IS_HEADER_CHAR(ch)                                                     \
+  (ch == CR || ch == LF || ch == 9 || (ch > 31 && ch != 127))
 
 #define start_state (parser->type == HTTP_REQUEST ? s_start_req : s_start_res)
 
@@ -638,6 +644,8 @@ size_t http_parser_execute (http_parser *parser,
   const char *status_mark = 0;
   enum state p_state = (enum state) parser->state;
 
+  const unsigned int lenient = parser->lenient_http_headers;
+
   /* We're in an error state. Don't bother doing anything. */
   if (HTTP_PARSER_ERRNO(parser) != HPE_OK) {
     return 0;
@@ -1384,7 +1392,12 @@ reexecute:
                   || c != CONTENT_LENGTH[parser->index]) {
                 parser->header_state = h_general;
               } else if (parser->index == sizeof(CONTENT_LENGTH)-2) {
+                if (parser->flags & F_CONTENTLENGTH) {
+                  SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
+                  goto error;
+                }
                 parser->header_state = h_content_length;
+                parser->flags |= F_CONTENTLENGTH;
               }
               break;
 
@@ -1536,6 +1549,11 @@ reexecute:
             REEXECUTE();
           }
 
+          if (!lenient && !IS_HEADER_CHAR(ch)) {
+            SET_ERRNO(HPE_INVALID_HEADER_TOKEN);
+            goto error;
+          }
+
           c = LOWER(ch);
 
           switch (h_state) {
@@ -1703,7 +1721,10 @@ reexecute:
 
       case s_header_almost_done:
       {
-        STRICT_CHECK(ch != LF);
+        if (UNLIKELY(ch != LF)) {
+          SET_ERRNO(HPE_LF_EXPECTED);
+          goto error;
+        }
 
         UPDATE_STATE(s_header_value_lws);
         break;
@@ -1787,6 +1808,14 @@ reexecute:
           REEXECUTE();
         }
 
+        /* Cannot use chunked encoding and a content-length header together
+           per the HTTP specification. */
+        if ((parser->flags & F_CHUNKED) &&
+            (parser->flags & F_CONTENTLENGTH)) {
+          SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
+          goto error;
+        }
+
         UPDATE_STATE(s_headers_done);
 
         /* Set this here so that on_headers_complete() callbacks can see it */

deps/http_parser/http_parser.h

@@ -27,7 +27,7 @@ extern "C" {
 /* Also update SONAME in the Makefile whenever you change these. */
 #define HTTP_PARSER_VERSION_MAJOR 2
 #define HTTP_PARSER_VERSION_MINOR 5
-#define HTTP_PARSER_VERSION_PATCH 0
+#define HTTP_PARSER_VERSION_PATCH 1
 
 #include <sys/types.h>
 #if defined(_WIN32) && !defined(__MINGW32__) && (!defined(_MSC_VER) || _MSC_VER<1600)
@@ -140,6 +140,7 @@ enum flags
   , F_TRAILING              = 1 << 4
   , F_UPGRADE               = 1 << 5
   , F_SKIPBODY              = 1 << 6
+  , F_CONTENTLENGTH         = 1 << 7
   };
 
 
@@ -182,6 +183,8 @@ enum flags
   XX(INVALID_HEADER_TOKEN, "invalid character in header")            \
   XX(INVALID_CONTENT_LENGTH,                                         \
      "invalid character in content-length header")                   \
+  XX(UNEXPECTED_CONTENT_LENGTH,                                      \
+     "unexpected content-length header")                             \
   XX(INVALID_CHUNK_SIZE,                                             \
      "invalid character in chunk size header")                       \
   XX(INVALID_CONSTANT, "invalid constant string")                    \
@@ -206,10 +209,11 @@ enum http_errno {
 struct http_parser {
   /** PRIVATE **/
   unsigned int type : 2;         /* enum http_parser_type */
-  unsigned int flags : 7;        /* F_* values from 'flags' enum; semi-public */
+  unsigned int flags : 8;        /* F_* values from 'flags' enum; semi-public */
   unsigned int state : 7;        /* enum state from http_parser.c */
-  unsigned int header_state : 8; /* enum header_state from http_parser.c */
-  unsigned int index : 8;        /* index into current matcher */
+  unsigned int header_state : 7; /* enum header_state from http_parser.c */
+  unsigned int index : 7;        /* index into current matcher */
+  unsigned int lenient_http_headers : 1;
 
   uint32_t nread;          /* # bytes read in various scenarios */
   uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */

deps/http_parser/test.c

@@ -3165,6 +3165,155 @@ test_simple (const char *buf, enum http_errno err_expected)
   }
 }
 
+void
+test_invalid_header_content (int req, const char* str)
+{
+  http_parser parser;
+  http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE);
+  size_t parsed;
+  const char *buf;
+  buf = req ?
+    "GET / HTTP/1.1\r\n" :
+    "HTTP/1.1 200 OK\r\n";
+  parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
+  assert(parsed == strlen(buf));
+
+  buf = str;
+  size_t buflen = strlen(buf);
+
+  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
+  if (parsed != buflen) {
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN);
+    return;
+  }
+
+  fprintf(stderr,
+          "\n*** Error expected but none in invalid header content test ***\n");
+  abort();
+}
+
+void
+test_invalid_header_field_content_error (int req)
+{
+  test_invalid_header_content(req, "Foo: F\01ailure");
+  test_invalid_header_content(req, "Foo: B\02ar");
+}
+
+void
+test_invalid_header_field (int req, const char* str)
+{
+  http_parser parser;
+  http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE);
+  size_t parsed;
+  const char *buf;
+  buf = req ?
+    "GET / HTTP/1.1\r\n" :
+    "HTTP/1.1 200 OK\r\n";
+  parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
+  assert(parsed == strlen(buf));
+
+  buf = str;
+  size_t buflen = strlen(buf);
+
+  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
+  if (parsed != buflen) {
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN);
+    return;
+  }
+
+  fprintf(stderr,
+          "\n*** Error expected but none in invalid header token test ***\n");
+  abort();
+}
+
+void
+test_invalid_header_field_token_error (int req)
+{
+  test_invalid_header_field(req, "Fo@: Failure");
+  test_invalid_header_field(req, "Foo\01\test: Bar");
+}
+
+void
+test_double_content_length_error (int req)
+{
+  http_parser parser;
+  http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE);
+  size_t parsed;
+  const char *buf;
+  buf = req ?
+    "GET / HTTP/1.1\r\n" :
+    "HTTP/1.1 200 OK\r\n";
+  parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
+  assert(parsed == strlen(buf));
+
+  buf = "Content-Length: 0\r\nContent-Length: 1\r\n\r\n";
+  size_t buflen = strlen(buf);
+
+  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
+  if (parsed != buflen) {
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_MULTIPLE_CONTENT_LENGTH);
+    return;
+  }
+
+  fprintf(stderr,
+          "\n*** Error expected but none in double content-length test ***\n");
+  abort();
+}
+
+void
+test_chunked_content_length_error (int req)
+{
+  http_parser parser;
+  http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE);
+  size_t parsed;
+  const char *buf;
+  buf = req ?
+    "GET / HTTP/1.1\r\n" :
+    "HTTP/1.1 200 OK\r\n";
+  parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
+  assert(parsed == strlen(buf));
+
+  buf = "Transfer-Encoding: chunked\r\nContent-Length: 1\r\n\r\n";
+  size_t buflen = strlen(buf);
+
+  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
+  if (parsed != buflen) {
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_CHUNKED_WITH_CONTENT_LENGTH);
+    return;
+  }
+
+  fprintf(stderr,
+          "\n*** Error expected but none in chunked content-length test ***\n");
+  abort();
+}
+
+void
+test_header_cr_no_lf_error (int req)
+{
+  http_parser parser;
+  http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE);
+  size_t parsed;
+  const char *buf;
+  buf = req ?
+    "GET / HTTP/1.1\r\n" :
+    "HTTP/1.1 200 OK\r\n";
+  parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
+  assert(parsed == strlen(buf));
+
+  buf = "Foo: 1\rBar: 1\r\n\r\n";
+  size_t buflen = strlen(buf);
+
+  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
+  if (parsed != buflen) {
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_LF_EXPECTED);
+    return;
+  }
+
+  fprintf(stderr,
+          "\n*** Error expected but none in header whitespace test ***\n");
+  abort();
+}
+
 void
 test_header_overflow_error (int req)
 {
@@ -3591,6 +3740,18 @@ main (void)
   test_header_content_length_overflow_error();
   test_chunk_content_length_overflow_error();
 
+  //// HEADER FIELD CONDITIONS
+  test_double_content_length_error(HTTP_REQUEST);
+  test_chunked_content_length_error(HTTP_REQUEST);
+  test_header_cr_no_lf_error(HTTP_REQUEST);
+  test_invalid_header_field_token_error(HTTP_REQUEST);
+  test_invalid_header_field_content_error(HTTP_REQUEST);
+  test_double_content_length_error(HTTP_RESPONSE);
+  test_chunked_content_length_error(HTTP_RESPONSE);
+  test_header_cr_no_lf_error(HTTP_RESPONSE);
+  test_invalid_header_field_token_error(HTTP_RESPONSE);
+  test_invalid_header_field_content_error(HTTP_RESPONSE);
+
   //// RESPONSES
 
   for (i = 0; i < response_count; i++) {

src/node_http_parser.cc

@@ -1,6 +1,7 @@
 #include "node.h"
 #include "node_buffer.h"
 #include "node_http_parser.h"
+#include "node_revert.h"
 
 #include "base-object.h"
 #include "base-object-inl.h"
@@ -670,6 +671,8 @@ class Parser : public BaseObject {
 
   void Init(enum http_parser_type type) {
     http_parser_init(&parser_, type);
+    /* Allow the strict http header parsing to be reverted */
+    parser_.lenient_http_headers = IsReverted(REVERT_CVE_2016_2216) ? 1 : 0;
     url_.Reset();
     status_message_.Reset();
     num_fields_ = 0;

src/node_revert.h

@@ -12,8 +12,8 @@
  * For *master* this list should always be empty!
  *
  **/
-#define REVERSIONS(XX)
-//  XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
+#define REVERSIONS(XX)                                                        \
+  XX(CVE_2016_2216, "CVE-2016-2216", "Strict HTTP Header Parsing")
 
 namespace node {
 

test/parallel/test-http-client-reject-chunked-with-content-length.js

@@ -0,0 +1,28 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const net = require('net');
+const assert = require('assert');
+
+const reqstr = 'HTTP/1.1 200 OK\r\n' +
+               'Content-Length: 1\r\n' +
+               'Transfer-Encoding: chunked\r\n\r\n';
+
+const server = net.createServer((socket) => {
+  socket.write(reqstr);
+});
+
+server.listen(common.PORT, () => {
+  // The callback should not be called because the server is sending
+  // both a Content-Length header and a Transfer-Encoding: chunked
+  // header, which is a violation of the HTTP spec.
+  const req = http.get({port:common.PORT}, (res) => {
+    assert.fail(null, null, 'callback should not be called');
+  });
+  req.on('error', common.mustCall((err) => {
+    assert(/^Parse Error/.test(err.message));
+    assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH');
+    server.close();
+  }));
+});

test/parallel/test-http-client-reject-cr-no-lf.js

@@ -0,0 +1,27 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const net = require('net');
+const assert = require('assert');
+
+const reqstr = 'HTTP/1.1 200 OK\r\n' +
+               'Foo: Bar\r' +
+               'Content-Length: 1\r\n\r\n';
+
+const server = net.createServer((socket) => {
+  socket.write(reqstr);
+});
+
+server.listen(common.PORT, () => {
+  // The callback should not be called because the server is sending a
+  // header field that ends only in \r with no following \n
+  const req = http.get({port:common.PORT}, (res) => {
+    assert.fail(null, null, 'callback should not be called');
+  });
+  req.on('error', common.mustCall((err) => {
+    assert(/^Parse Error/.test(err.message));
+    assert.equal(err.code, 'HPE_LF_EXPECTED');
+    server.close();
+  }));
+});

test/parallel/test-http-double-content-length.js

@@ -0,0 +1,33 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const assert = require('assert');
+
+// The callback should never be invoked because the server
+// should respond with a 400 Client Error when a double
+// Content-Length header is received.
+const server = http.createServer((req, res) => {
+  assert(false, 'callback should not have been invoked');
+  res.end();
+});
+server.on('clientError', common.mustCall((err, socket) => {
+  assert(/^Parse Error/.test(err.message));
+  assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH');
+  socket.destroy();
+}));
+
+server.listen(common.PORT, () => {
+  const req = http.get({
+    port: common.PORT,
+    // Send two content-length header values.
+    headers: {'Content-Length': [1, 2]}},
+    (res) => {
+      assert.fail(null, null, 'an error should have occurred');
+      server.close();
+    }
+  );
+  req.on('error', common.mustCall(() => {
+    server.close();
+  }));
+});

test/parallel/test-http-header-response-splitting.js

@@ -1,3 +1,4 @@
+// Flags: --security-revert=CVE-2016-2216
 'use strict';
 var common = require('../common'),
     assert = require('assert'),

test/parallel/test-http-response-multi-content-length.js

@@ -0,0 +1,52 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const assert = require('assert');
+
+const MAX_COUNT = 2;
+
+const server = http.createServer((req, res) => {
+  const num = req.headers['x-num'];
+  // TODO(@jasnell) At some point this should be refactored as the API
+  // should not be allowing users to set multiple content-length values
+  // in the first place.
+  switch (num) {
+    case '1':
+      res.setHeader('content-length', [2, 1]);
+      break;
+    case '2':
+      res.writeHead(200, {'content-length': [1, 2]});
+      break;
+    default:
+      assert.fail(null, null, 'should never get here');
+  }
+  res.end('ok');
+});
+
+var count = 0;
+
+server.listen(common.PORT, common.mustCall(() => {
+  for (let n = 1; n <= MAX_COUNT ; n++) {
+    // This runs twice, the first time, the server will use
+    // setHeader, the second time it uses writeHead. In either
+    // case, the error handler must be called because the client
+    // is not allowed to accept multiple content-length headers.
+    http.get(
+      {port:common.PORT, headers:{'x-num': n}},
+      (res) => {
+        assert(false, 'client allowed multiple content-length headers.');
+      }
+    ).on('error', common.mustCall((err) => {
+      assert(/^Parse Error/.test(err.message));
+      assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH');
+      count++;
+      if (count === MAX_COUNT)
+        server.close();
+    }));
+  }
+}));
+
+process.on('exit', () => {
+  assert.equal(count, MAX_COUNT);
+});

test/parallel/test-http-server-multiheaders2.js

@@ -57,7 +57,6 @@ var srv = http.createServer(function(req, res) {
     assert.equal(req.headers[header.toLowerCase()],
                  'foo, bar', 'header parsed incorrectly: ' + header);
   });
-  assert.equal(req.headers['content-length'], 0);
 
   res.writeHead(200, {'Content-Type' : 'text/plain'});
   res.end('EOF');
@@ -75,10 +74,7 @@ var headers = []
   .concat(multipleAllowed.map(makeHeader('foo')))
   .concat(multipleForbidden.map(makeHeader('foo')))
   .concat(multipleAllowed.map(makeHeader('bar')))
-  .concat(multipleForbidden.map(makeHeader('bar')))
-  // content-length is a special case since node.js
-  // is dropping connetions with non-numeric headers
-  .concat([['content-length', 0], ['content-length', 123]]);
+  .concat(multipleForbidden.map(makeHeader('bar')));
 
 srv.listen(common.PORT, function() {
   http.get({

test/parallel/test-http-server-reject-chunked-with-content-length.js

@@ -0,0 +1,31 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const net = require('net');
+const assert = require('assert');
+
+const reqstr = 'POST / HTTP/1.1\r\n' +
+               'Content-Length: 1\r\n' +
+               'Transfer-Encoding: chunked\r\n\r\n';
+
+const server = http.createServer((req, res) => {
+  assert.fail(null, null, 'callback should not be invoked');
+});
+server.on('clientError', common.mustCall((err) => {
+  assert(/^Parse Error/.test(err.message));
+  assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH');
+  server.close();
+}));
+server.listen(common.PORT, () => {
+  const client = net.connect({port: common.PORT}, () => {
+    client.write(reqstr);
+    client.end();
+  });
+  client.on('data', (data) => {
+    // Should not get to this point because the server should simply
+    // close the connection without returning any data.
+    assert.fail(null, null, 'no data should be returned by the server');
+  });
+  client.on('end', common.mustCall(() => {}));
+});

test/parallel/test-http-server-reject-cr-no-lf.js

@@ -0,0 +1,33 @@
+'use strict';
+
+const common = require('../common');
+const net = require('net');
+const http = require('http');
+const assert = require('assert');
+
+const str = 'GET / HTTP/1.1\r\n' +
+            'Dummy: Header\r' +
+            'Content-Length: 1\r\n' +
+            '\r\n';
+
+
+const server = http.createServer((req, res) => {
+  assert.fail(null, null, 'this should not be called');
+});
+server.on('clientError', common.mustCall((err) => {
+  assert(/^Parse Error/.test(err.message));
+  assert.equal(err.code, 'HPE_LF_EXPECTED');
+  server.close();
+}));
+server.listen(common.PORT, () => {
+  const client = net.connect({port:common.PORT}, () => {
+    client.on('data', (chunk) => {
+      assert.fail(null, null, 'this should not be called');
+    });
+    client.on('end', common.mustCall(() => {
+      server.close();
+    }));
+    client.write(str);
+    client.end();
+  });
+});