Update http-parser for CVE. (#1387)

Lukasa · web-flow · commit bfde40cac8ec · 2020-02-10T16:44:44.000Z
Motivation: http-parser shipped a patche for node.js CVE-2019-15605, which allowed HTTP request smuggling. This affected SwiftNIO as well, and so we need to immediately ship an update to help protect affected users. A CVE for SwiftNIO will follow, but as this patch is in the wild and SwiftNIO is known to be affected we should not delay shipping this fix. Modifications: - Update http-parser. - Add regression tests to validate this behaviour. Result: Close request smugging vector.
diff --git a/Sources/CNIOHTTPParser/c_nio_http_parser.c b/Sources/CNIOHTTPParser/c_nio_http_parser.c
@@ -384,7 +384,10 @@ enum header_states
   , h_transfer_encoding
   , h_upgrade
 
+  , h_matching_transfer_encoding_token_start
   , h_matching_transfer_encoding_chunked
+  , h_matching_transfer_encoding_token
+
   , h_matching_connection_token_start
   , h_matching_connection_keep_alive
   , h_matching_connection_close
@@ -1338,6 +1341,7 @@ size_t c_nio_http_parser_execute (http_parser *parser,
                 parser->header_state = h_general;
               } else if (parser->index == sizeof(TRANSFER_ENCODING)-2) {
                 parser->header_state = h_transfer_encoding;
+                parser->flags |= F_TRANSFER_ENCODING;
               }
               break;
 
@@ -1419,10 +1423,14 @@ size_t c_nio_http_parser_execute (http_parser *parser,
             if ('c' == c) {
               parser->header_state = h_matching_transfer_encoding_chunked;
             } else {
-              parser->header_state = h_general;
+              parser->header_state = h_matching_transfer_encoding_token;
             }
             break;
 
+          /* Multi-value `Transfer-Encoding` header */
+          case h_matching_transfer_encoding_token_start:
+            break;
+
           case h_content_length:
             if (UNLIKELY(!IS_NUM(ch))) {
               SET_ERRNO(HPE_INVALID_CONTENT_LENGTH);
@@ -1566,16 +1574,41 @@ size_t c_nio_http_parser_execute (http_parser *parser,
               goto error;
 
             /* Transfer-Encoding: chunked */
+            case h_matching_transfer_encoding_token_start:
+              /* looking for 'Transfer-Encoding: chunked' */
+              if ('c' == c) {
+                h_state = h_matching_transfer_encoding_chunked;
+              } else if (STRICT_TOKEN(c)) {
+                /* TODO(indutny): similar code below does this, but why?
+                 * At the very least it seems to be inconsistent given that
+                 * h_matching_transfer_encoding_token does not check for
+                 * `STRICT_TOKEN`
+                 */
+                h_state = h_matching_transfer_encoding_token;
+              } else if (c == ' ' || c == '\t') {
+                /* Skip lws */
+              } else {
+                h_state = h_general;
+              }
+              break;
+
             case h_matching_transfer_encoding_chunked:
               parser->index++;
               if (parser->index > sizeof(CHUNKED)-1
                   || c != CHUNKED[parser->index]) {
-                h_state = h_general;
+                h_state = h_matching_transfer_encoding_token;
               } else if (parser->index == sizeof(CHUNKED)-2) {
                 h_state = h_transfer_encoding_chunked;
               }
               break;
 
+            case h_matching_transfer_encoding_token:
+              if (ch == ',') {
+                h_state = h_matching_transfer_encoding_token_start;
+                parser->index = 0;
+              }
+              break;
+
             case h_matching_connection_token_start:
               /* looking for 'Connection: keep-alive' */
               if (c == 'k') {
@@ -1634,7 +1667,7 @@ size_t c_nio_http_parser_execute (http_parser *parser,
               break;
 
             case h_transfer_encoding_chunked:
-              if (ch != ' ') h_state = h_general;
+              if (ch != ' ') h_state = h_matching_transfer_encoding_token;
               break;
 
             case h_connection_keep_alive:
@@ -1768,12 +1801,17 @@ size_t c_nio_http_parser_execute (http_parser *parser,
           REEXECUTE();
         }
 
-        /* Cannot use chunked encoding and a content-length header together
-           per the HTTP specification. */
-        if ((parser->flags & F_CHUNKED) &&
+        /* Cannot us transfer-encoding and a content-length header together
+           per the HTTP specification. (RFC 7230 Section 3.3.3) */
+        if ((parser->flags & F_TRANSFER_ENCODING) &&
             (parser->flags & F_CONTENTLENGTH)) {
-          SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
-          goto error;
+          /* Allow it for lenient parsing as long as `Transfer-Encoding` is
+           * not `chunked`
+           */
+          if (!lenient || (parser->flags & F_CHUNKED)) {
+            SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
+            goto error;
+          }
         }
 
         UPDATE_STATE(s_headers_done);
@@ -1848,8 +1886,31 @@ size_t c_nio_http_parser_execute (http_parser *parser,
           UPDATE_STATE(NEW_MESSAGE());
           CALLBACK_NOTIFY(message_complete);
         } else if (parser->flags & F_CHUNKED) {
-          /* chunked encoding - ignore Content-Length header */
+          /* chunked encoding - ignore Content-Length header,
+           * prepare for a chunk */
           UPDATE_STATE(s_chunk_size_start);
+        } else if (parser->flags & F_TRANSFER_ENCODING) {
+          if (parser->type == HTTP_REQUEST && !lenient) {
+            /* RFC 7230 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.
+             */
+            SET_ERRNO(HPE_INVALID_TRANSFER_ENCODING);
+            RETURN(p - data); /* Error */
+          } else {
+            /* RFC 7230 3.3.3 */
+
+            /* If a Transfer-Encoding header field is present in a response and
+             * the chunked transfer coding is not the final encoding, the
+             * message body length is determined by reading the connection until
+             * it is closed by the server.
+             */
+            UPDATE_STATE(s_body_identity_eof);
+          }
         } else {
           if (parser->content_length == 0) {
             /* Content-Length header given but zero: Content-Length: 0\r\n */
@@ -2103,6 +2164,12 @@ c_nio_http_message_needs_eof (const http_parser *parser)
     return 0;
   }
 
+  /* RFC 7230 3.3.3, see `s_headers_almost_done` */
+  if ((parser->flags & F_TRANSFER_ENCODING) &&
+      (parser->flags & F_CHUNKED) == 0) {
+    return 1;
+  }
+
   if ((parser->flags & F_CHUNKED) || parser->content_length != ULLONG_MAX) {
     return 0;
   }
diff --git a/Sources/CNIOHTTPParser/include/c_nio_http_parser.h b/Sources/CNIOHTTPParser/include/c_nio_http_parser.h
@@ -30,7 +30,7 @@ extern "C" {
 /* Also update SONAME in the Makefile whenever you change these. */
 #define HTTP_PARSER_VERSION_MAJOR 2
 #define HTTP_PARSER_VERSION_MINOR 9
-#define HTTP_PARSER_VERSION_PATCH 2
+#define HTTP_PARSER_VERSION_PATCH 3
 
 #include <stddef.h>
 #if defined(_WIN32) && !defined(__MINGW32__) && \
@@ -228,6 +228,7 @@ enum flags
   , F_UPGRADE               = 1 << 5
   , F_SKIPBODY              = 1 << 6
   , F_CONTENTLENGTH         = 1 << 7
+  , F_TRANSFER_ENCODING     = 1 << 8
   };
 
 
@@ -274,6 +275,8 @@ enum flags
      "unexpected content-length header")                             \
   XX(INVALID_CHUNK_SIZE,                                             \
      "invalid character in chunk size header")                       \
+  XX(INVALID_TRANSFER_ENCODING,                                      \
+     "request has invalid transfer-encoding")                        \
   XX(INVALID_CONSTANT, "invalid constant string")                    \
   XX(INVALID_INTERNAL_STATE, "encountered unexpected internal state")\
   XX(STRICT, "strict mode assertion failed")                         \
@@ -296,11 +299,11 @@ enum http_errno {
 struct http_parser {
   /** PRIVATE **/
   unsigned int type : 2;         /* enum http_parser_type */
-  unsigned int flags : 8;        /* F_* values from 'flags' enum; semi-public */
   unsigned int state : 7;        /* enum state from http_parser.c */
   unsigned int header_state : 7; /* enum header_state from http_parser.c */
   unsigned int index : 7;        /* index into current matcher */
   unsigned int lenient_http_headers : 1;
+  unsigned int flags : 16;       /* F_* values from 'flags' enum; semi-public */
 
   uint32_t nread;          /* # bytes read in various scenarios */
   uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */
diff --git a/Sources/NIOHTTP1/HTTPDecoder.swift b/Sources/NIOHTTP1/HTTPDecoder.swift
@@ -706,6 +706,9 @@ extension HTTPParserError {
             return .paused
         case HPE_UNKNOWN:
             return .unknown
+        case HPE_INVALID_TRANSFER_ENCODING:
+            // The downside of enums here, we don't have a case for this. Map it to .unknown for now.
+            return .unknown
         default:
             return nil
         }
diff --git a/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest+XCTest.swift b/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest+XCTest.swift
@@ -46,7 +46,7 @@ extension HTTPDecoderLengthTest {
                 ("testIgnoresTransferEncodingFieldOn304Responses", testIgnoresTransferEncodingFieldOn304Responses),
                 ("testIgnoresContentLengthFieldOn304Responses", testIgnoresContentLengthFieldOn304Responses),
                 ("testEarlyFinishWithoutLengthAtAllOn304Responses", testEarlyFinishWithoutLengthAtAllOn304Responses),
-                ("testMultipleTEWithChunkedLastHasNoBodyOnRequest", testMultipleTEWithChunkedLastHasNoBodyOnRequest),
+                ("testMultipleTEWithChunkedLastWorksFine", testMultipleTEWithChunkedLastWorksFine),
                 ("testMultipleTEWithChunkedFirstHasNoBodyOnRequest", testMultipleTEWithChunkedFirstHasNoBodyOnRequest),
                 ("testMultipleTEWithChunkedInTheMiddleHasNoBodyOnRequest", testMultipleTEWithChunkedInTheMiddleHasNoBodyOnRequest),
                 ("testMultipleTEWithChunkedLastHasEOFBodyOnResponseWithChannelInactive", testMultipleTEWithChunkedLastHasEOFBodyOnResponseWithChannelInactive),
diff --git a/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift b/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift
@@ -292,14 +292,26 @@ class HTTPDecoderLengthTest: XCTestCase {
         try assertIgnoresLengthFields(requestMethod: .GET, responseStatus: .notModified, responseFramingField: .neither)
     }
 
-    private func assertRequestTransferEncodingHasNoBody(transferEncodingHeader: String) throws {
+    private func assertRequestTransferEncodingInError(transferEncodingHeader: String) throws {
         XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder())).wait())
 
         let handler = MessageEndHandler<HTTPRequestHead, ByteBuffer>()
         XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait())
 
         // Send a GET with the appropriate Transfer Encoding header.
-        XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "POST / HTTP/1.1\r\nTransfer-Encoding: \(transferEncodingHeader)\r\n\r\n")))
+        XCTAssertThrowsError(try channel.writeInbound(ByteBuffer(string: "POST / HTTP/1.1\r\nTransfer-Encoding: \(transferEncodingHeader)\r\n\r\n"))) { error in
+            XCTAssertEqual(error as? HTTPParserError, .unknown)
+        }
+    }
+
+    func testMultipleTEWithChunkedLastWorksFine() throws {
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder())).wait())
+
+        let handler = MessageEndHandler<HTTPRequestHead, ByteBuffer>()
+        XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait())
+
+        // Send a GET with the appropriate Transfer Encoding header.
+        XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "POST / HTTP/1.1\r\nTransfer-Encoding: gzip, chunked\r\n\r\n0\r\n\r\n")))
 
         // We should have a request, no body, and immediately see end of request.
         XCTAssert(handler.seenHead)
@@ -309,22 +321,12 @@ class HTTPDecoderLengthTest: XCTestCase {
         XCTAssertTrue(try channel.finish().isClean)
     }
 
-    func testMultipleTEWithChunkedLastHasNoBodyOnRequest() throws {
-        // This is not quite right: RFC 7230 should allow this as chunked. However, http_parser
-        // does not, so we don't either.
-        try assertRequestTransferEncodingHasNoBody(transferEncodingHeader: "gzip, chunked")
-    }
-
     func testMultipleTEWithChunkedFirstHasNoBodyOnRequest() throws {
-        // Here http_parser is again wrong: strictly this should 400. Again, though,
-        // if http_parser doesn't support this neither do we.
-        try assertRequestTransferEncodingHasNoBody(transferEncodingHeader: "chunked, gzip")
+        try assertRequestTransferEncodingInError(transferEncodingHeader: "chunked, gzip")
     }
 
     func testMultipleTEWithChunkedInTheMiddleHasNoBodyOnRequest() throws {
-        // Here http_parser is again wrong: strictly this should 400. Again, though,
-        // if http_parser doesn't support this neither do we.
-        try assertRequestTransferEncodingHasNoBody(transferEncodingHeader: "gzip, chunked, deflate")
+        try assertRequestTransferEncodingInError(transferEncodingHeader: "gzip, chunked, deflate")
     }
 
     private func assertResponseTransferEncodingHasBodyTerminatedByEOF(transferEncodingHeader: String, eofMechanism: EOFMechanism) throws {
@@ -366,10 +368,51 @@ class HTTPDecoderLengthTest: XCTestCase {
         XCTAssertTrue(try channel.finish().isClean)
     }
 
+    private func assertResponseTransferEncodingHasBodyTerminatedByEndOfChunk(transferEncodingHeader: String, eofMechanism: EOFMechanism) throws {
+        XCTAssertNoThrow(try channel.pipeline.addHandler(HTTPRequestEncoder()).wait())
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPResponseDecoder())).wait())
+
+        let handler = MessageEndHandler<HTTPResponseHead, ByteBuffer>()
+        XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait())
+
+        // Prime the decoder with a request and consume it.
+        XCTAssertTrue(try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 1, minor: 1),
+                                                                                           method: .GET,
+                                                                                           uri: "/"))).isFull)
+        XCTAssertNoThrow(XCTAssertNotNil(try channel.readOutbound(as: ByteBuffer.self)))
+
+        // Send a 200 with the appropriate Transfer Encoding header. We should see the request.
+        XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "HTTP/1.1 200 OK\r\nTransfer-Encoding: \(transferEncodingHeader)\r\n\r\n")))
+        XCTAssert(handler.seenHead)
+        XCTAssertFalse(handler.seenBody)
+        XCTAssertFalse(handler.seenEnd)
+
+        // Now send body. Note that this *is* chunk encoded. We should also see a body.
+        XCTAssertNoThrow(try channel.writeInbound(ByteBuffer(string: "9\r\ncaribbean\r\n")))
+        XCTAssert(handler.seenHead)
+        XCTAssert(handler.seenBody)
+        XCTAssertFalse(handler.seenEnd)
+
+        // Now send EOF. This should error, as we're expecting the end chunk.
+        if case .halfClosure = eofMechanism {
+            channel.pipeline.fireUserInboundEventTriggered(ChannelEvent.inputClosed)
+        } else {
+            channel.pipeline.fireChannelInactive()
+        }
+
+        XCTAssert(handler.seenHead)
+        XCTAssert(handler.seenBody)
+        XCTAssertFalse(handler.seenEnd)
+
+        XCTAssertThrowsError(try channel.throwIfErrorCaught()) { error in
+            XCTAssertEqual(error as? HTTPParserError, .invalidEOFState)
+        }
+
+        XCTAssertTrue(try channel.finish().isClean)
+    }
+
     func testMultipleTEWithChunkedLastHasEOFBodyOnResponseWithChannelInactive() throws {
-        // This is not right: RFC 7230 should allow this as chunked, but http_parser instead parses it as
-        // EOF-terminated. We can't easily override that, so we don't.
-        try assertResponseTransferEncodingHasBodyTerminatedByEOF(transferEncodingHeader: "gzip, chunked", eofMechanism: .channelInactive)
+        try assertResponseTransferEncodingHasBodyTerminatedByEndOfChunk(transferEncodingHeader: "gzip, chunked", eofMechanism: .channelInactive)
     }
 
     func testMultipleTEWithChunkedFirstHasEOFBodyOnResponseWithChannelInactive() throws {
@@ -383,9 +426,7 @@ class HTTPDecoderLengthTest: XCTestCase {
     }
 
     func testMultipleTEWithChunkedLastHasEOFBodyOnResponseWithHalfClosure() throws {
-        // This is not right: RFC 7230 should allow this as chunked, but http_parser instead parses it as
-        // EOF-terminated. We can't easily override that, so we don't.
-        try assertResponseTransferEncodingHasBodyTerminatedByEOF(transferEncodingHeader: "gzip, chunked", eofMechanism: .halfClosure)
+        try assertResponseTransferEncodingHasBodyTerminatedByEndOfChunk(transferEncodingHeader: "gzip, chunked", eofMechanism: .halfClosure)
     }
 
     func testMultipleTEWithChunkedFirstHasEOFBodyOnResponseWithHalfClosure() throws {
diff --git a/Tests/NIOHTTP1Tests/HTTPDecoderTest+XCTest.swift b/Tests/NIOHTTP1Tests/HTTPDecoderTest+XCTest.swift
@@ -54,6 +54,8 @@ extension HTTPDecoderTest {
                 ("testAppropriateErrorWhenReceivingUnsolicitedResponse", testAppropriateErrorWhenReceivingUnsolicitedResponse),
                 ("testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover", testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover),
                 ("testOneRequestTwoResponses", testOneRequestTwoResponses),
+                ("testRefusesRequestSmugglingAttempt", testRefusesRequestSmugglingAttempt),
+                ("testTrimsTrailingOWS", testTrimsTrailingOWS),
            ]
    }
 }
