Reject long sequences of CONTINUATION frames #443

clintonpi · 2024-06-24T09:34:56Z

Motivation:

Long sequences of CONTINUATION frames can be used to mount attacks by attempting to get a remote peer to consume large amounts of memory.

Modifications:

Add a limit to the number of sequential CONTINUATION frames that can be received. This limit is configurable by users at the NIOHTTP2Handler level and has a default value of 5. When this limit is exceeded, the recipient responds with a GOAWAY frame and an "Enhance Your Calm" error of a newly created type ExcessiveContinuationFrames.

Result:

Long sequences of CONTINUATION frames are now rejected by the recipient.

Motivation: Long sequences of CONTINUATION frames can be used to mount attacks by attempting to get a remote peer to consume large amounts of memory. Modifications: - Add a limit to the number of sequential CONTINUATION frames that can be received. This limit is configurable by users at the NIOHTTP2Handler level and has a default value of 5. When this limit is exceeded, the recipient responds with a GOAWAY frame and an "Enhance Your Calm" error of a newly created type `ExcessiveContinuationFrames`. Result: Long sequences of CONTINUATION frames are now rejected by the recipient.

FranzBusch

This looks great. I left a comment w.r.t. the changes in the public API

FranzBusch · 2024-06-24T11:11:24Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -228,19 +232,22 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
    ///   - maximumBufferedControlFrames: Controls the maximum buffer size of buffered outbound control frames. If we are unable to send control frames as
    ///         fast as we produce them we risk building up an unbounded buffer and exhausting our memory. To protect against this DoS vector, we put an
    ///         upper limit on the depth of this queue. Defaults to 10,000.
