QUIC: enforce active_connection_id_limit on received NEW_CONNECTION_ID

Sources/SwiftNetwork/QUIC/QUICConnection.swift

@@ -200,7 +200,7 @@ public final class QUICConnection: ManyToManyApplicationStreamProtocol,
     private var largestSentLocalCIDSequenceNumber: UInt64 = 1
 
     // All the CIDs advertised by the peer
-    var remoteCIDs = QUICConnectionIDList()
+    var remoteCIDs = QUICConnectionIDList(remembersHeldSequenceNumbers: true)
 
     // The largest "retire prior to" value received
     private var retiredRemoteCIDSequenceNumberThreshold: UInt64 = 0
@@ -6966,6 +6966,7 @@ extension QUICConnection {
         // connection ID, unless it has already done so for that sequence number.
         if frame.retirePriorToSequence > retiredRemoteCIDSequenceNumberThreshold {
             retiredRemoteCIDSequenceNumberThreshold = frame.retirePriorToSequence
+            remoteCIDs.markHeld(priorTo: frame.retirePriorToSequence)
         }
         if frame.sequence < retiredRemoteCIDSequenceNumberThreshold {
             // Send a frame to retire the connection ID
@@ -6981,7 +6982,12 @@ extension QUICConnection {
         // If we have not seen this frame before and haven't reached the
         // active CID limit, add it to the CID table.
         let cidLimit = remoteCIDs.activeConnectionIDLimit
-        if remoteCIDs.count < cidLimit {
+        if remoteCIDs.hasHeld(sequenceNumber: frame.sequence) {
+            // The lookup by connection ID above misses a connection ID that we have retired ourselves,
+            // so a repeat of its frame is recognized by sequence number. It must not be added again,
+            // and must not count against the limit.
+            log.debug("Ignoring NEW_CONNECTION_ID with already seen sequence \(frame.sequence)")
+        } else if remoteCIDs.count < cidLimit {
             do {
                 try remoteCIDs.insert(
                     sequenceNumber: frame.sequence,
@@ -7011,7 +7017,14 @@ extension QUICConnection {
                 )
             }
         } else {
-            log.info("Attempt to add new CID that exceeds the configured cid limit (\(cidLimit))")
+            // RFC 9000: 5.1.1:
+            // After processing a NEW_CONNECTION_ID frame and adding and retiring active connection IDs, if the
+            // number of active connection IDs exceeds the value advertised in its active_connection_id_limit
+            // transport parameter, an endpoint MUST close the connection with an error of type
+            // CONNECTION_ID_LIMIT_ERROR.
+            log.error("Attempt to add new CID that exceeds the configured cid limit (\(cidLimit))")
+            close(with: .connectionIDLimitError, "NEW_CONNECTION_ID: CID limit exceeded", in: &eventContext)
+            return false
         }
 
         // Re-point the path only after the insert above: the CID this frame supplies may be

Sources/SwiftNetwork/QUIC/QUICConnectionID.swift

@@ -253,6 +253,41 @@ struct QUICConnectionIDList: Sequence, IteratorProtocol {
         forType: .activeConnectionIDLimit
     )!
 
+    // Sequence numbers this list has held, remembered after they are retired so that a repeated
+    // NEW_CONNECTION_ID frame for a retired connection ID is not taken for a new one. Only the
+    // list of the peer's connection IDs needs this, so it is off unless asked for.
+    private let remembersHeldSequenceNumbers: Bool
+    private var heldSequenceNumbers = RangeSet<UInt64>()
+
+    init(remembersHeldSequenceNumbers: Bool = false) {
+        self.remembersHeldSequenceNumbers = remembersHeldSequenceNumbers
+    }
+
+    func hasHeld(sequenceNumber: UInt64) -> Bool {
+        heldSequenceNumbers.contains(sequenceNumber)
+    }
+
+    private mutating func recordHeld(sequenceNumber: UInt64) {
+        guard remembersHeldSequenceNumbers else { return }
+        // Sequence numbers are variable-length integers, so at most 2^62 - 1, and cannot overflow.
+        heldSequenceNumbers.insert(contentsOf: sequenceNumber..<sequenceNumber &+ 1)
+        // Everything below Retire Prior To is filled in by markHeld(priorTo:), so a gap is a frame
+        // above it that is lost or still in flight, and a conforming peer has at most the limit of
+        // those. Close the oldest gap once there are more gaps than twice the limit, so that the set
+        // stays bounded against a peer that is not.
+        let ranges = heldSequenceNumbers.ranges
+        if ranges.count - 1 > 2 * activeConnectionIDLimit {
+            heldSequenceNumbers.insert(contentsOf: ranges[0].upperBound..<ranges[1].lowerBound)
+        }
+    }
+
+    // Every sequence number below Retire Prior To is retired as soon as its frame arrives, without
+    // reaching this list, so count them all as held rather than leaving a gap for each late one.
+    mutating func markHeld(priorTo: UInt64) {
+        guard remembersHeldSequenceNumbers else { return }
+        heldSequenceNumbers.insert(contentsOf: 0..<priorTo)
+    }
+
     // The initial connection ID is valid without a Stateless Reset Token (see RFC9000, Section 18.2).
     // If peer's transport parameters include a stateless reset token, use the normal `insert()` call.
     // NOTE: This API may only be called once for this instance of QUICConnectionIDList
@@ -270,6 +305,7 @@ struct QUICConnectionIDList: Sequence, IteratorProtocol {
             used: true
         )
         managedConnectionIDs.append(newCID)
+        recordHeld(sequenceNumber: 0)
     }
 
     static let preferredAddressSequenceNumber: UInt64 = 1
@@ -317,6 +353,7 @@ struct QUICConnectionIDList: Sequence, IteratorProtocol {
         )
         managedConnectionID.preferredAddress = preferredAddress
         managedConnectionIDs.append(managedConnectionID)
+        recordHeld(sequenceNumber: sequenceNumber)
     }
 
     @discardableResult

Tests/QUICTests/ConnectionIDRotationTests.swift

@@ -120,6 +120,135 @@ final class ConnectionIDRotationTests: XCTestCase {
         }
         wait(for: [expectation], timeout: 5.0)
     }
+
+    // RFC 9000 5.1.1: a NEW_CONNECTION_ID frame that takes the active CID count past the advertised
+    // active_connection_id_limit, without retiring anything, must close the connection with
+    // CONNECTION_ID_LIMIT_ERROR. Filling the pool to the limit, or repeating a frame, must not.
+    func testNewConnectionIDOverLimitClosesConnection() {
+        let expectation = XCTestExpectation()
+        connection.context.async {
+            let path = self.makePath(dcid: QUICConnectionID([0xA1, 0xA2, 0xA3, 0xA4])!, sequenceNumber: 0, used: true)
+            self.connection.currentPath = path
+            self.connection.remoteCIDs.activeConnectionIDLimit = 2
+
+            let atLimit = FrameNewConnectionID(
+                sequence: 1,
+                retirePriorToSequence: 0,
+                connectionID: QUICConnectionID([0xB1, 0xB2, 0xB3, 0xB4])!,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+            let overLimit = FrameNewConnectionID(
+                sequence: 2,
+                retirePriorToSequence: 0,
+                connectionID: QUICConnectionID([0xC1, 0xC2, 0xC3, 0xC4])!,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+
+            self.connection.fromExternal { eventContext in
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(atLimit, in: &eventContext))
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(atLimit, in: &eventContext))
+            }
+            XCTAssertNil(self.connection.closeError, "Reaching the limit, or a repeated frame, is not an error")
+            XCTAssertEqual(self.connection.remoteCIDs.count, 2)
+
+            self.connection.fromExternal { eventContext in
+                XCTAssertFalse(self.connection.processNewConnectionIDFrame(overLimit, in: &eventContext))
+            }
+            XCTAssertEqual(
+                self.connection.closeError?.code,
+                QUICTransportError.QUICTransportErrorCode.connectionIDLimitError.rawValue,
+                "Exceeding the limit should close with CONNECTION_ID_LIMIT_ERROR"
+            )
+
+            expectation.fulfill()
+        }
+        wait(for: [expectation], timeout: 5.0)
+    }
+
+    // RFC 9000 section 5.1.1: a peer may send a connection ID that temporarily exceeds the limit if the
+    // frame's Retire Prior To retires the excess. Retirement must be counted before the limit is.
+    func testNewConnectionIDAtLimitWithRetirePriorToIsAccepted() {
+        let expectation = XCTestExpectation()
+        connection.context.async {
+            let path = self.makePath(dcid: QUICConnectionID([0xA1, 0xA2, 0xA3, 0xA4])!, sequenceNumber: 0, used: true)
+            self.connection.currentPath = path
+            self.connection.remoteCIDs.activeConnectionIDLimit = 2
+
+            let atLimit = FrameNewConnectionID(
+                sequence: 1,
+                retirePriorToSequence: 0,
+                connectionID: QUICConnectionID([0xB1, 0xB2, 0xB3, 0xB4])!,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+            let replacesAll = FrameNewConnectionID(
+                sequence: 2,
+                retirePriorToSequence: 2,
+                connectionID: QUICConnectionID([0xC1, 0xC2, 0xC3, 0xC4])!,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+
+            self.connection.fromExternal { eventContext in
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(atLimit, in: &eventContext))
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(replacesAll, in: &eventContext))
+            }
+            XCTAssertNil(self.connection.closeError, "Retiring the excess in the same frame is not an error")
+            XCTAssertEqual(self.connection.remoteCIDs.count, 1)
+            XCTAssertNotNil(self.connection.remoteCIDs.find(sequenceNumber: 2))
+
+            expectation.fulfill()
+        }
+        wait(for: [expectation], timeout: 5.0)
+    }
+
+    // RFC 9000 19.15: receiving the same NEW_CONNECTION_ID frame more than once must not be treated
+    // as a connection error. A CID we retired ourselves is gone from remoteCIDs, so a late
+    // retransmission of its frame has to be recognized by sequence number. It must not be added
+    // back, and must not count against the limit once the peer has replaced the retired CID.
+    func testRepeatedNewConnectionIDForLocallyRetiredCIDIsIgnored() {
+        let expectation = XCTestExpectation()
+        connection.context.async {
+            let retiredCID = QUICConnectionID([0xB1, 0xB2, 0xB3, 0xB4])!
+            let path = self.makePath(dcid: QUICConnectionID([0xA1, 0xA2, 0xA3, 0xA4])!, sequenceNumber: 0, used: true)
+            self.connection.currentPath = path
+            self.connection.remoteCIDs.activeConnectionIDLimit = 2
+
+            // A second path used seq 1 and went away, as after a migration.
+            let oldPath = self.makePath(dcid: retiredCID, sequenceNumber: 1, used: true)
+            self.connection.retireOutboundCID(forPathGoingAway: oldPath)
+            XCTAssertEqual(self.connection.remoteCIDs.count, 1, "Seq 1 should be retired")
+
+            let repeated = FrameNewConnectionID(
+                sequence: 1,
+                retirePriorToSequence: 0,
+                connectionID: retiredCID,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+            let replacement = FrameNewConnectionID(
+                sequence: 2,
+                retirePriorToSequence: 0,
+                connectionID: QUICConnectionID([0xC1, 0xC2, 0xC3, 0xC4])!,
+                statelessResetToken: QUICStatelessResetToken()
+            )
+
+            self.connection.fromExternal { eventContext in
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(repeated, in: &eventContext))
+            }
+            XCTAssertNil(
+                self.connection.remoteCIDs.find(connectionID: retiredCID),
+                "A retired CID must not be added back"
+            )
+
+            self.connection.fromExternal { eventContext in
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(replacement, in: &eventContext))
+                XCTAssertTrue(self.connection.processNewConnectionIDFrame(repeated, in: &eventContext))
+            }
+            XCTAssertNil(self.connection.closeError, "A repeated frame for a retired CID is not an error")
+            XCTAssertEqual(self.connection.remoteCIDs.count, 2)
+
+            expectation.fulfill()
+        }
+        wait(for: [expectation], timeout: 5.0)
+    }
 }
 
 #endif

