http-parser does not correct sanitize NUL characters in header values

[htuch]

As described in CVE-2019-9900 and envoyproxy/envoy#6434, http-parser will not reject headers containing NUL in header values (beyond the first character) as required by RFC 7230.

This leads to potentially serious security vulnerabilities for projects, e.g. Envoy that depend on http-parser, as they are likely to expect RFC value sanitization of any headers that are passed. See envoyproxy/envoy#6434 for an example of this and our announcement at https://groups.google.com/forum/#!topic/envoy-announce/VoHfnDqZiAM.

The Node.JS security WG were contacted about this issue on initial discovery.

[indutny]

Really sad to hear that!

Would you care to share your test case with us?

I've tried reproducing it from the description, but it appears that ASCII code 0 is not accepted as a part of header value:

diff --git a/test.c b/test.c
index c3fddd5..af7f7b7 100644
--- a/test.c
+++ b/test.c
@@ -3555,6 +3555,35 @@ test_simple (const char *buf, enum http_errno err_expected)
   test_simple_type(buf, err_expected, HTTP_REQUEST);
 }
 
+
+void
+test_raw_type (const char *buf,
+               size_t len,
+               enum http_errno err_expected,
+               enum http_parser_type type)
+{
+  parser_init(type);
+
+  enum http_errno err;
+
+  parse(buf, len);
+  err = HTTP_PARSER_ERRNO(&parser);
+  parse(NULL, 0);
+
+  /* In strict mode, allow us to pass with an unexpected HPE_STRICT as
+   * long as the caller isn't expecting success.
+   */
+#if HTTP_PARSER_STRICT
+  if (err_expected != err && err_expected != HPE_OK && err != HPE_STRICT) {
+#else
+  if (err_expected != err) {
+#endif
+    fprintf(stderr, "\n*** test_simple expected %s, but saw %s ***\n\n%s\n",
+        http_errno_name(err_expected), http_errno_name(err), buf);
+    abort();
+  }
+}
+
 void
 test_invalid_header_content (int req, const char* str)
 {
@@ -4316,6 +4345,9 @@ main (void)
   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_raw_type("GET / HTTP/1.0\r\nHello: w\0rld\r\n\r\n",
+      sizeof("GET / HTTP/1.0\r\nHello: w\0rld\r\n\r\n") - 1, HPE_INVALID_HEADER_TOKEN, HTTP_REQUEST);
+
   // Extended characters - see nodejs/test/parallel/test-http-headers-obstext.js
   test_simple("GET / HTTP/1.1\r\n"
               "Test: Düsseldorf\r\n",

[indutny]

Nevermind, this diff reproduces it:

diff --git a/test.c b/test.c
index c3fddd5..adfd1e6 100644
--- a/test.c
+++ b/test.c
@@ -3555,6 +3555,35 @@ test_simple (const char *buf, enum http_errno err_expected)
   test_simple_type(buf, err_expected, HTTP_REQUEST);
 }
 
+
+void
+test_raw_type (const char *buf,
+               size_t len,
+               enum http_errno err_expected,
+               enum http_parser_type type)
+{
+  parser_init(type);
+
+  enum http_errno err;
+
+  parse(buf, len);
+  err = HTTP_PARSER_ERRNO(&parser);
+  parse(NULL, 0);
+
+  /* In strict mode, allow us to pass with an unexpected HPE_STRICT as
+   * long as the caller isn't expecting success.
+   */
+#if HTTP_PARSER_STRICT
+  if (err_expected != err && err_expected != HPE_OK && err != HPE_STRICT) {
+#else
+  if (err_expected != err) {
+#endif
+    fprintf(stderr, "\n*** test_simple expected %s, but saw %s ***\n\n%s\n",
+        http_errno_name(err_expected), http_errno_name(err), buf);
+    abort();
+  }
+}
+
 void
 test_invalid_header_content (int req, const char* str)
 {
@@ -4316,6 +4345,9 @@ main (void)
   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_raw_type("GET / HTTP/1.0\r\nHello: waa\0rld\r\n\r\n",
+      sizeof("GET / HTTP/1.0\r\nHello: waa\0rld\r\n\r\n") - 1, HPE_INVALID_HEADER_TOKEN, HTTP_REQUEST);
+
   // Extended characters - see nodejs/test/parallel/test-http-headers-obstext.js
   test_simple("GET / HTTP/1.1\r\n"
               "Test: Düsseldorf\r\n",

Sorry, I should have read more information from that envoyproxy issue before posting previous message!

[indutny]

Should be fixed by #469. Thank you for report.

[htuch]

@indutny thanks for the fix. I left a few minor comments, please don't block on me here though.