header field in a http request. For example, two Transfer-Encoding header fields. In this case Node.js identifies the first header field and ignores the second. This can lead to HTTP Request Smuggling (https://cwe.mitre.org/data/definitions/444.html). (bsc#1180554, CVE-2020-8287) OBS-URL: https://build.opensuse.org/package/show/devel:languages:nodejs/nodejs8?expand=0&rev=171
163 lines
5.0 KiB
Diff
163 lines
5.0 KiB
Diff
commit fc70ce08f5818a286fb5899a1bc3aff5965a745e
|
|
Author: Fedor Indutny <fedor@indutny.com>
|
|
Date: Wed Nov 18 20:50:21 2020 -0800
|
|
|
|
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.
|
|
|
|
CVE-ID: CVE-2020-8287
|
|
PR-URL: https://github.com/nodejs-private/node-private/pull/235
|
|
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
|
|
|
|
commit aa6b97fb99d7528649fadb4c6a894e078fe4323c
|
|
Author: Richard Lau <rlau@redhat.com>
|
|
Date: Thu Nov 26 15:50:56 2020 +0000
|
|
|
|
http: add test for http transfer encoding smuggling
|
|
|
|
Refs: https://github.com/nodejs-private/node-private/pull/228
|
|
Refs: https://hackerone.com/bugs?report_id=1002188&subject=nodejs
|
|
|
|
PR-URL: https://github.com/nodejs-private/node-private/pull/235
|
|
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
|
|
|
|
|
|
Index: node-v8.17.0/test/parallel/test-http-transfer-encoding-smuggling.js
|
|
===================================================================
|
|
--- /dev/null
|
|
+++ node-v8.17.0/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();
|
|
+ }));
|
|
+}));
|
|
Index: node-v8.17.0/deps/http_parser/http_parser.c
|
|
===================================================================
|
|
--- node-v8.17.0.orig/deps/http_parser/http_parser.c
|
|
+++ node-v8.17.0/deps/http_parser/http_parser.c
|
|
@@ -1339,6 +1339,13 @@ reexecute:
|
|
} else if (parser->index == sizeof(TRANSFER_ENCODING)-2) {
|
|
parser->header_state = h_transfer_encoding;
|
|
parser->flags |= F_TRANSFER_ENCODING;
|
|
+
|
|
+ /* Multiple `Transfer-Encoding` headers should be treated as
|
|
+ * one, but with values separate by a comma.
|
|
+ *
|
|
+ * See: https://tools.ietf.org/html/rfc7230#section-3.2.2
|
|
+ */
|
|
+ parser->flags &= ~F_CHUNKED;
|
|
}
|
|
break;
|
|
|
|
Index: node-v8.17.0/deps/http_parser/test.c
|
|
===================================================================
|
|
--- node-v8.17.0.orig/deps/http_parser/test.c
|
|
+++ node-v8.17.0/deps/http_parser/test.c
|
|
@@ -2045,6 +2045,58 @@ const struct message responses[] =
|
|
,.body= "2\r\nOK\r\n0\r\n\r\n"
|
|
,.num_chunks_complete= 0
|
|
}
|
|
+#define HTTP_200_DUPLICATE_TE_NOT_LAST_CHUNKED 30
|
|
+, {.name= "HTTP 200 response with `chunked` and duplicate Transfer-Encoding"
|
|
+ ,.type= HTTP_RESPONSE
|
|
+ ,.raw= "HTTP/1.1 200 OK\r\n"
|
|
+ "Transfer-Encoding: chunked\r\n"
|
|
+ "Transfer-Encoding: identity\r\n"
|
|
+ "\r\n"
|
|
+ "2\r\n"
|
|
+ "OK\r\n"
|
|
+ "0\r\n"
|
|
+ "\r\n"
|
|
+ ,.should_keep_alive= FALSE
|
|
+ ,.message_complete_on_eof= TRUE
|
|
+ ,.http_major= 1
|
|
+ ,.http_minor= 1
|
|
+ ,.status_code= 200
|
|
+ ,.response_status= "OK"
|
|
+ ,.content_length= -1
|
|
+ ,.num_headers= 2
|
|
+ ,.headers=
|
|
+ { { "Transfer-Encoding", "chunked" }
|
|
+ , { "Transfer-Encoding", "identity" }
|
|
+ }
|
|
+ ,.body= "2\r\nOK\r\n0\r\n\r\n"
|
|
+ ,.num_chunks_complete= 0
|
|
+ }
|
|
+#define HTTP_200_DUPLICATE_TE_NOT_LAST_CHUNKED 30
|
|
+, {.name= "HTTP 200 response with `chunked` and duplicate Transfer-Encoding"
|
|
+ ,.type= HTTP_RESPONSE
|
|
+ ,.raw= "HTTP/1.1 200 OK\r\n"
|
|
+ "Transfer-Encoding: chunked\r\n"
|
|
+ "Transfer-Encoding: identity\r\n"
|
|
+ "\r\n"
|
|
+ "2\r\n"
|
|
+ "OK\r\n"
|
|
+ "0\r\n"
|
|
+ "\r\n"
|
|
+ ,.should_keep_alive= FALSE
|
|
+ ,.message_complete_on_eof= TRUE
|
|
+ ,.http_major= 1
|
|
+ ,.http_minor= 1
|
|
+ ,.status_code= 200
|
|
+ ,.response_status= "OK"
|
|
+ ,.content_length= -1
|
|
+ ,.num_headers= 2
|
|
+ ,.headers=
|
|
+ { { "Transfer-Encoding", "chunked" }
|
|
+ , { "Transfer-Encoding", "identity" }
|
|
+ }
|
|
+ ,.body= "2\r\nOK\r\n0\r\n\r\n"
|
|
+ ,.num_chunks_complete= 0
|
|
+ }
|
|
};
|
|
|
|
/* strnlen() is a POSIX.2008 addition. Can't rely on it being available so
|