Tests/QUICTests/QUICConnectionIDListTests.swift

@@ -266,6 +266,82 @@ class QUICConnectionIDListTests: XCTestCase {
         XCTAssertEqual(list.count, 0)
     }
 
+    // The list of our own connection IDs does not pay for remembering sequence numbers.
+    func testDoesNotRememberHeldByDefault() {
+        XCTAssertNoThrow(
+            try list.insertInitialConnectionID(QUICConnectionID(5))
+        )
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 0))
+    }
+
+    func testHasHeldAfterRetire() {
+        list = QUICConnectionIDList(remembersHeldSequenceNumbers: true)
+        let c1 = QUICConnectionID(5)
+        XCTAssertNoThrow(
+            try list.insertInitialConnectionID(QUICConnectionID(5))
+        )
+        XCTAssertNoThrow(
+            try list.insert(sequenceNumber: 2, connectionID: c1, token: QUICStatelessResetToken())
+        )
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 0))
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 1))
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 2))
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 3))
+
+        list.retire(connectionID: c1)
+        _ = list.retire(priorTo: 1)
+        XCTAssertTrue(list.isEmpty)
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 0))
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 1))
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 2))
+    }
+
+    // A sequence number below Retire Prior To is retired on arrival without reaching the list, so a
+    // lost frame below it must not leave a gap.
+    func testMarkHeldPriorToClosesGaps() {
+        list = QUICConnectionIDList(remembersHeldSequenceNumbers: true)
+        XCTAssertNoThrow(
+            try list.insertInitialConnectionID(QUICConnectionID(5))
+        )
+        XCTAssertNoThrow(
+            try list.insert(sequenceNumber: 2, connectionID: QUICConnectionID(5), token: QUICStatelessResetToken())
+        )
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 1))
+
+        list.markHeld(priorTo: 3)
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 1))
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 2))
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 3))
+    }
+
+    // Sequence numbers that never arrive must not make the list remember gaps without bound.
+    func testHasHeldClosesOldestGap() {
+        list = QUICConnectionIDList(remembersHeldSequenceNumbers: true)
+        list.activeConnectionIDLimit = 2
+        XCTAssertNoThrow(
+            try list.insertInitialConnectionID(QUICConnectionID(5))
+        )
+        // Seq 1, 3, 5 and 7 are missing, which leaves four gaps, twice the limit.
+        for sequenceNumber: UInt64 in [2, 4, 6, 8] {
+            XCTAssertNoThrow(
+                try list.insert(
+                    sequenceNumber: sequenceNumber,
+                    connectionID: QUICConnectionID(5),
+                    token: QUICStatelessResetToken()
+                )
+            )
+        }
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 1))
+
+        // A fifth gap closes the oldest one, so seq 1 now reads as held and seq 3 still does not.
+        XCTAssertNoThrow(
+            try list.insert(sequenceNumber: 10, connectionID: QUICConnectionID(5), token: QUICStatelessResetToken())
+        )
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 1))
+        XCTAssertFalse(list.hasHeld(sequenceNumber: 3))
+        XCTAssertTrue(list.hasHeld(sequenceNumber: 10))
+    }
+
     func verifyInitialState() {
         XCTAssertTrue(list.isEmpty)
         XCTAssertEqual(list.count, 0)