+    ///   - maximumSequentialContinuationFrames: The maximum number of sequential CONTINUATION frames.
    public convenience init(mode: ParserMode,


This is a breaking API change. We can't add a new parameter to a public method even if we default it since somebody could reference the init/method. That's the reason we introduced ConnectionConfiguration which is extensible. So let's remove any changes from the public inits here and just change the ConnectionConfiguration struct

glbrntt · 2024-06-24T13:01:17Z

Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift

@@ -133,6 +133,8 @@ class ConnectionStateMachineTests: XCTestCase {
    var clientEncoder: HTTP2FrameEncoder!
    var clientDecoder: HTTP2FrameDecoder!

+    var maximumSequentialContinuationFrames: Int = 5


This can be a let, changing it after setUp has been called (which happens before the test is run) will have no effect.

glbrntt · 2024-06-24T13:01:56Z

Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift

@@ -785,6 +835,61 @@ class HTTP2FrameParserTests: XCTestCase {
        try assertReadsFrame(from: buf, matching: expectedFrame)
    }

+    func testMaximumSequentialContinuationFrames() throws {
+        let CONTINUATION: [UInt8] = [


no need to use shouty-case here, let continuationBytes is fine

glbrntt · 2024-06-24T13:07:13Z

Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift

+            // CONTINUATION frame with the END_HEADERS flag not set
+            0x00, 0x00, 0x00,           // 3-byte payload length (0 bytes)
+            0x09,                       // 1-byte frame type (CONTINUATION)
+            0x00,                       // 1-byte flags (END_HEADERS)


The end headers flag isn't set here but the comment says it is

glbrntt · 2024-06-24T14:09:05Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

@@ -31,9 +31,11 @@ struct HTTP2FrameDecoder {
    /// The state for a parser that is waiting for the client magic.
    private struct ClientMagicState {
        private var pendingBytes: ByteBuffer?
+        private var maximumSequentialContinuationFrames: Int


I don't think this value should ever change, it should be a let

glbrntt · 2024-06-24T14:10:00Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

@@ -58,14 +63,16 @@ struct HTTP2FrameDecoder {
    /// The state for a parser that is currently accumulating the bytes of a frame header.
    private struct AccumulatingFrameHeaderParserState {
        private(set) var unusedBytes: ByteBuffer
+        private(set) var maximumSequentialContinuationFrames: Int


Same here, can this ever change?

No, it can't.

glbrntt · 2024-06-24T14:13:47Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

+        init(
+            fromIdle state: AccumulatingFrameHeaderParserState,
+            header: FrameHeader,
+            maximumSequentialContinuationFrames: Int


Same here, we can just copy it from state rather than pasing it in separately

glbrntt · 2024-06-24T14:15:30Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

            self.header = header
            self.payload = state.unusedBytes
            self.expectedPadding = nil
            self.remainingByteCount = remainingBytes
            self.flowControlledLength = header.length
+            self.maximumSequentialContinuationFrames = maximumSequentialContinuationFrames


same here re: copying from state

glbrntt · 2024-06-24T14:16:26Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

            precondition(expectedPadding > 0)

            self.header = state.header
            self.excessBytes = excessBytes
            self.expectedPadding = expectedPadding
+            self.maximumSequentialContinuationFrames = maximumSequentialContinuationFrames


same here, let's just copy from state

glbrntt · 2024-06-24T14:16:37Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

            precondition(expectedPadding > 0)

            self.header = state.header
            self.excessBytes = excessBytes
            self.expectedPadding = expectedPadding
+            self.maximumSequentialContinuationFrames = maximumSequentialContinuationFrames


same here, let's just copy from state

glbrntt · 2024-06-24T14:17:06Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

-
-        init(fromAccumulatingHeaderBlockFragments acc: AccumulatingHeaderBlockFragmentsParserState,
-             continuationHeader: FrameHeader) {
+        private var sequentialContinuationFramesSize: Int


nit: prefer count to size when counting things

glbrntt · 2024-06-25T09:47:19Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -104,6 +104,9 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
    /// The delegate for (de)multiplexing inbound streams.
    private var inboundStreamMultiplexerState: InboundStreamMultiplexerState

+    /// The maximum number of sequential CONTINUATION frames.
+    private var maximumSequentialContinuationFrames: Int


This should be a constant

glbrntt · 2024-06-26T13:03:10Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

@@ -685,6 +775,7 @@ struct HTTP2FrameDecoder {

    internal var headerDecoder: HPACKDecoder
    private var state: ParserState
+    private var maximumSequentialContinuationFrames: Int


This should be a let. Also we don't need to pass maximumSequentialContinuationFrames through all of the states, I think we only need to pass it to the process func of the appropriate state.

Also we don't need to pass maximumSequentialContinuationFrames through all of the states, I think we only need to pass it to the process func of the appropriate state.

Yes. I completely missed that. Thanks!

This should still be a let

- Refactor implementation - Revert unnecessary diffs

glbrntt · 2024-06-27T07:00:11Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -1109,6 +1129,7 @@ extension NIOHTTP2Handler {
        public var contentLengthValidation: ValidationState = .enabled
        public var maximumSequentialEmptyDataFrames: Int = 1
        public var maximumBufferedControlFrames: Int = 10000
+        public let maximumSequentialContinuationFrames: Int = 5


This should be var as its a config that users can adjust before passing in

Suggested change

public let maximumSequentialContinuationFrames: Int = 5

public var maximumSequentialContinuationFrames: Int = 5

glbrntt · 2024-06-27T07:03:11Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

@@ -601,6 +619,11 @@ struct HTTP2FrameDecoder {
                throw NIOHTTP2Errors.excessivelyLargeHeaderBlock()
            }

+            // The sequence of CONTINUATION frames received is not longer than the configured limit
+            guard self.sequentialContinuationFramesCount < maximumSequentialContinuationFrames else {


Shouldn't this be <=?

No, it shouldn't be. But I have made some adjustments to make the condition clearer.

glbrntt · 2024-06-27T07:05:44Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

+                try self.processNextState(
+                    maximumSequentialContinuationFrames: self.maximumSequentialContinuationFrames
+                )


We don't need other pass this in here: maximumSequentialContinuationFrames is already stored on self so we can just read it from self in processNextState

- Change `public let maximumSequentialContinuationFrames` to `public var`. - Refactor implementation.

glbrntt · 2024-06-28T09:12:55Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

@@ -685,6 +775,7 @@ struct HTTP2FrameDecoder {

    internal var headerDecoder: HPACKDecoder
    private var state: ParserState
+    private var maximumSequentialContinuationFrames: Int


This should still be a let

glbrntt · 2024-06-28T09:13:16Z

Sources/NIOHTTP2/HTTP2FrameParser.swift

+            switch (
+                try self.processNextState()
+            ) {


Suggested change

switch (

try self.processNextState()

) {

switch (try self.processNextState()) {

glbrntt · 2024-06-28T09:14:51Z

Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift

+            0x00,                       // payload
+
+        ]


Suggested change

0x00, // payload

]

0x00, // payload

]

glbrntt · 2024-06-28T09:15:13Z

Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift

+        )
+
+        let headersFrame: [UInt8] = [
+            0x00, 0x00, 0x01,           // 3-byte payload length (1 byte)


Why do we have a 1-byte payload?

glbrntt · 2024-06-28T09:17:52Z

Tests/NIOHTTP2Tests/HTTP2InlineStreamMultiplexerTests.swift

-        var frameDecoder = HTTP2FrameDecoder(allocator: ByteBufferAllocator(), expectClientMagic: mode == .client)
+    func drainConnectionSetupWrites(
+        mode: NIOHTTP2Handler.ParserMode = .server,
+        maximumSequentialContinuationFrames: Int


Can we just default this to 5? Doing that means we can avoid all the other changes in this file.

glbrntt · 2024-06-28T16:31:11Z

@swift-server-bot add to allowlist

FranzBusch

LGTM just one nit

FranzBusch · 2024-07-03T12:08:55Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -212,6 +215,7 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
                  contentLengthValidation: contentLengthValidation,
                  maximumSequentialEmptyDataFrames: 1,
                  maximumBufferedControlFrames: 10000,
+                  maximumSequentialContinuationFrames: 5,


I have one more nit here. Instead of respelling this default of 5 in multiple places can we pull it out into one static let that we reference?

… static let`

FranzBusch

Awesome! LGTM

FranzBusch reviewed Jun 24, 2024

View reviewed changes

glbrntt requested changes Jun 24, 2024

View reviewed changes

clintonpi added 2 commits June 25, 2024 10:35

Refactor implementation

63aa030

Minor adjustments

ebd2ba2

clintonpi requested review from FranzBusch and glbrntt June 25, 2024 09:42

glbrntt requested changes Jun 26, 2024

View reviewed changes

Lukasa added the 🔨 semver/patch label Jun 26, 2024

Implement adjustments

dd71a09

- Refactor implementation - Revert unnecessary diffs

clintonpi requested a review from glbrntt June 26, 2024 15:32

glbrntt requested changes Jun 27, 2024

View reviewed changes

Implement adjustments

9a66969

- Change `public let maximumSequentialContinuationFrames` to `public var`. - Refactor implementation.

clintonpi requested a review from glbrntt June 28, 2024 08:59

glbrntt requested changes Jun 28, 2024

View reviewed changes

Implement minor adjustments

Loading
Loading status checks…

9c886f0

clintonpi requested a review from glbrntt June 28, 2024 16:02

glbrntt approved these changes Jun 28, 2024

View reviewed changes

Merge branch 'main' into reject-long-sequences-of-continuation-frames

Loading
Loading status checks…

166114a

FranzBusch reviewed Jul 3, 2024

View reviewed changes

Store the default maximumSequentialContinuationFrames in a `private…

Loading
Loading status checks…

2b6e459

… static let`

FranzBusch approved these changes Jul 4, 2024

View reviewed changes

FranzBusch merged commit d629013 into apple:main Jul 4, 2024
6 of 7 checks passed

clintonpi deleted the reject-long-sequences-of-continuation-frames branch July 4, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject long sequences of CONTINUATION frames #443

Reject long sequences of CONTINUATION frames #443

clintonpi commented Jun 24, 2024

FranzBusch left a comment

FranzBusch Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

clintonpi Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 24, 2024

glbrntt Jun 25, 2024

glbrntt Jun 26, 2024

clintonpi Jun 26, 2024

glbrntt Jun 28, 2024

glbrntt Jun 27, 2024

glbrntt Jun 27, 2024

clintonpi Jun 28, 2024

glbrntt Jun 27, 2024

glbrntt Jun 28, 2024

glbrntt Jun 28, 2024

glbrntt Jun 28, 2024

glbrntt Jun 28, 2024

glbrntt Jun 28, 2024

glbrntt commented Jun 28, 2024

FranzBusch left a comment

FranzBusch Jul 3, 2024

FranzBusch left a comment

	public let maximumSequentialContinuationFrames: Int = 5
	public var maximumSequentialContinuationFrames: Int = 5

Reject long sequences of CONTINUATION frames #443

Reject long sequences of CONTINUATION frames #443

Conversation

clintonpi commented Jun 24, 2024

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt commented Jun 28, 2024

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment