From 000ca94f9de92c95b9ac85d44600b7b0fe25a3e5 Mon Sep 17 00:00:00 2001 From: George Barnett Date: Fri, 11 Feb 2022 09:19:11 +0000 Subject: [PATCH] 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. --- ...ized-ServerFuzzer-release-6318975863095296 | Bin 0 -> 252 bytes .../ConnectionStateMachine.swift | 39 ++++++++++++ Sources/NIOHTTP2/HTTP2ChannelHandler.swift | 16 +++-- Sources/NIOHTTP2/HTTP2Frame.swift | 6 ++ .../ConnectionStateMachineTests+XCTest.swift | 2 + .../ConnectionStateMachineTests.swift | 12 ++++ .../NIOHTTP2Tests/HTTP2FrameParserTests.swift | 2 +- ...ServerFramePayloadStreamTests+XCTest.swift | 2 + ...eClientServerFramePayloadStreamTests.swift | 57 ++++++++++++++++++ 9 files changed, 129 insertions(+), 7 deletions(-) create mode 100644 FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-6318975863095296 diff --git a/FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-6318975863095296 b/FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-6318975863095296 new file mode 100644 index 0000000000000000000000000000000000000000..262d9727b28c5db02b366797ea4b1c2d128e3a2a GIT binary patch literal 252 zcmWFt@>I}L@CXSB&^OXE;N{}w3ibt&3=FJXj0_A68Vn2^?9+^aEUE!x7Lb{Y|Nk>- zXXfoP0@?x**rlD9$*2yu21S)3T-AT=|CvmAyA;2{v_Z{*s7TJwOHD2T;mrIzpemTQ TB&c;vAmbSs^cnxRg#-Wq&p;&T literal 0 HcmV?d00001 diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift index ae097e07..d5438866 100644 --- a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift +++ b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift @@ -1441,6 +1441,45 @@ extension HTTP2ConnectionStateMachine { preconditionFailure("Must not be left in modifying state") } } + + /// Called when an ALTSVC frame has been received. + /// + /// At present the frame is unconditionally ignored. + mutating func receiveAlternativeService(origin: String?, field: ByteBuffer?) -> StateMachineResultWithEffect { + // We don't support ALTSVC frames right now so we just ignore them. + // + // From RFC 7838 § 4: + // > The ALTSVC frame is a non-critical extension to HTTP/2. Endpoints + // > that do not support this frame will ignore it (as per the + // > extensibility rules defined in Section 4.1 of RFC7540). + return .init(result: .ignoreFrame, effect: .none) + } + + /// Called when an ALTSVC frame is sent. + /// + /// At present the frame is not handled, calling this function will trap. + mutating func sendAlternativeService(origin: String?, field: ByteBuffer?) -> Never { + fatalError("Currently ALTSVC frames are unhandled.") + } + + /// Called when an ORIGIN frame has been received. + /// + /// At present the frame is unconditionally ignored. + mutating func receiveOrigin(origins: [String]) -> StateMachineResultWithEffect { + // We don't support ORIGIN frames right now so we just ignore them. + // + // From RFC 8336 § 2.1: + // > The ORIGIN frame is a non-critical extension to HTTP/2. Endpoints + // > that do not support this frame can safely ignore it upon receipt. + return .init(result: .ignoreFrame, effect: .none) + } + + /// Called when an ORIGIN frame is sent. + /// + /// At present the frame is not handled, calling this function will trap. + mutating func sendOrigin(origins: [String]) -> Never { + fatalError("Currently ORIGIN frames are unhandled.") + } } // Mark:- Private helper methods diff --git a/Sources/NIOHTTP2/HTTP2ChannelHandler.swift b/Sources/NIOHTTP2/HTTP2ChannelHandler.swift index 7649ba2c..455a35cd 100644 --- a/Sources/NIOHTTP2/HTTP2ChannelHandler.swift +++ b/Sources/NIOHTTP2/HTTP2ChannelHandler.swift @@ -233,9 +233,10 @@ extension NIOHTTP2Handler { var result: StateMachineResultWithEffect switch frame.payload { - case .alternativeService, .origin: - // TODO(cory): Implement - fatalError("Currently some frames are unhandled.") + case .alternativeService(let origin, let field): + result = self.stateMachine.receiveAlternativeService(origin: origin, field: field) + case .origin(let origins): + result = self.stateMachine.receiveOrigin(origins: origins) case .data(let dataBody): result = self.stateMachine.receiveData(streamID: frame.streamID, contentLength: dataBody.data.readableBytes, flowControlledBytes: flowControlledLength, isEndStreamSet: dataBody.endStream) case .goAway(let lastStreamID, _, _): @@ -422,9 +423,12 @@ extension NIOHTTP2Handler { let result: StateMachineResultWithEffect switch frame.payload { - case .alternativeService, .origin: - // TODO(cory): Implement - fatalError("Currently some frames are unhandled.") + case .alternativeService(let origin, let field): + // Returns 'Never'; alt service frames are not currently handled. + self.stateMachine.sendAlternativeService(origin: origin, field: field) + case .origin(let origins): + // Returns 'Never'; origin frames are not currently handled. + self.stateMachine.sendOrigin(origins: origins) case .data(let data): // TODO(cory): Correctly account for padding data. result = self.stateMachine.sendData(streamID: frame.streamID, contentLength: data.data.readableBytes, flowControlledBytes: data.data.readableBytes, isEndStreamSet: data.endStream) diff --git a/Sources/NIOHTTP2/HTTP2Frame.swift b/Sources/NIOHTTP2/HTTP2Frame.swift index 11113acb..e1edc2dd 100644 --- a/Sources/NIOHTTP2/HTTP2Frame.swift +++ b/Sources/NIOHTTP2/HTTP2Frame.swift @@ -107,6 +107,9 @@ public struct HTTP2Frame { /// the locations at which they may be addressed. /// /// See [RFC 7838 § 4](https://tools.ietf.org/html/rfc7838#section-4). + /// + /// - Important: ALTSVC frames are not currently supported. Any received ALTSVC frames will + /// be ignored. Attempting to send an ALTSVC frame will result in a fatal error. indirect case alternativeService(origin: String?, field: ByteBuffer?) /// An ORIGIN frame. This allows servers which allow access to multiple origins @@ -114,6 +117,9 @@ public struct HTTP2Frame { /// this manner. /// /// See [RFC 8336 § 2](https://tools.ietf.org/html/rfc8336#section-2). + /// + /// - Important: ORIGIN frames are not currently supported. Any received ORIGIN frames will + /// be ignored. Attempting to send an ORIGIN frame will result in a fatal error. case origin([String]) /// The payload of a DATA frame. diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift index e780dcaf..807515de 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift @@ -142,6 +142,8 @@ extension ConnectionStateMachineTests { ("testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled), ("testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled), ("testWeTolerateOneStreamBeingResetTwice", testWeTolerateOneStreamBeingResetTwice), + ("testReceivedAltServiceFramesAreIgnored", testReceivedAltServiceFramesAreIgnored), + ("testReceivedOriginFramesAreIgnored", testReceivedOriginFramesAreIgnored), ] } } diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift index 0b85b3a2..0866645c 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift @@ -2974,6 +2974,18 @@ class ConnectionStateMachineTests: XCTestCase { assertSucceeds(self.server.receiveRstStream(streamID: streamOne, reason: .cancel)) assertIgnored(self.server.receiveRstStream(streamID: streamOne, reason: .streamClosed)) } + + func testReceivedAltServiceFramesAreIgnored() { + self.exchangePreamble() + assertIgnored(self.client.receiveAlternativeService(origin: "over-there", field: nil)) + assertIgnored(self.server.receiveAlternativeService(origin: "over-there", field: nil)) + } + + func testReceivedOriginFramesAreIgnored() { + self.exchangePreamble() + assertIgnored(self.client.receiveOrigin(origins: ["one", "two"])) + assertIgnored(self.server.receiveOrigin(origins: ["one", "two"])) + } } diff --git a/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift b/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift index 5dd33a90..88c9da71 100644 --- a/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift +++ b/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift @@ -1806,7 +1806,7 @@ class HTTP2FrameParserTests: XCTestCase { let frameBytes: [UInt8] = [ 0x00, 0x00, 0x2a, // 3-byte payload length (42 bytes) - 0x0c, // 1-byte frame type (ALTSVC) + 0x0c, // 1-byte frame type (ORIGIN) 0x00, // 1-byte flags (none) 0x00, 0x00, 0x00, 0x00, // 4-byte stream identifier, ] diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift index 6dbbb530..09aff318 100644 --- a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift +++ b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift @@ -76,6 +76,8 @@ extension SimpleClientServerFramePayloadStreamTests { ("testStreamCreationOrder", testStreamCreationOrder), ("testStreamClosedInvalidRequestHeaders", testStreamClosedInvalidRequestHeaders), ("testHTTP2HandlerDoesNotFlushExcessively", testHTTP2HandlerDoesNotFlushExcessively), + ("testHTTPHandlerIgnoresInboundAltServiceFrames", testHTTPHandlerIgnoresInboundAltServiceFrames), + ("testHTTPHandlerIgnoresInboundOriginFrames", testHTTPHandlerIgnoresInboundOriginFrames), ] } } diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift index 55d4ab70..29f351e0 100644 --- a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift +++ b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift @@ -1982,4 +1982,61 @@ class SimpleClientServerFramePayloadStreamTests: XCTestCase { self.clientChannel.pipeline.fireChannelReadComplete() XCTAssertEqual(counter.flushCount, 0) } + + func testHTTPHandlerIgnoresInboundAltServiceFrames() throws { + try self.basicHTTP2Connection() + + let h2ClientHandler = try self.clientChannel.pipeline.syncOperations.handler(type: NIOHTTP2Handler.self) + let h2FrameRecorder = FrameRecorderHandler() + try self.clientChannel.pipeline.syncOperations.addHandler(h2FrameRecorder, position: .after(h2ClientHandler)) + + let altServiceFrameBytes: [UInt8] = [ + 0x00, 0x00, 0x15, // 3-byte payload length (21 bytes) + 0x0a, // 1-byte frame type (ALTSVC) + 0x00, // 1-byte flags (none) + 0x00, 0x00, 0x00, 0x00, // 4-byte stream identifier + 0x00, 0x09, // 2-byte origin size ("apple.com"; 9 bytes) + ] + + var buffer = self.clientChannel.allocator.buffer(bytes: altServiceFrameBytes) + buffer.writeString("apple.com") + buffer.writeString("h2=\":8000\"") + XCTAssertEqual(buffer.readableBytes, 30) // 9 (header) + 21 (origin, field) + XCTAssertNoThrow(try self.clientChannel.writeInbound(buffer)) + + // Frame should be dropped. + XCTAssert(h2FrameRecorder.receivedFrames.isEmpty) + } + + func testHTTPHandlerIgnoresInboundOriginFrames() throws { + try self.basicHTTP2Connection() + + let h2ClientHandler = try self.clientChannel.pipeline.syncOperations.handler(type: NIOHTTP2Handler.self) + let h2FrameRecorder = FrameRecorderHandler() + try self.clientChannel.pipeline.syncOperations.addHandler(h2FrameRecorder, position: .after(h2ClientHandler)) + + let originFrameBytes: [UInt8] = [ + 0x00, 0x00, 0x2a, // 3-byte payload length (42 bytes) + 0x0c, // 1-byte frame type (ORIGIN) + 0x00, // 1-byte flags (none) + 0x00, 0x00, 0x00, 0x00 // 4-byte stream identifier, + ] + + var buffer = self.clientChannel.allocator.buffer(bytes: originFrameBytes) + var originBytesWritten = 0 + + for origin in ["apple.com", "www.apple.com", "www2.apple.com"] { + originBytesWritten += try buffer.writeLengthPrefixed(as: UInt16.self) { + $0.writeString(origin) + } + } + + XCTAssertEqual(originBytesWritten, 42) + XCTAssertEqual(buffer.readableBytes, 51) + + XCTAssertNoThrow(try self.clientChannel.writeInbound(buffer)) + + // Frame should be dropped. + XCTAssert(h2FrameRecorder.receivedFrames.isEmpty) + } }