diff --git a/Tests/NIOHTTP1Tests/HTTPDecoderTest.swift b/Tests/NIOHTTP1Tests/HTTPDecoderTest.swift
@@ -832,4 +832,46 @@ class HTTPDecoderTest: XCTestCase {
         XCTAssertEqual(["channelReadComplete", "write", "flush", "channelRead", "errorCaught"], eventCounter.allTriggeredEvents())
         XCTAssertNoThrow(XCTAssertTrue(try channel.finish().isClean))
     }
+
+    func testRefusesRequestSmugglingAttempt() throws {
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder())).wait())
+
+        // This is a request smuggling attempt caused by duplicating the Transfer-Encoding and Content-Length headers.
+        var buffer = channel.allocator.buffer(capacity: 256)
+        buffer.writeString("POST /foo HTTP/1.1\r\n" +
+                           "Host: localhost\r\n" +
+                           "Content-length: 1\r\n" +
+                           "Transfer-Encoding: gzip, chunked\r\n\r\n" +
+                           "3\r\na=1\r\n0\r\n\r\n")
+
+        do {
+            try channel.writeInbound(buffer)
+            XCTFail("Did not error")
+        } catch HTTPParserError.unexpectedContentLength {
+            // ok
+        } catch {
+            XCTFail("Unexpected error: \(error)")
+        }
+
+        loop.run()
+    }
+
+    func testTrimsTrailingOWS() throws {
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder())).wait())
+
+        var buffer = channel.allocator.buffer(capacity: 64)
+        buffer.writeStaticString("GET / HTTP/1.1\r\nHost: localhost\r\nFoo: bar \r\nBaz: Boz\r\n\r\n")
+
+        XCTAssertNoThrow(try channel.writeInbound(buffer))
+        let request = try assertNoThrowWithValue(channel.readInbound(as: HTTPServerRequestPart.self))
+        guard case .some(.head(let head)) = request else {
+            XCTFail("Unexpected first message: \(String(describing: request))")
+            return
+        }
+        XCTAssertEqual(head.headers[canonicalForm: "Foo"], ["bar"])
+        guard case .some(.end) = try assertNoThrowWithValue(channel.readInbound(as: HTTPServerRequestPart.self)) else {
+            XCTFail("Unexpected last message")
+            return
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,8 @@ extension HTTPDecoderTest {`
`54`	`54`	`("testAppropriateErrorWhenReceivingUnsolicitedResponse", testAppropriateErrorWhenReceivingUnsolicitedResponse),`
`55`	`55`	`("testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover", testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover),`
`56`	`56`	`("testOneRequestTwoResponses", testOneRequestTwoResponses),`
	`57`	`+ ("testRefusesRequestSmugglingAttempt", testRefusesRequestSmugglingAttempt),`
	`58`	`+ ("testTrimsTrailingOWS", testTrimsTrailingOWS),`
`57`	`59`	`]`
`58`	`60`	`}`
`59`	`61`	`}`