Transfer-Encoding parsing bugs - RFC violation

[yadhukrishnam]

Summary:
The llhttp parser in the http module in Node.js (v20.3.0) does not adhere to key-points in RFC 7230 in handling HTTP requests with both Transfer-Encoding and Content-Length headers.

Description:
RFC 7230 states the following:

Section 3.3.1 - A server that receives a request message with a transfer coding it does not understand SHOULD respond with 501 (Not Implemented).
Section 3.3.3 (3) - If a message is received with both a Transfer-Encoding and a Content-Length header field, the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt to perform request smuggling (Section 9.5) or response splitting (Section 9.4) and ought to be handled as an error.
Section 3.3.3 (3) - If a Transfer-Encoding header field is present in a request and the chunked transfer coding is not the final encoding, the message body length cannot be determined reliably; the server MUST respond with the 400 (Bad Request) status code and then close the connection.

Steps To Reproduce:

Server:

const http = require("http");
http
  .createServer((request, response) => {
    let body = [];
    request
      .on("error", (err) => {
        response.end("Request Error: " + err);
      })
      .on("data", (chunk) => {
        body.push(chunk);
      })
      .on("end", () => {
        body = Buffer.concat(body).toString();
        console.log("Response");
        console.log(request.headers);
        console.log(body);
        console.log("---");
        response.on("error", (err) => {
          response.end("Response Error: " + err);
        });
        response.end(
          "Body length: " + body.length.toString() + " Body: " + body
        );
      });
  })
  .listen(5000);

Request
Consider the following HTTP request:

POST / HTTP/1.1
Host: localhost:5000
Content-Length: 10
Transfer-Encoding:
Transfer-Encoding:
Transfer-Encoding:
2
AA
0

printf "POST / HTTP/1.1\r\n"\
               "Host: localhost:5000\r\n"\
               "Content-Length: 10\r\n"\
               "Transfer-Encoding: \r\n"\
               "Transfer-Encoding: \r\n"\
               "Transfer-Encoding: \r\n"\
               "\r\n2\r\nAA\r\n0\r\n\r\n" | nc localhost 5000

Response
The following is the stdout generated:

Response
{
  host: 'localhost:5000',
  'content-length': '10',
  'transfer-encoding': ', , '
}
2
AA
0

The following is the HTTP Response generated:

HTTP/1.1 200 OK
Date: Sun, 18 Jun 2023 10:38:11 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Content-Length: 32
Body length: 10 Body: 2
AA
0

These indicate:

• Server silently ignored invalid blank Transfer-Encoding values.
• Content-Length header was given priority even when a Transfer-Encoding header was present.
• The request was processed even when chunked was not the final encoding.
• Connection is keep-alive
  These clearly break the excerpted points from RFC 7230 stated above.
  Response from Go Lang net/http for the same HTTP Request.

HTTP/1.1 501 Not Implemented
Content-Type: text/plain; charset=utf-8
Connection: close
Unsupported transfer encoding

Supporting Material/References:

• https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3

[Dreamsorcerer]

It's only an empty value in Transfer-Encoding that reproduces the issue (not clear in the original post). If there's any value there, then you get Content-Length can't be present with Transfer-Encoding as expected.

Probably not really a security issue, as no application is going to do something with an empty Transfer-Encoding value. It'll either use Content-Length and see the same message, or it'll reject the message (which llhttp should probably do as well).

[ShogunPanda]

Fixed in #243.