Skip to content

Commit 1680b76

Browse files
LukasaLukasa
authored
Merge pull request from GHSA-v3r5-pjpm-mwgq
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)
1 parent 3fd0658 commit 1680b76

File tree

4 files changed

+179
-30
lines changed

4 files changed

+179
-30
lines changed

‎Sources/AsyncHTTPClient/HTTPClient.swift‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
989989
case uncleanShutdown
990990
case traceRequestWithBody
991991
case invalidHeaderFieldNames([String])
992+
case invalidHeaderFieldValues([String])
992993
case bodyLengthMismatch
993994
case writeAfterRequestSent
994995
case incompatibleHeaders
@@ -1042,6 +1043,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
10421043
public static let traceRequestWithBody = HTTPClientError(code: .traceRequestWithBody)
10431044
/// Header field names contain invalid characters.
10441045
public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) }
1046+
/// Header field values contain invalid characters.
1047+
public static func invalidHeaderFieldValues(_ values: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldValues(values)) }
10451048
/// Body length is not equal to `Content-Length`.
10461049
public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch)
10471050
/// Body part was written after request was fully sent.

‎Sources/AsyncHTTPClient/RequestValidation.swift‎

Lines changed: 81 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,20 @@ extension HTTPHeaders {
2121
if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") {
2222
throw HTTPClientError.incompatibleHeaders
2323
}
24-
24+
2525
var transferEncoding: String?
2626
var contentLength: Int?
2727
let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() }
28-
28+
2929
guard !encodings.contains("identity") else {
3030
throw HTTPClientError.identityCodingIncorrectlyPresent
3131
}
32-
32+
3333
self.remove(name: "Transfer-Encoding")
34-
34+
3535
try self.validateFieldNames()
36-
36+
try self.validateFieldValues()
37+
3738
guard let body = body else {
3839
self.remove(name: "Content-Length")
3940
// if we don't have a body we might not need to send the Content-Length field
@@ -52,17 +53,17 @@ extension HTTPHeaders {
5253
return
5354
}
5455
}
55-
56+
5657
if case .TRACE = method {
5758
// A client MUST NOT send a message body in a TRACE request.
5859
// https://tools.ietf.org/html/rfc7230#section-4.3.8
5960
throw HTTPClientError.traceRequestWithBody
6061
}
61-
62+
6263
guard (encodings.filter { $0 == "chunked" }.count <= 1) else {
6364
throw HTTPClientError.chunkedSpecifiedMultipleTimes
6465
}
65-
66+
6667
if encodings.isEmpty {
6768
if let length = body.length {
6869
self.remove(name: "Content-Length")
@@ -72,7 +73,7 @@ extension HTTPHeaders {
7273
}
7374
} else {
7475
self.remove(name: "Content-Length")
75-
76+
7677
transferEncoding = encodings.joined(separator: ", ")
7778
if !encodings.contains("chunked") {
7879
guard let length = body.length else {
@@ -81,7 +82,7 @@ extension HTTPHeaders {
8182
contentLength = length
8283
}
8384
}
84-
85+
8586
// add headers if required
8687
if let enc = transferEncoding {
8788
self.add(name: "Transfer-Encoding", value: enc)
@@ -91,40 +92,90 @@ extension HTTPHeaders {
9192
self.add(name: "Content-Length", value: String(length))
9293
}
9394
}
94-
95+
9596
func validateFieldNames() throws {
9697
let invalidFieldNames = self.compactMap { (name, _) -> String? in
9798
let satisfy = name.utf8.allSatisfy { (char) -> Bool in
9899
switch char {
99100
case UInt8(ascii: "a")...UInt8(ascii: "z"),
100-
UInt8(ascii: "A")...UInt8(ascii: "Z"),
101-
UInt8(ascii: "0")...UInt8(ascii: "9"),
102-
UInt8(ascii: "!"),
103-
UInt8(ascii: "#"),
104-
UInt8(ascii: "$"),
105-
UInt8(ascii: "%"),
106-
UInt8(ascii: "&"),
107-
UInt8(ascii: "'"),
108-
UInt8(ascii: "*"),
109-
UInt8(ascii: "+"),
110-
UInt8(ascii: "-"),
111-
UInt8(ascii: "."),
112-
UInt8(ascii: "^"),
113-
UInt8(ascii: "_"),
114-
UInt8(ascii: "`"),
115-
UInt8(ascii: "|"),
116-
UInt8(ascii: "~"):
101+
UInt8(ascii: "A")...UInt8(ascii: "Z"),
102+
UInt8(ascii: "0")...UInt8(ascii: "9"),
103+
UInt8(ascii: "!"),
104+
UInt8(ascii: "#"),
105+
UInt8(ascii: "$"),
106+
UInt8(ascii: "%"),
107+
UInt8(ascii: "&"),
108+
UInt8(ascii: "'"),
109+
UInt8(ascii: "*"),
110+
UInt8(ascii: "+"),
111+
UInt8(ascii: "-"),
112+
UInt8(ascii: "."),
113+
UInt8(ascii: "^"),
114+
UInt8(ascii: "_"),
115+
UInt8(ascii: "`"),
116+
UInt8(ascii: "|"),
117+
UInt8(ascii: "~"):
117118
return true
118119
default:
119120
return false
120121
}
121122
}
122-
123+
123124
return satisfy ? nil : name
124125
}
125-
126+
126127
guard invalidFieldNames.count == 0 else {
127128
throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames)
128129
}
129130
}
131+
132+
private func validateFieldValues() throws {
133+
let invalidValues = self.compactMap { _, value -> String? in
134+
let satisfy = value.utf8.allSatisfy { char -> Bool in
135+
/// Validates a byte of a given header field value against the definition in RFC 9110.
136+
///
137+
/// The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid
138+
/// characters as the following:
139+
///
140+
/// ```
141+
/// field-value = *field-content
142+
/// field-content = field-vchar
143+
/// [ 1*( SP / HTAB / field-vchar ) field-vchar ]
144+
/// field-vchar = VCHAR / obs-text
145+
/// obs-text = %x80-FF
146+
/// ```
147+
///
148+
/// Additionally, it makes the following note:
149+
///
150+
/// "Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the
151+
/// varying ways that implementations might parse and interpret those characters; a recipient
152+
/// of CR, LF, or NUL within a field value MUST either reject the message or replace each of
153+
/// those characters with SP before further processing or forwarding of that message. Field
154+
/// values containing other CTL characters are also invalid; however, recipients MAY retain
155+
/// such characters for the sake of robustness when they appear within a safe context (e.g.,
156+
/// an application-specific quoted string that will not be processed by any downstream HTTP
157+
/// parser)."
158+
///
159+
/// As we cannot guarantee the context is safe, this code will reject all ASCII control characters
160+
/// directly _except_ for HTAB, which is explicitly allowed.
161+
switch char {
162+
case UInt8(ascii: "\t"):
163+
// HTAB, explicitly allowed.
164+
return true
165+
case 0...0x1f, 0x7F:
166+
// ASCII control character, forbidden.
167+
return false
168+
default:
169+
// Printable or non-ASCII, allowed.
170+
return true
171+
}
172+
}
173+
174+
return satisfy ? nil : value
175+
}
176+
177+
guard invalidValues.count == 0 else {
178+
throw HTTPClientError.invalidHeaderFieldValues(invalidValues)
179+
}
180+
}
130181
}

‎Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,8 @@ extension HTTPClientTests {
135135
("testCloseWhileBackpressureIsExertedIsFine", testCloseWhileBackpressureIsExertedIsFine),
136136
("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
137137
("testRequestSpecificTLS", testRequestSpecificTLS),
138+
("testRejectsInvalidCharactersInHeaderFieldNames_http1", testRejectsInvalidCharactersInHeaderFieldNames_http1),
139+
("testRejectsInvalidCharactersInHeaderFieldValues_http1", testRejectsInvalidCharactersInHeaderFieldValues_http1),
138140
]
139141
}
140142
}

‎Tests/AsyncHTTPClientTests/HTTPClientTests.swift‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2977,4 +2977,97 @@ class HTTPClientTests: XCTestCase {
29772977
XCTAssertEqual(firstConnectionNumber, secondConnectionNumber, "Identical TLS configurations did not use the same connection")
29782978
XCTAssertNotEqual(thirdConnectionNumber, firstConnectionNumber, "Different TLS configurations did not use different connections.")
29792979
}
2980+
2981+
func testRejectsInvalidCharactersInHeaderFieldNames_http1() throws {
2982+
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
2983+
defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }
2984+
let client = HTTPClient(eventLoopGroupProvider: .shared(group))
2985+
defer { XCTAssertNoThrow(try client.syncShutdown()) }
2986+
let bin = HTTPBin()
2987+
defer { XCTAssertNoThrow(try bin.shutdown()) }
2988+
2989+
// The spec in [RFC 9110](https://httpwg.org/specs/rfc9110.html#fields.values) defines the valid
2990+
// characters as the following:
2991+
//
2992+
// ```
2993+
// field-name = token
2994+
//
2995+
// token = 1*tchar
2996+
//
2997+
// tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*"
2998+
// / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
2999+
// / DIGIT / ALPHA
3000+
// ; any VCHAR, except delimiters
3001+
let weirdAllowedFieldName = "!#$%&'*+-.^_`|~0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
3002+
3003+
var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get")
3004+
request.headers.add(name: weirdAllowedFieldName, value: "present")
3005+
3006+
// This should work fine.
3007+
let response = try client.execute(request: request).wait()
3008+
XCTAssertEqual(response.status, .ok)
3009+
3010+
// 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.
3011+
for byte in UInt8(0)...UInt8(127) {
3012+
// Skip bytes that we already believe are allowed.
3013+
if weirdAllowedFieldName.utf8.contains(byte) {
3014+
continue
3015+
}
3016+
let forbiddenFieldName = weirdAllowedFieldName + String(decoding: [byte], as: UTF8.self)
3017+
3018+
var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get")
3019+
request.headers.add(name: forbiddenFieldName, value: "present")
3020+
3021+
XCTAssertThrowsError(try client.execute(request: request).wait()) { error in
3022+
XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldNames([forbiddenFieldName]))
3023+
}
3024+
}
3025+
}
3026+
3027+
func testRejectsInvalidCharactersInHeaderFieldValues_http1() throws {
3028+
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
3029+
defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }
3030+
let client = HTTPClient(eventLoopGroupProvider: .shared(group))
3031+
defer { XCTAssertNoThrow(try client.syncShutdown()) }
3032+
let bin = HTTPBin()
3033+
defer { XCTAssertNoThrow(try bin.shutdown()) }
3034+
3035+
// We reject all ASCII control characters except HTAB and tolerate everything else.
3036+
let weirdAllowedFieldValue = "!\" \t#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"
3037+
3038+
var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get")
3039+
request.headers.add(name: "Weird-Value", value: weirdAllowedFieldValue)
3040+
3041+
// This should work fine.
3042+
let response = try client.execute(request: request).wait()
3043+
XCTAssertEqual(response.status, .ok)
3044+
3045+
// Now, let's confirm all other bytes in the ASCII range ar rejected
3046+
for byte in UInt8(0)...UInt8(127) {
3047+
// Skip bytes that we already believe are allowed.
3048+
if weirdAllowedFieldValue.utf8.contains(byte) {
3049+
continue
3050+
}
3051+
let forbiddenFieldValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self)
3052+
3053+
var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get")
3054+
request.headers.add(name: "Weird-Value", value: forbiddenFieldValue)
3055+
3056+
XCTAssertThrowsError(try client.execute(request: request).wait()) { error in
3057+
XCTAssertEqual(error as? HTTPClientError, .invalidHeaderFieldValues([forbiddenFieldValue]))
3058+
}
3059+
}
3060+
3061+
// All the bytes outside the ASCII range are fine though.
3062+
for byte in UInt8(128)...UInt8(255) {
3063+
let evenWeirderAllowedValue = weirdAllowedFieldValue + String(decoding: [byte], as: UTF8.self)
3064+
3065+
var request = try Request(url: "\(self.defaultHTTPBinURLPrefix)get")
3066+
request.headers.add(name: "Weird-Value", value: evenWeirderAllowedValue)
3067+
3068+
// This should work fine.
3069+
let response = try client.execute(request: request).wait()
3070+
XCTAssertEqual(response.status, .ok)
3071+
}
3072+
}
29803073
}

0 commit comments

Comments
 (0)