CONTINUATION frames must be for the correct stream. (apple#66)

Motivation: We mustn't allow CONTINUATION frames to be sent on invalid stream IDs. Modifications: - Police that the CONTINUATION frame raw stream ID matches the previous frame's raw stream ID. Result: Better conformance to the RFC
Lukasa · Mar 15, 2019 · 1138881 · 1138881
1 parent d9f1131
commit 1138881
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 0 deletions.
diff --git a/Sources/NIOHTTP2/HTTP2FrameParser.swift b/Sources/NIOHTTP2/HTTP2FrameParser.swift
@@ -528,6 +528,11 @@ struct HTTP2FrameDecoder {
                 throw InternalError.codecError(code: .protocolError)
             }
 
+            // This must be for the stream we're buffering header block fragments for, or this is an error.
+            guard header.rawStreamID == state.header.rawStreamID else {
+                throw InternalError.codecError(code: .protocolError)
+            }
+
             self.state = .accumulatingContinuationPayload(AccumulatingContinuationPayloadParserState(fromAccumulatingHeaderBlockFragments: state, continuationHeader: header))
         }
 

diff --git a/Tests/NIOHTTP2Tests/HTTP2FrameParserTests+XCTest.swift b/Tests/NIOHTTP2Tests/HTTP2FrameParserTests+XCTest.swift
@@ -79,6 +79,8 @@ extension HTTP2FrameParserTests {
                 ("testHeadersContinuationFrameDecoding", testHeadersContinuationFrameDecoding),
                 ("testPushPromiseContinuationFrameDecoding", testPushPromiseContinuationFrameDecoding),
                 ("testUnsolicitedContinuationFrame", testUnsolicitedContinuationFrame),
+                ("testContinuationFrameStreamZero", testContinuationFrameStreamZero),
+                ("testContinuationFrameWrongStream", testContinuationFrameWrongStream),
                 ("testAltServiceFrameDecoding", testAltServiceFrameDecoding),
                 ("testAltServiceFrameDecodingFailure", testAltServiceFrameDecodingFailure),
                 ("testAltServiceFrameEncoding", testAltServiceFrameEncoding),

diff --git a/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift b/Tests/NIOHTTP2Tests/HTTP2FrameParserTests.swift
@@ -1481,6 +1481,84 @@ class HTTP2FrameParserTests: XCTestCase {
             XCTAssertEqual(error as? InternalError, .codecError(code: .protocolError))
         }
     }
+
+    func testContinuationFrameStreamZero() throws {
+        var headers2 = self.byteBuffer(withBytes: self.simpleHeadersEncoded)
+        var headers1 = headers2.readSlice(length: 10)!
+
+        let frameBytes: [UInt8] = [
+            0x00, 0x00, 0x0a,           // 3-byte payload length (10 bytes)
+            0x01,                       // 1-byte frame type (HEADERS)
+            0x00,                       // 1-byte flags (none)
+            0x00, 0x00, 0x00, 0x01,     // 4-byte stream identifier
+        ]
+        var buf = byteBuffer(withBytes: frameBytes, extraCapacity: 10)
+        buf.writeBuffer(&headers1)
+
+        var decoder = HTTP2FrameDecoder(allocator: self.allocator, expectClientMagic: false)
+
+        // should return nothing thus far and wait for CONTINUATION frames and an END_HEADERS flag
+        decoder.append(bytes: &buf)
+        XCTAssertNil(try decoder.nextFrame())
+
+        // should consume all the bytes
+        XCTAssertEqual(buf.readableBytes, 0)
+        buf.clear()
+
+        let continuationFrameBytes: [UInt8] = [
+            0x00, 0x00, 0x07,           // 3-byte payload length (7 bytes)
+            0x09,                       // 1-byte frame type (CONTINUATION)
+            0x04,                       // 1-byte flags (END_HEADERS)
+            0x00, 0x00, 0x00, 0x00,     // 4-byte stream identifier, stream 0
+        ]
+        buf.writeBytes(continuationFrameBytes)
+        buf.writeBuffer(&headers2)
+
+        // This should fail
+        decoder.append(bytes: &buf)
+        XCTAssertThrowsError(try decoder.nextFrame()) { error in
+            XCTAssertEqual(error as? InternalError, .codecError(code: .protocolError))
+        }
+    }
+
+    func testContinuationFrameWrongStream() throws {
+        var headers2 = self.byteBuffer(withBytes: self.simpleHeadersEncoded)
+        var headers1 = headers2.readSlice(length: 10)!
+
+        let frameBytes: [UInt8] = [
+            0x00, 0x00, 0x0a,           // 3-byte payload length (10 bytes)
+            0x01,                       // 1-byte frame type (HEADERS)
+            0x00,                       // 1-byte flags (none)
+            0x00, 0x00, 0x00, 0x01,     // 4-byte stream identifier
+        ]
+        var buf = byteBuffer(withBytes: frameBytes, extraCapacity: 10)
+        buf.writeBuffer(&headers1)
+
+        var decoder = HTTP2FrameDecoder(allocator: self.allocator, expectClientMagic: false)
+
+        // should return nothing thus far and wait for CONTINUATION frames and an END_HEADERS flag
+        decoder.append(bytes: &buf)
+        XCTAssertNil(try decoder.nextFrame())
+
+        // should consume all the bytes
+        XCTAssertEqual(buf.readableBytes, 0)
+        buf.clear()
+
+        let continuationFrameBytes: [UInt8] = [
+            0x00, 0x00, 0x07,           // 3-byte payload length (7 bytes)
+            0x09,                       // 1-byte frame type (CONTINUATION)
+            0x04,                       // 1-byte flags (END_HEADERS)
+            0x00, 0x00, 0x00, 0x03,     // 4-byte stream identifier, stream 3
+        ]
+        buf.writeBytes(continuationFrameBytes)
+        buf.writeBuffer(&headers2)
+
+        // This should fail
+        decoder.append(bytes: &buf)
+        XCTAssertThrowsError(try decoder.nextFrame()) { error in
+            XCTAssertEqual(error as? InternalError, .codecError(code: .protocolError))
+        }
+    }
 
     // MARK: - ALTSVC frames