Skip to content

Commit

Permalink
Merge pull request from GHSA-v3r5-pjpm-mwgq
Browse files Browse the repository at this point in the history 
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

(cherry picked from commit 3034835a213babfcda19031e80c0b7c9780475e9)
  • Loading branch information
@Lukasa
Lukasa authored Jan 17, 2023
1 parent 3fd0658 commit 1680b76
Show file tree
Hide file tree
Showing 4 changed files with 179 additions and 30 deletions.
3 changes: 3 additions & 0 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
111 changes: 81 additions & 30 deletions Sources/AsyncHTTPClient/RequestValidation.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand All @@ -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 {
Expand All @@ -81,7 +82,7 @@ extension HTTPHeaders {
contentLength = length
}
}

// add headers if required
if let enc = transferEncoding {
self.add(name: "Transfer-Encoding", value: enc)
Expand All @@ -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)
}
}
}
2 changes: 2 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ extension HTTPClientTests {
("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine),
("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
("testRequestSpecificTLS", testRequestSpecificTLS),
("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1),
("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1),
]
}
}
93 changes: 93 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}

0 comments on commit 1680b76

Please sign in to comment.