http: stricter Transfer-Encoding parsing

nodejs · mcollina · Jul 6, 2022 · Mar 31, 2022 · Apr 27, 2022 · Apr 27, 2022
commit b9d7ec327443feb6e737a8ca9d2c26aecae51a41
diff --git a/src/llhttp/constants.ts b/src/llhttp/constants.ts
@@ -58,6 +58,7 @@ export enum LENIENT_FLAGS {
   HEADERS = 1 << 0,
   CHUNKED_LENGTH = 1 << 1,
   KEEP_ALIVE = 1 << 2,
+  TRANSFER_ENCODING = 1 << 3,
 }
 
 export enum METHODS {

diff --git a/src/llhttp/http.ts b/src/llhttp/http.ts
@@ -490,11 +490,27 @@ export class HTTP {
       FLAGS.CHUNKED,
       'header_value_te_chunked');
 
+    // Once chunked has been selected, no other encoding is possible in requests
+    // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.1
+    const forbidAfterChunkedInRequest = (otherwise: Node) => {
+      return this.load('type', {
+        [TYPE.REQUEST]: this.testLenientFlags(LENIENT_FLAGS.TRANSFER_ENCODING, {
+          0: span.headerValue.end().skipTo(
+            p.error(ERROR.INVALID_TRANSFER_ENCODING, 'Invalid `Transfer-Encoding` header value'),
+          ),
+        }).otherwise(otherwise),
+      }, otherwise);
+    };
+
     n('header_value_start')
       .otherwise(this.load('header_state', {
         [HEADER_STATE.UPGRADE]: this.setFlag(FLAGS.UPGRADE, fallback),
-        [HEADER_STATE.TRANSFER_ENCODING]: this.setFlag(
-          FLAGS.TRANSFER_ENCODING, toTransferEncoding),
+        [HEADER_STATE.TRANSFER_ENCODING]: this.testFlags(
+          FLAGS.CHUNKED,
+          {
+            1: forbidAfterChunkedInRequest(this.setFlag(FLAGS.TRANSFER_ENCODING, toTransferEncoding)),
+          },
+          this.setFlag(FLAGS.TRANSFER_ENCODING, toTransferEncoding)),
         [HEADER_STATE.CONTENT_LENGTH]: n('header_value_content_length_once'),
         [HEADER_STATE.CONNECTION]: n('header_value_connection'),
       }, 'header_value'));
@@ -513,9 +529,12 @@ export class HTTP {
 
     n('header_value_te_chunked_last')
       .match(' ', n('header_value_te_chunked_last'))
+      // Make sure a chunked word repeated more than once does not enable chunked encoding
+      .match('chunked', n('header_value_te_token'))
       .peek([ '\r', '\n' ], this.update('header_state',
         HEADER_STATE.TRANSFER_ENCODING_CHUNKED,
         'header_value_otherwise'))
+      .peek(',', forbidAfterChunkedInRequest(n('header_value_te_chunked')))
       .otherwise(n('header_value_te_chunked'));
 
     n('header_value_te_token')

diff --git a/src/native/api.c b/src/native/api.c
@@ -253,6 +253,14 @@ void llhttp_set_lenient_keep_alive(llhttp_t* parser, int enabled) {
   }
 }
 
+void llhttp_set_lenient_transfer_encoding(llhttp_t* parser, int enabled) {
+  if (enabled) {
+    parser->lenient_flags |= LENIENT_TRANSFER_ENCODING;
+  } else {
+    parser->lenient_flags &= ~LENIENT_TRANSFER_ENCODING;
+  }
+}
+
 /* Callbacks */
 
 

diff --git a/src/native/api.h b/src/native/api.h
@@ -246,6 +246,19 @@ void llhttp_set_lenient_chunked_length(llhttp_t* parser, int enabled);
  */
 void llhttp_set_lenient_keep_alive(llhttp_t* parser, int enabled);
 
+/* Enables/disables lenient handling of `Transfer-Encoding` header.
+ *
+ * Normally `llhttp` would error when a `Transfer-Encoding` has `chunked` value
+ * and another value after it (either in a single header or in multiple
+ * headers whose value are internally joined using `, `).
+ * This is mandated by the spec to reliably determine request body size and thus
+ * avoid request smuggling.
+ * With this flag the extra value will be parsed normally.
+ *
+ * **(USE AT YOUR OWN RISK)**
+ */
+void llhttp_set_lenient_transfer_encoding(llhttp_t* parser, int enabled);
+
 #ifdef __cplusplus
 }  /* extern "C" */
 #endif

