Skip to content

Commit 000ca94

Browse files
glbrnttglbrntt
authored
Merge pull request from GHSA-pgfx-g6rc-8cjv
Motivation: We don't currently support ALTSVC and ORIGIN frames. At the moment when receiving or attempting to write these frames we trap. Trapping when receiving an unsupported frame can lead to DoS attacks. Moreover these frames are non critical so can safely be ignored. Modifications: - Forward inbound and outbound ALTSVC and ORIGIN frames to the connection state machine - The connection state machine now ignores inbound frames of this type - The connection state machine traps on outbound frames of this type (as per the current behaviour) - Document this behaviour on `HTTP2Frame.FramePayload` - Tests and fuzz testing failure case Result: We don't trap when receiving unsupported frames.
1 parent 56c60a2 commit 000ca94

9 files changed

+129
-7
lines changed

‎FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-6318975863095296

252 Bytes
Binary file not shown.

‎Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1441,6 +1441,45 @@ extension HTTP2ConnectionStateMachine {
14411441
preconditionFailure("Must not be left in modifying state")
14421442
}
14431443
}
1444+
1445+
/// Called when an ALTSVC frame has been received.
1446+
///
1447+
/// At present the frame is unconditionally ignored.
1448+
mutating func receiveAlternativeService(origin: String?, field: ByteBuffer?) -> StateMachineResultWithEffect {
1449+
// We don't support ALTSVC frames right now so we just ignore them.
1450+
//
1451+
// From RFC 7838 § 4:
1452+
// > The ALTSVC frame is a non-critical extension to HTTP/2. Endpoints
1453+
// > that do not support this frame will ignore it (as per the
1454+
// > extensibility rules defined in Section 4.1 of RFC7540).
1455+
return .init(result: .ignoreFrame, effect: .none)
1456+
}
1457+
1458+
/// Called when an ALTSVC frame is sent.
1459+
///
1460+
/// At present the frame is not handled, calling this function will trap.
1461+
mutating func sendAlternativeService(origin: String?, field: ByteBuffer?) -> Never {
1462+
fatalError("Currently ALTSVC frames are unhandled.")
1463+
}
1464+
1465+
/// Called when an ORIGIN frame has been received.
1466+
///
1467+
/// At present the frame is unconditionally ignored.
1468+
mutating func receiveOrigin(origins: [String]) -> StateMachineResultWithEffect {
1469+
// We don't support ORIGIN frames right now so we just ignore them.
1470+
//
1471+
// From RFC 8336 § 2.1:
1472+
// > The ORIGIN frame is a non-critical extension to HTTP/2. Endpoints
1473+
// > that do not support this frame can safely ignore it upon receipt.
1474+
return .init(result: .ignoreFrame, effect: .none)
1475+
}
1476+
1477+
/// Called when an ORIGIN frame is sent.
1478+
///
1479+
/// At present the frame is not handled, calling this function will trap.
1480+
mutating func sendOrigin(origins: [String]) -> Never {
1481+
fatalError("Currently ORIGIN frames are unhandled.")
1482+
}
14441483
}
14451484

14461485
// Mark:- Private helper methods

