http: unset `F_CHUNKED` on new `Transfer-Encoding`

Duplicate `Transfer-Encoding` header should be a treated as a single,
but with original header values concatenated with a comma separator. In
the light of this, even if the past `Transfer-Encoding` ended with
`chunked`, we should be not let the `F_CHUNKED` to leak into the next
header, because mere presence of another header indicates that `chunked`
is not the last transfer-encoding token.

Ref: https://github.com/nodejs-private/llhttp-private/pull/3
See: https://hackerone.com/bugs?report_id=1002188&subject=nodejs

CVE-ID: CVE-2020-8287
PR-URL: https://github.com/nodejs-private/node-private/pull/236
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

2 files changed, 80 insertions, 2 deletions

diff --git a/deps/llhttp/src/llhttp.c b/deps/llhttp/src/llhttp.c
index acc35479f8..3019c41096 100644
--- a/deps/llhttp/src/llhttp.c
+++ b/deps/llhttp/src/llhttp.c
@@ -813,6 +813,14 @@ int llhttp__internal__c_or_flags_16(
   return 0;
 }
 
+int llhttp__internal__c_and_flags(
+    llhttp__internal_t* state,
+    const unsigned char* p,
+    const unsigned char* endp) {
+  state->flags &= -9;
+  return 0;
+}
+
 int llhttp__internal__c_update_header_state_7(
     llhttp__internal_t* state,
     const unsigned char* p,
@@ -5974,10 +5982,18 @@ static llparse_state_t llhttp__internal__run(
     /* UNREACHABLE */;
     abort();
   }
+  s_n_llhttp__internal__n_invoke_and_flags: {
+    switch (llhttp__internal__c_and_flags(state, p, endp)) {
+      default:
+        goto s_n_llhttp__internal__n_header_value_te_chunked;
+    }
+    /* UNREACHABLE */;
+    abort();
+  }
   s_n_llhttp__internal__n_invoke_or_flags_16: {
     switch (llhttp__internal__c_or_flags_16(state, p, endp)) {
       default:
-        goto s_n_llhttp__internal__n_header_value_te_chunked;
+        goto s_n_llhttp__internal__n_invoke_and_flags;
     }
     /* UNREACHABLE */;
     abort();
@@ -7625,6 +7641,14 @@ int llhttp__internal__c_or_flags_16(
   return 0;
 }
 
+int llhttp__internal__c_and_flags(
+    llhttp__internal_t* state,
+    const unsigned char* p,
+    const unsigned char* endp) {
+  state->flags &= -9;
+  return 0;
+}
+
 int llhttp__internal__c_update_header_state_7(
     llhttp__internal_t* state,
     const unsigned char* p,
@@ -12522,10 +12546,18 @@ static llparse_state_t llhttp__internal__run(
     /* UNREACHABLE */;
     abort();
   }
+  s_n_llhttp__internal__n_invoke_and_flags: {
+    switch (llhttp__internal__c_and_flags(state, p, endp)) {
+      default:
+        goto s_n_llhttp__internal__n_header_value_te_chunked;
+    }
+    /* UNREACHABLE */;
+    abort();
+  }
   s_n_llhttp__internal__n_invoke_or_flags_16: {
     switch (llhttp__internal__c_or_flags_16(state, p, endp)) {
       default:
-        goto s_n_llhttp__internal__n_header_value_te_chunked;
+        goto s_n_llhttp__internal__n_invoke_and_flags;
     }
     /* UNREACHABLE */;
     abort();
diff --git a/test/parallel/test-http-transfer-encoding-smuggling.js b/test/parallel/test-http-transfer-encoding-smuggling.js
new file mode 100644
index 0000000000..9d97db4c0a
--- /dev/null
+++ b/test/parallel/test-http-transfer-encoding-smuggling.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+const msg = [
+  'POST / HTTP/1.1',
+  'Host: 127.0.0.1',
+  'Transfer-Encoding: chunked',
+  'Transfer-Encoding: chunked-false',
+  'Connection: upgrade',
+  '',
+  '1',
+  'A',
+  '0',
+  '',
+  'GET /flag HTTP/1.1',
+  'Host: 127.0.0.1',
+  '',
+  '',
+].join('\r\n');
+
+// Verify that the server is called only once even with a smuggled request.
+
+const server = http.createServer(common.mustCall((req, res) => {
+  res.end();
+}, 1));
+
+function send(next) {
+  const client = net.connect(server.address().port, 'localhost');
+  client.setEncoding('utf8');
+  client.on('error', common.mustNotCall());
+  client.on('end', next);
+  client.write(msg);
+  client.resume();
+}
+
+server.listen(0, common.mustCall((err) => {
+  assert.ifError(err);
+  send(common.mustCall(() => {
+    server.close();
+  }));
+}));