diff --git a/src/native/http.c b/src/native/http.c
@@ -52,7 +52,8 @@ int llhttp__after_headers_complete(llhttp_t* parser, const char* p,
     return 2;
   } else if (parser->flags & F_TRANSFER_ENCODING) {
     if (parser->type == HTTP_REQUEST &&
-        (parser->lenient_flags & LENIENT_CHUNKED_LENGTH) == 0) {
+        (parser->lenient_flags & LENIENT_CHUNKED_LENGTH) == 0 &&
+        (parser->lenient_flags & LENIENT_TRANSFER_ENCODING) == 0) {
       /* RFC 7230 3.3.3 */
 
       /* If a Transfer-Encoding header field

diff --git a/test/fixtures/extra.c b/test/fixtures/extra.c
@@ -82,6 +82,11 @@ void llhttp__test_init_request_lenient_keep_alive(llparse_t* s) {
   s->lenient_flags |= LENIENT_KEEP_ALIVE;
 }
 
+void llhttp__test_init_request_lenient_transfer_encoding(llparse_t* s) {
+  llhttp__test_init_request(s);
+  s->lenient_flags |= LENIENT_TRANSFER_ENCODING;
+}
+
 
 void llhttp__test_init_response_lenient_keep_alive(llparse_t* s) {
   llhttp__test_init_response(s);

diff --git a/test/fixtures/index.ts b/test/fixtures/index.ts
@@ -9,8 +9,9 @@ import * as path from 'path';
 import * as llhttp from '../../src/llhttp';
 
 export type TestType = 'request' | 'response' | 'request-lenient-headers' |
-  'request-lenient-chunked-length' | 'request-lenient-keep-alive' |
-  'response-lenient-keep-alive' | 'request-finish' | 'response-finish' |
+  'request-lenient-chunked-length' | 'request-lenient-transfer-encoding' |
+  'request-lenient-keep-alive' | 'response-lenient-keep-alive' |
+  'request-finish' | 'response-finish' |
   'none' | 'url';
 
 export { FixtureResult };
@@ -63,6 +64,7 @@ export async function build(
   if (ty === 'request' || ty === 'response' ||
       ty === 'request-lenient-headers' ||
       ty === 'request-lenient-chunked-length' ||
+      ty === 'request-lenient-transfer-encoding' ||
       ty === 'request-lenient-keep-alive' ||
       ty === 'response-lenient-keep-alive') {
     extra.push(

diff --git a/test/md-test.ts b/test/md-test.ts
@@ -84,6 +84,8 @@ const http: IFixtureMap = {
     'request-lenient-headers': buildMode('loose', 'request-lenient-headers'),
     'request-lenient-keep-alive': buildMode(
       'loose', 'request-lenient-keep-alive'),
+    'request-lenient-transfer-encoding':
+      buildMode('loose', 'request-lenient-transfer-encoding'),
     'response': buildMode('loose', 'response'),
     'response-finish': buildMode('loose', 'response-finish'),
     'response-lenient-keep-alive': buildMode(
@@ -99,6 +101,8 @@ const http: IFixtureMap = {
     'request-lenient-headers': buildMode('strict', 'request-lenient-headers'),
     'request-lenient-keep-alive': buildMode(
       'strict', 'request-lenient-keep-alive'),
+    'request-lenient-transfer-encoding':
+      buildMode('loose', 'request-lenient-transfer-encoding'),
     'response': buildMode('strict', 'response'),
     'response-finish': buildMode('strict', 'response-finish'),
     'response-lenient-keep-alive': buildMode(
@@ -165,6 +169,8 @@ function run(name: string): void {
           types = [ 'request-lenient-chunked-length' ];
         } else if (meta.type === 'request-lenient-keep-alive') {
           types = [ 'request-lenient-keep-alive' ];
+        } else if (meta.type === 'request-lenient-transfer-encoding') {
+          types = [ 'request-lenient-transfer-encoding' ];
         } else if (meta.type === 'response-lenient-keep-alive') {
           types = [ 'response-lenient-keep-alive' ];
         } else if (meta.type === 'response-only') {

diff --git a/test/request/transfer-encoding.md b/test/request/transfer-encoding.md
@@ -347,10 +347,8 @@ off=62 len=3 span[header_value]="*/*"
 off=67 header_value complete
 off=67 len=17 span[header_field]="Transfer-Encoding"
 off=85 header_field complete
-off=86 len=16 span[header_value]="chunked, deflate"
-off=104 header_value complete
-off=106 headers complete method=3 v=1/1 flags=200 content_length=0
-off=106 error code=15 reason="Request has invalid `Transfer-Encoding`"
+off=86 len=7 span[header_value]="chunked"
+off=94 error code=15 reason="Invalid `Transfer-Encoding` header value"
 ```
 
 ## POST with `chunked` and duplicate transfer-encoding
@@ -379,18 +377,13 @@ off=86 len=7 span[header_value]="chunked"
 off=95 header_value complete
 off=95 len=17 span[header_field]="Transfer-Encoding"
 off=113 header_field complete
-off=114 len=7 span[header_value]="deflate"
-off=123 header_value complete
-off=125 headers complete method=3 v=1/1 flags=200 content_length=0
-off=125 error code=15 reason="Request has invalid `Transfer-Encoding`"
+off=114 len=0 span[header_value]=""
+off=115 error code=15 reason="Invalid `Transfer-Encoding` header value"
 ```
 
 ## POST with `chunked` before other transfer-coding (lenient)
 
-TODO(indutny): should we allow it even in lenient mode? (Consider disabling
-this).
-
-<!-- meta={"type": "request-lenient-chunked-length"} -->
+<!-- meta={"type": "request-lenient-transfer-encoding"} -->
 ```http
 POST /post_identity_body_world?q=search#hey HTTP/1.1
 Accept: */*
@@ -415,7 +408,39 @@ off=106 headers complete method=3 v=1/1 flags=200 content_length=0
 off=106 len=5 span[body]="World"
 ```
 
-## POST with `chunked` as last transfer-coding
+## POST with `chunked` and duplicate transfer-encoding (lenient)
+
+<!-- meta={"type": "request-lenient-transfer-encoding"} -->
+```http
+POST /post_identity_body_world?q=search#hey HTTP/1.1
+Accept: */*
+Transfer-Encoding: chunked
+Transfer-Encoding: deflate
+
+World
+```
+
+```log
+off=0 message begin
+off=5 len=38 span[url]="/post_identity_body_world?q=search#hey"
+off=44 url complete
+off=54 len=6 span[header_field]="Accept"
+off=61 header_field complete
+off=62 len=3 span[header_value]="*/*"
+off=67 header_value complete
+off=67 len=17 span[header_field]="Transfer-Encoding"
+off=85 header_field complete
+off=86 len=7 span[header_value]="chunked"
+off=95 header_value complete
+off=95 len=17 span[header_field]="Transfer-Encoding"
+off=113 header_field complete
+off=114 len=7 span[header_value]="deflate"
+off=123 header_value complete
+off=125 headers complete method=3 v=1/1 flags=200 content_length=0
+off=125 len=5 span[body]="World"
+```
+
+## POST with `chunked` as last transfer-encoding
 
 <!-- meta={"type": "request"} -->
 ```http
@@ -451,6 +476,78 @@ off=121 chunk complete
 off=121 message complete
 ```
 
+## POST with `chunked` as last transfer-encoding (multiple headers)
+
+<!-- meta={"type": "request"} -->
+```http
+POST /post_identity_body_world?q=search#hey HTTP/1.1
+Accept: */*
+Transfer-Encoding: deflate
+Transfer-Encoding: chunked
+
+5
+World
+0
+
+
+```
+
+```log
+off=0 message begin
+off=5 len=38 span[url]="/post_identity_body_world?q=search#hey"
+off=44 url complete
+off=54 len=6 span[header_field]="Accept"
+off=61 header_field complete
+off=62 len=3 span[header_value]="*/*"
+off=67 header_value complete
+off=67 len=17 span[header_field]="Transfer-Encoding"
+off=85 header_field complete
+off=86 len=7 span[header_value]="deflate"
+off=95 header_value complete
+off=95 len=17 span[header_field]="Transfer-Encoding"
+off=113 header_field complete
+off=114 len=7 span[header_value]="chunked"
+off=123 header_value complete
+off=125 headers complete method=3 v=1/1 flags=208 content_length=0
+off=128 chunk header len=5
+off=128 len=5 span[body]="World"
+off=135 chunk complete
+off=138 chunk header len=0
+off=140 chunk complete
+off=140 message complete
+```
+
+## POST with `chunkedchunked` as transfer-encoding
+
+<!-- meta={"type": "request"} -->
+```http
+POST /post_identity_body_world?q=search#hey HTTP/1.1
+Accept: */*
+Transfer-Encoding: chunkedchunked
+
+5
+World
+0
+
+
+```
+
+```log
+off=0 message begin
+off=5 len=38 span[url]="/post_identity_body_world?q=search#hey"
+off=44 url complete
+off=54 len=6 span[header_field]="Accept"
+off=61 header_field complete
+off=62 len=3 span[header_value]="*/*"
+off=67 header_value complete
+off=67 len=17 span[header_field]="Transfer-Encoding"
+off=85 header_field complete
+off=86 len=14 span[header_value]="chunkedchunked"
+off=102 header_value complete
+off=104 headers complete method=3 v=1/1 flags=200 content_length=0
+off=104 error code=15 reason="Request has invalid `Transfer-Encoding`"
+```
+
 ## Missing last-chunk
 
 <!-- meta={"type": "request"} -->

diff --git a/test/response/transfer-encoding.md b/test/response/transfer-encoding.md
@@ -48,7 +48,7 @@ off=159 chunk complete
 off=159 message complete
 ```
 
-## `chunked` before other transfer-coding
+### `chunked` before other transfer-encoding
 
 <!-- meta={"type": "response"} -->
 ```http
@@ -74,3 +74,78 @@ off=67 header_value complete
 off=69 headers complete status=200 v=1/1 flags=200 content_length=0
 off=69 len=5 span[body]="World"
 ```
+
+### multiple transfer-encoding where chunked is not the last one
+
+<!-- meta={"type": "response"} -->
+```http
+HTTP/1.1 200 OK
+Accept: */*
+Transfer-Encoding: chunked
+Transfer-Encoding: identity
+
+World
+```
+
+```log
+off=0 message begin
+off=13 len=2 span[status]="OK"
+off=17 status complete
+off=17 len=6 span[header_field]="Accept"
+off=24 header_field complete
+off=25 len=3 span[header_value]="*/*"
+off=30 header_value complete
+off=30 len=17 span[header_field]="Transfer-Encoding"
+off=48 header_field complete
+off=49 len=7 span[header_value]="chunked"
+off=58 header_value complete
+off=58 len=17 span[header_field]="Transfer-Encoding"
+off=76 header_field complete
+off=77 len=8 span[header_value]="identity"
+off=87 header_value complete
+off=89 headers complete status=200 v=1/1 flags=200 content_length=0
+off=89 len=5 span[body]="World"
+```
+
+### `chunkedchunked` transfer-encoding does not enable chunked enconding
+
+This check that the word `chunked` repeat more than once (with or without spaces) does not mistakenly enables chunked encoding.
+
+<!-- meta={"type": "response"} -->
+```http
+HTTP/1.1 200 OK
+Accept: */*
+Transfer-Encoding: chunkedchunked
+
+2
+OK
+0
+
+
+```
+
+```log
+off=0 message begin
+off=13 len=2 span[status]="OK"
+off=17 status complete
+off=17 len=6 span[header_field]="Accept"
+off=24 header_field complete
+off=25 len=3 span[header_value]="*/*"
+off=30 header_value complete
+off=30 len=17 span[header_field]="Transfer-Encoding"
+off=48 header_field complete
+off=49 len=14 span[header_value]="chunkedchunked"
+off=65 header_value complete
+off=67 headers complete status=200 v=1/1 flags=200 content_length=0
+off=67 len=1 span[body]="2"
+off=68 len=1 span[body]=cr
+off=69 len=1 span[body]=lf
+off=70 len=2 span[body]="OK"
+off=72 len=1 span[body]=cr
+off=73 len=1 span[body]=lf
+off=74 len=1 span[body]="0"
+off=75 len=1 span[body]=cr
+off=76 len=1 span[body]=lf
+off=77 len=1 span[body]=cr
+off=78 len=1 span[body]=lf
+```