‎Sources/NIOHTTP2/HTTP2ChannelHandler.swift

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -233,9 +233,10 @@ extension NIOHTTP2Handler {
233233
var result: StateMachineResultWithEffect
234234

235235
switch frame.payload {
236-
case .alternativeService, .origin:
237-
// TODO(cory): Implement
238-
fatalError("Currently some frames are unhandled.")
236+
case .alternativeService(let origin, let field):
237+
result = self.stateMachine.receiveAlternativeService(origin: origin, field: field)
238+
case .origin(let origins):
239+
result = self.stateMachine.receiveOrigin(origins: origins)
239240
case .data(let dataBody):
240241
result = self.stateMachine.receiveData(streamID: frame.streamID, contentLength: dataBody.data.readableBytes, flowControlledBytes: flowControlledLength, isEndStreamSet: dataBody.endStream)
241242
case .goAway(let lastStreamID, _, _):
@@ -422,9 +423,12 @@ extension NIOHTTP2Handler {
422423
let result: StateMachineResultWithEffect
423424

424425
switch frame.payload {
425-
case .alternativeService, .origin:
426-
// TODO(cory): Implement
427-
fatalError("Currently some frames are unhandled.")
426+
case .alternativeService(let origin, let field):
427+
// Returns 'Never'; alt service frames are not currently handled.
428+
self.stateMachine.sendAlternativeService(origin: origin, field: field)
429+
case .origin(let origins):
430+
// Returns 'Never'; origin frames are not currently handled.
431+
self.stateMachine.sendOrigin(origins: origins)
428432
case .data(let data):
429433
// TODO(cory): Correctly account for padding data.
430434
result = self.stateMachine.sendData(streamID: frame.streamID, contentLength: data.data.readableBytes, flowControlledBytes: data.data.readableBytes, isEndStreamSet: data.endStream)

‎Sources/NIOHTTP2/HTTP2Frame.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,19 @@ public struct HTTP2Frame {
107107
/// the locations at which they may be addressed.
108108
///
109109
/// See [RFC 7838 § 4](https://tools.ietf.org/html/rfc7838#section-4).
110+
///
111+
/// - Important: ALTSVC frames are not currently supported. Any received ALTSVC frames will
112+
/// be ignored. Attempting to send an ALTSVC frame will result in a fatal error.
110113
indirect case alternativeService(origin: String?, field: ByteBuffer?)
111114

112115
/// An ORIGIN frame. This allows servers which allow access to multiple origins
113116
/// via the same socket connection to identify which origins may be accessed in
114117
/// this manner.
115118
///
116119
/// See [RFC 8336 § 2](https://tools.ietf.org/html/rfc8336#section-2).
120+
///
121+
/// - Important: ORIGIN frames are not currently supported. Any received ORIGIN frames will
122+
/// be ignored. Attempting to send an ORIGIN frame will result in a fatal error.
117123
case origin([String])
118124

119125
/// The payload of a DATA frame.

‎Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,8 @@ extension ConnectionStateMachineTests {
142142
("testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled),
143143
("testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled),
144144
("testWeTolerateOneStreamBeingResetTwice", testWeTolerateOneStreamBeingResetTwice),
145+
("testReceivedAltServiceFramesAreIgnored", testReceivedAltServiceFramesAreIgnored),
146+
("testReceivedOriginFramesAreIgnored", testReceivedOriginFramesAreIgnored),
145147
]
146148
}
147149
}

‎Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2974,6 +2974,18 @@ class ConnectionStateMachineTests: XCTestCase {
29742974
assertSucceeds(self.server.receiveRstStream(streamID: streamOne, reason: .cancel))
29752975
assertIgnored(self.server.receiveRstStream(streamID: streamOne, reason: .streamClosed))
29762976
}
2977+
2978+
func testReceivedAltServiceFramesAreIgnored() {
2979+
self.exchangePreamble()
2980+
assertIgnored(self.client.receiveAlternativeService(origin: "over-there", field: nil))
2981+
assertIgnored(self.server.receiveAlternativeService(origin: "over-there", field: nil))
2982+
}
2983+
2984+
func testReceivedOriginFramesAreIgnored() {
2985+
self.exchangePreamble()
2986+
assertIgnored(self.client.receiveOrigin(origins: ["one", "two"]))
2987+
assertIgnored(self.server.receiveOrigin(origins: ["one", "two"]))
2988+
}
29772989
}
29782990

29792991

‎Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1806,7 +1806,7 @@ class HTTP2FrameParserTests: XCTestCase {
18061806

18071807
let frameBytes: [UInt8] = [
18081808
0x00, 0x00, 0x2a, // 3-byte payload length (42 bytes)
1809-
0x0c, // 1-byte frame type (ALTSVC)
1809+
0x0c, // 1-byte frame type (ORIGIN)
18101810
0x00, // 1-byte flags (none)
18111811
0x00, 0x00, 0x00, 0x00, // 4-byte stream identifier,
18121812
]

‎Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ extension SimpleClientServerFramePayloadStreamTests {
7676
("testStreamCreationOrder", testStreamCreationOrder),
7777
("testStreamClosedInvalidRequestHeaders", testStreamClosedInvalidRequestHeaders),
7878
("testHTTP2HandlerDoesNotFlushExcessively", testHTTP2HandlerDoesNotFlushExcessively),
79+
("testHTTPHandlerIgnoresInboundAltServiceFrames", testHTTPHandlerIgnoresInboundAltServiceFrames),
80+
("testHTTPHandlerIgnoresInboundOriginFrames", testHTTPHandlerIgnoresInboundOriginFrames),
7981
]
8082
}
8183
}

‎Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1982,4 +1982,61 @@ class SimpleClientServerFramePayloadStreamTests: XCTestCase {
19821982
self.clientChannel.pipeline.fireChannelReadComplete()
19831983
XCTAssertEqual(counter.flushCount, 0)
19841984
}
1985+
1986+
func testHTTPHandlerIgnoresInboundAltServiceFrames() throws {
1987+
try self.basicHTTP2Connection()
1988+
1989+
let h2ClientHandler = try self.clientChannel.pipeline.syncOperations.handler(type: NIOHTTP2Handler.self)
1990+
let h2FrameRecorder = FrameRecorderHandler()
1991+
try self.clientChannel.pipeline.syncOperations.addHandler(h2FrameRecorder, position: .after(h2ClientHandler))
1992+
1993+
let altServiceFrameBytes: [UInt8] = [
1994+
0x00, 0x00, 0x15, // 3-byte payload length (21 bytes)
1995+
0x0a, // 1-byte frame type (ALTSVC)
1996+
0x00, // 1-byte flags (none)
1997+
0x00, 0x00, 0x00, 0x00, // 4-byte stream identifier
1998+
0x00, 0x09, // 2-byte origin size ("apple.com"; 9 bytes)
1999+
]
2000+
2001+
var buffer = self.clientChannel.allocator.buffer(bytes: altServiceFrameBytes)
2002+
buffer.writeString("apple.com")
2003+
buffer.writeString("h2=\":8000\"")
2004+
XCTAssertEqual(buffer.readableBytes, 30) // 9 (header) + 21 (origin, field)
2005+
XCTAssertNoThrow(try self.clientChannel.writeInbound(buffer))
2006+
2007+
// Frame should be dropped.
2008+
XCTAssert(h2FrameRecorder.receivedFrames.isEmpty)
2009+
}
2010+
2011+
func testHTTPHandlerIgnoresInboundOriginFrames() throws {
2012+
try self.basicHTTP2Connection()
2013+
2014+
let h2ClientHandler = try self.clientChannel.pipeline.syncOperations.handler(type: NIOHTTP2Handler.self)
2015+
let h2FrameRecorder = FrameRecorderHandler()
2016+
try self.clientChannel.pipeline.syncOperations.addHandler(h2FrameRecorder, position: .after(h2ClientHandler))
2017+
2018+
let originFrameBytes: [UInt8] = [
2019+
0x00, 0x00, 0x2a, // 3-byte payload length (42 bytes)
2020+
0x0c, // 1-byte frame type (ORIGIN)
2021+
0x00, // 1-byte flags (none)
2022+
0x00, 0x00, 0x00, 0x00 // 4-byte stream identifier,
2023+
]
2024+
2025+
var buffer = self.clientChannel.allocator.buffer(bytes: originFrameBytes)
2026+
var originBytesWritten = 0
2027+
2028+
for origin in ["apple.com", "www.apple.com", "www2.apple.com"] {
2029+
originBytesWritten += try buffer.writeLengthPrefixed(as: UInt16.self) {
2030+
$0.writeString(origin)
2031+
}
2032+
}
2033+
2034+
XCTAssertEqual(originBytesWritten, 42)
2035+
XCTAssertEqual(buffer.readableBytes, 51)
2036+
2037+
XCTAssertNoThrow(try self.clientChannel.writeInbound(buffer))
2038+
2039+
// Frame should be dropped.
2040+
XCTAssert(h2FrameRecorder.receivedFrames.isEmpty)
2041+
}
19852042
}

0 commit comments

Comments
 (0)