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

    ///         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

    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

    }

+    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

    /// 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

    /// 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.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

            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

            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

    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


    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

        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

            }

+            // 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


    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

                  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 No public API change. 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

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

166114a

FranzBusch reviewed Jul 3, 2024

View reviewed changes

Store the default maximumSequentialContinuationFrames in a `private…

2b6e459

… static let`

FranzBusch approved these changes Jul 4, 2024

View reviewed changes

FranzBusch merged commit d629013 into apple:main Jul 4, 2024

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

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

Reject long sequences of CONTINUATION frames #443

Reject long sequences of CONTINUATION frames #443

Uh oh!

Conversation

clintonpi commented Jun 24, 2024

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glbrntt commented Jun 28, 2024

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants