Fix for CVE-2018-12122 can be bypassed via keep-alive requests

How it was found

Summary: Fix for CVE-2018-12122 can be bypassed via keep-alive requests

Description:

I'm not a security expert, neither I'm familiar with Node.js core, so please forgive me if this report is inaccurate (and in that case, sorry for your time).

While investigating the issue #515I checked out the fix to Fix for CVE-2018-12122 in node 8.14.0 and - according to my tests - the fix can be bypassed using a keep-alive connection.

The core of the fix is to introduce headersTimeout, which is a timeout that destroy the socket if all headers are not received within that timeout. As far as I can see from this changeset, the parser.parsingHeadersStart timestamp is set on connectionListenerInternal(), reset to zero once the full request headers are received (this is used as a short circuit in onParserExecute()) , but it's never set againt to a timestamp once a subsequent request on the same keep-alive connection is received.

Steps To Reproduce

1. Run an HTTP server and lower headersTimeout to 10s for simplicity (faster to test)

const http = require("http");

const server = http.createServer((req, res) => {
    res.writeHead(200);
    res.end();
});

server.headersTimeout   = 10000;
server.keepAliveTimeout = 60000;

server.listen(4050);

2. Connect with telnet localhost 4050

3. Send the first request, typing...

GET / HTTP/1.1
Connection: keep-alive

4. Then, once the server response is received, send only the first line of the subsequent request on the same connection:

GET / HTTP/1.1

5. Wait longer than the headersTimeout and send a second header

Host: localhost

6. Wait more time, if you want send further headers, and finally a newline to signal the end of the headers request. The server will not destroy the socket / close the connection after 10s (or whatever headersTimeout is set to), but will successfully reply.

Impact

To my understanding, it has the same impact of CVE-2018-12122, but I may also be terribly wrong.

Supporting Material/References:

N/A

Impact

It may DoS a Node.js application.