diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 51a1c8c4d..6ed1ed519 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -989,6 +989,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case uncleanShutdown case traceRequestWithBody case invalidHeaderFieldNames([String]) + case invalidHeaderFieldValues([String]) case bodyLengthMismatch case writeAfterRequestSent case incompatibleHeaders @@ -1042,6 +1043,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 index a9c1678f4..8a8b2a90f 100644 --- a/Sources/AsyncHTTPClient/RequestValidation.swift +++ b/Sources/AsyncHTTPClient/RequestValidation.swift @@ -21,19 +21,20 @@ extension HTTPHeaders { if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") { throw HTTPClientError.incompatibleHeaders } - + var transferEncoding: String? var contentLength: Int? let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() } - + guard !encodings.contains("identity") else { throw HTTPClientError.identityCodingIncorrectlyPresent } - + self.remove(name: "Transfer-Encoding") - + try self.validateFieldNames() - + try self.validateFieldValues() + guard let body = body else { self.remove(name: "Content-Length") // if we don't have a body we might not need to send the Content-Length field @@ -52,17 +53,17 @@ extension HTTPHeaders { return } } - + if case .TRACE = method { // A client MUST NOT send a message body in a TRACE request. // https://tools.ietf.org/html/rfc7230#section-4.3.8 throw HTTPClientError.traceRequestWithBody } - + guard (encodings.filter { $0 == "chunked" }.count <= 1) else { throw HTTPClientError.chunkedSpecifiedMultipleTimes } - + if encodings.isEmpty { if let length = body.length { self.remove(name: "Content-Length") @@ -72,7 +73,7 @@ extension HTTPHeaders { } } else { self.remove(name: "Content-Length") - + transferEncoding = encodings.joined(separator: ", ") if !encodings.contains("chunked") { guard let length = body.length else { @@ -81,7 +82,7 @@ extension HTTPHeaders { contentLength = length } } - + // add headers if required if let enc = transferEncoding { self.add(name: "Transfer-Encoding", value: enc) @@ -91,40 +92,90 @@ extension HTTPHeaders { self.add(name: "Content-Length", value: String(length)) } } - + func validateFieldNames() throws { let invalidFieldNames = self.compactMap { (name, _) -> String? in let satisfy = name.utf8.allSatisfy { (char) -> Bool in switch char { case UInt8(ascii: "a")...UInt8(ascii: "z"), - UInt8(ascii: "A")...UInt8(ascii: "Z"), - UInt8(ascii: "0")...UInt8(ascii: "9"), - UInt8(ascii: "!"), - UInt8(ascii: "#"), - UInt8(ascii: "$"), - UInt8(ascii: "%"), - UInt8(ascii: "&"), - UInt8(ascii: "'"), - UInt8(ascii: "*"), - UInt8(ascii: "+"), - UInt8(ascii: "-"), - UInt8(ascii: "."), - UInt8(ascii: "^"), - UInt8(ascii: "_"), - UInt8(ascii: "`"), - UInt8(ascii: "|"), - UInt8(ascii: "~"): + UInt8(ascii: "A")...UInt8(ascii: "Z"), + UInt8(ascii: "0")...UInt8(ascii: "9"), + UInt8(ascii: "!"), + UInt8(ascii: "#"), + UInt8(ascii: "$"), + UInt8(ascii: "%"), + UInt8(ascii: "&"), + UInt8(ascii: "'"), + UInt8(ascii: "*"), + UInt8(ascii: "+"), + UInt8(ascii: "-"), + UInt8(ascii: "."), + UInt8(ascii: "^"), + UInt8(ascii: "_"), + UInt8(ascii: "`"), + UInt8(ascii: "|"), + UInt8(ascii: "~"): return true default: return false } } - + return satisfy ? nil : name } - + guard invalidFieldNames.count == 0 else { throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames) } } + + 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) + } + } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 98bfb0b54..6a58228a1 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -135,6 +135,8 @@ extension HTTPClientTests { ("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine), ("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted), ("testRequestSpecificTLS", testRequestSpecificTLS), + ("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1), + ("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index eef14a78a..90c57a4a7 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2977,4 +2977,97 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(firstConnectionNumber, secondConnectionNumber, "Identical TLS configurations did not use the same connection") XCTAssertNotEqual(thirdConnectionNumber, firstConnectionNumber, "Different TLS configurations did not use different connections.") } + + func testRejectsInvalidCharactersInHeaderFieldNames_http1() throws { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } + let client = HTTPClient(eventLoopGroupProvider: .shared(group)) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let bin = HTTPBin() + defer { XCTAssertNoThrow(try bin.shutdown()) } + + // 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 = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: weirdAllowedFieldName, value: "present") + + // This should work fine. + let response = try client.execute(request: request).wait() + 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 = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: forbiddenFieldName, value: "present") + + XCTAssertThrowsError(try client.execute(request: request).wait()) { error in + XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldNames([forbiddenFieldName])) + } + } + } + + func testRejectsInvalidCharactersInHeaderFieldValues_http1() throws { + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } + let client = HTTPClient(eventLoopGroupProvider: .shared(group)) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let bin = HTTPBin() + defer { XCTAssertNoThrow(try bin.shutdown()) } + + // We reject all ASCII control characters except HTAB and tolerate everything else. + let weirdAllowedFieldValue = "!\" \t#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" + + var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: weirdAllowedFieldValue) + + // This should work fine. + let response = try client.execute(request: request).wait() + 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 = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: forbiddenFieldValue) + + XCTAssertThrowsError(try client.execute(request: request).wait()) { 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 = try Request(url: "\(self.defaultHTTPBinURLPrefix)get") + request.headers.add(name: "Weird-Value", value: evenWeirderAllowedValue) + + // This should work fine. + let response = try client.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + } + } }