Merge pull request from GHSA-v3r5-pjpm-mwgq

Lukasa · web-flow · commit 7f05a8da46cc · 2023-01-17T11:06:08.000Z
Motivation

Allowing arbitrary data in outbound header field values allows for the
possibility that users of AHC will accidentally pass untrusted data into
those values. That untrusted data can substantially alter the parsing
and content of the HTTP requests, which is extremely dangerous. The
result of this is vulnerability to CRLF injection.

Modifications

Add validation of outbound header field values.

Result

No longer vulnerable to CRLF injection
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -1032,6 +1032,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case uncleanShutdown
         case traceRequestWithBody
         case invalidHeaderFieldNames([String])
+        case invalidHeaderFieldValues([String])
         case bodyLengthMismatch
         case writeAfterRequestSent
         @available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.")
@@ -1100,6 +1101,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
             return "Trace request with body"
         case .invalidHeaderFieldNames:
             return "Invalid header field names"
+        case .invalidHeaderFieldValues:
+            return "Invalid header field values"
         case .bodyLengthMismatch:
             return "Body length mismatch"
         case .writeAfterRequestSent:
@@ -1166,6 +1169,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
     public static let traceRequestWithBody = HTTPClientError(code: .traceRequestWithBody)
     /// Header field names contain invalid characters.
     public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) }
+    /// Header field values contain invalid characters.
+    public static func invalidHeaderFieldValues(_ values: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldValues(values)) }
     /// Body length is not equal to `Content-Length`.
     public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch)
     /// Body part was written after request was fully sent.
diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift
@@ -21,6 +21,7 @@ extension HTTPHeaders {
         bodyLength: RequestBodyLength
     ) throws -> RequestFramingMetadata {
         try self.validateFieldNames()
+        try self.validateFieldValues()
 
         if case .TRACE = method {
             switch bodyLength {
@@ -80,6 +81,56 @@ extension HTTPHeaders {
         }
     }
 
+    private func validateFieldValues() throws {
+        let invalidValues = self.compactMap { _, value -> String? in
+            let satisfy = value.utf8.allSatisfy { char -> Bool in
+                /// Validates a byte of a given header field value against the definition in RFC 9110.
+                ///
+                /// The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid
+                /// characters as the following:
+                ///
+                /// ```
+                /// field-value    = *field-content
+                /// field-content  = field-vchar
+                ///                  [ 1*( SP / HTAB / field-vchar ) field-vchar ]
+                /// field-vchar    = VCHAR / obs-text
+                /// obs-text       = %x80-FF
+                /// ```
+                ///
+                /// Additionally, it makes the following note:
+                ///
+                /// "Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the
+                /// varying ways that implementations might parse and interpret those characters; a recipient
+                /// of CR, LF, or NUL within a field value MUST either reject the message or replace each of
+                /// those characters with SP before further processing or forwarding of that message. Field
+                /// values containing other CTL characters are also invalid; however, recipients MAY retain
+                /// such characters for the sake of robustness when they appear within a safe context (e.g.,
+                /// an application-specific quoted string that will not be processed by any downstream HTTP
+                /// parser)."
+                ///
+                /// As we cannot guarantee the context is safe, this code will reject all ASCII control characters
+                /// directly _except_ for HTAB, which is explicitly allowed.
+                switch char {
+                case UInt8(ascii: "\t"):
+                    // HTAB, explicitly allowed.
+                    return true
+                case 0...0x1f, 0x7F:
+                    // ASCII control character, forbidden.
+                    return false
+                default:
+                    // Printable or non-ASCII, allowed.
+                    return true
+                }
+            }
+
+            return satisfy ? nil : value
+        }
+
+        guard invalidValues.count == 0 else {
+            throw HTTPClientError.invalidHeaderFieldValues(invalidValues)
+        }
+    }
+
     private mutating func setTransportFraming(
         method: HTTPMethod,
         bodyLength: RequestBodyLength
diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests+XCTest.swift
@@ -45,6 +45,10 @@ extension AsyncAwaitEndToEndTests {
             ("testShutdown", testShutdown),
             ("testCancelingBodyDoesNotCrash", testCancelingBodyDoesNotCrash),
             ("testAsyncSequenceReuse", testAsyncSequenceReuse),
+            ("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1),
+            ("testRejectsInvalidCharactersInHeaderFieldNames_http2", testRejectsInvalidCharactersInHeaderFieldNames_http2),
+            ("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1),
+            ("testRejectsInvalidCharactersInHeaderFieldValues_http2", testRejectsInvalidCharactersInHeaderFieldValues_http2),
         ]
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift
@@ -597,6 +597,134 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
             XCTAssertEqual(body, ByteBuffer(string: "1234"))
         }
     }
+
+    func testRejectsInvalidCharactersInHeaderFieldNames_http1() {
+        self._rejectsInvalidCharactersInHeaderFieldNames(mode: .http1_1(ssl: true))
+    }
+
+    func testRejectsInvalidCharactersInHeaderFieldNames_http2() {
+        self._rejectsInvalidCharactersInHeaderFieldNames(mode: .http2(compress: false))
+    }
+
+    private func _rejectsInvalidCharactersInHeaderFieldNames(mode: HTTPBin<HTTPBinHandler>.Mode) {
+        guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
+        XCTAsyncTest {
+            let bin = HTTPBin(mode)
+            defer { XCTAssertNoThrow(try bin.shutdown()) }
+            let client = makeDefaultHTTPClient()
+            defer { XCTAssertNoThrow(try client.syncShutdown()) }
+            let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+
+            // The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid
+            // characters as the following:
+            //
+            // ```
+            // field-name     = token
+            //
+            // token          = 1*tchar
+            //
+            // tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
+            //                / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
+            //                / DIGIT / ALPHA
+            //                ; any VCHAR, except delimiters
+            let weirdAllowedFieldName = "!#$%&'*+-.^_`|~0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
+
+            var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/get")
+            request.headers.add(name: weirdAllowedFieldName, value: "present")
+
+            // This should work fine.
+            guard let response = await XCTAssertNoThrowWithResult(
+                try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)
+            ) else {
+                return
+            }
+
+            XCTAssertEqual(response.status, .ok)
+
+            // Now, let's confirm all other bytes are rejected. We want to stay within the ASCII space as the HTTPHeaders type will forbid anything else.
+            for byte in UInt8(0)...UInt8(127) {
+                // Skip bytes that we already believe are allowed.
+                if weirdAllowedFieldName.utf8.contains(byte) {
+                    continue
+                }
+                let forbiddenFieldName = weirdAllowedFieldName + String(decoding: [byte], as: UTF8.self)
+
+                var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/get")
+                request.headers.add(name: forbiddenFieldName, value: "present")
+
+                await XCTAssertThrowsError(try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)) { error in
+                    XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldNames([forbiddenFieldName]))
+                }
+            }
+        }
+    }
+
+    func testRejectsInvalidCharactersInHeaderFieldValues_http1() {
+        self._rejectsInvalidCharactersInHeaderFieldValues(mode: .http1_1(ssl: true))
+    }
+
+    func testRejectsInvalidCharactersInHeaderFieldValues_http2() {
+        self._rejectsInvalidCharactersInHeaderFieldValues(mode: .http2(compress: false))
+    }
+
+    private func _rejectsInvalidCharactersInHeaderFieldValues(mode: HTTPBin<HTTPBinHandler>.Mode) {
+        guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
+        XCTAsyncTest {
+            let bin = HTTPBin(mode)
+            defer { XCTAssertNoThrow(try bin.shutdown()) }
+            let client = makeDefaultHTTPClient()
+            defer { XCTAssertNoThrow(try client.syncShutdown()) }
+            let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+
+            // We reject all ASCII control characters except HTAB and tolerate everything else.
+            let weirdAllowedFieldValue = "!\" \t#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"
+
+            var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/get")
+            request.headers.add(name: "Weird-Value", value: weirdAllowedFieldValue)
+
+            // This should work fine.
+            guard let response = await XCTAssertNoThrowWithResult(
+                try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)
+            ) else {
+                return
+            }
+
+            XCTAssertEqual(response.status, .ok)
+
+            // Now, let's confirm all other bytes in the ASCII range ar rejected
+            for byte in UInt8(0)...UInt8(127) {
+                // Skip bytes that we already believe are allowed.
+                if weirdAllowedFieldValue.utf8.contains(byte) {
+                    continue
+                }
+                let forbiddenFieldValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self)
+
+                var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/get")
+                request.headers.add(name: "Weird-Value", value: forbiddenFieldValue)
+
+                await XCTAssertThrowsError(try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)) { error in
+                    XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldValues([forbiddenFieldValue]))
+                }
+            }
+
+            // All the bytes outside the ASCII range are fine though.
+            for byte in UInt8(128)...UInt8(255) {
+                let evenWeirderAllowedValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self)
+
+                var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/get")
+                request.headers.add(name: "Weird-Value", value: evenWeirderAllowedValue)
+
+                // This should work fine.
+                guard let response = await XCTAssertNoThrowWithResult(
+                    try await client.execute(request, deadline: .now() + .seconds(10), logger: logger)
+                ) else {
+                    return
+                }
+
+                XCTAssertEqual(response.status, .ok)
+            }
+        }
+    }
 }
 
 extension AsyncSequence where Element == ByteBuffer {

Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,10 @@ extension AsyncAwaitEndToEndTests {`
`45`	`45`	`("testShutdown", testShutdown),`
`46`	`46`	`("testCancelingBodyDoesNotCrash", testCancelingBodyDoesNotCrash),`
`47`	`47`	`("testAsyncSequenceReuse", testAsyncSequenceReuse),`
	`48`	`+ ("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1),`
	`49`	`+ ("testRejectsInvalidCharactersInHeaderFieldNames_http2", testRejectsInvalidCharactersInHeaderFieldNames_http2),`
	`50`	`+ ("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1),`
	`51`	`+ ("testRejectsInvalidCharactersInHeaderFieldValues_http2", testRejectsInvalidCharactersInHeaderFieldValues_http2),`
`48`	`52`	`]`
`49`	`53`	`}`
`50`	`54`	`}`