Repository navigation
Conversation
- Count the packets sealed with each key and refuse to seal once an AES-GCM key reaches its confidentiality limit of 2^23 packets (RFC 9001, Section 6.6) - Initiate a 1-RTT key update once the current key has used half of that limit, keeping the previous read keys until the peer responds in the new key phase - Count packets that fail authentication and close the connection with AEAD_LIMIT_REACHED at the integrity limit, or when the peer never responds to a key update - Add tests covering the per-key limit and an echo with key updates
| repeating: PacketNumber.initial, | ||
| count: PacketNumberSpace.allCases.count | ||
| ) | ||
| // RFC 9001, Section 6.6: an AES-GCM key must not seal more than 2^23 packets. |
There was a problem hiding this comment.
The RFC uses a conservative value. In practice, packets are never 64KB, so our limit is around ~2^28, but we do need to limit it based on the RFC.
There was a problem hiding this comment.
Agreed, kept 2^23 cuz cheap and unconditional
| // Set from initiating a key update until the peer responds in the new key phase | ||
| private(set) var keyUpdatePending = false | ||
| // Packets that failed authentication, across all keys (RFC 9001, Section 6.6) | ||
| private(set) var failedDecryptionCount: UInt64 = 0 |
There was a problem hiding this comment.
Shouldn't this be a property of the Protector?
There was a problem hiding this comment.
Agreed, moved in e7ab354cff671abb6bdebd6ce3bfd2d7dc26889e
| log.notice("Switching to keystate \(packetKeyState)") | ||
| keyState = packetKeyState | ||
| } else if protector.keyUpdateNeeded(for: keyState) { | ||
| close(with: .aeadLimitReached, "peer did not respond to key update", in: &eventContext) |
There was a problem hiding this comment.
I don't think this will work because there may be many packets still inbound that have the old key, right?
There was a problem hiding this comment.
old key packets are expected and dont trigger the close, keystate is already flipped so keyUpdateNeeded(for: keyState) checks the new key -> count starting at 0. Firing after 2^22 packets sent wit the new key and none from peer in new phase. Old packets meanwhile open with previous read keys.
A nice gap here though, old phase packet arriving after the peer's first new phase packet is dropped --> it's noted under not covered in the pr description above. I can add it here or as a follow up, open to suggestions.
- Protector now counts the packets that fail authentication and checks the count against the integrity limit (RFC 9001, Section 6.6) - QUICConnection only acts on the result
| try protector.seal(&packet, frame: &frame) | ||
| } | ||
|
|
||
| func testConfidentialityLimit() throws { |
There was a problem hiding this comment.
Could you please add a test for failedDecryptionCount?
There was a problem hiding this comment.
Added testIntegrityLimit in 866641c, the count increments per failed packet, carries over a key update and failedDrcryption(for:) returns once count exceeds integrity limit. nb failedDecrytionCount was made settable similar to aesGCMConfidentialityLimit, if that's ok?
There was a problem hiding this comment.
nb section 6.6 goes 2^52 limit for aes gcm and 2^36 for chacha20-poly1305, documented in the description
- Cover the failed decryption count and the integrity limit for AES-GCM and ChaCha20-Poly1305 (RFC 9001, Section 6.6) - Make failedDecryptionCount settable so the test can reach the limit
| ) | ||
| // RFC 9001, Section 6.6: an AES-GCM key must not seal more than 2^23 packets. | ||
| // ChaCha20-Poly1305 has no reachable confidentiality limit. | ||
| var aesGCMConfidentialityLimit: UInt64 = 1 << 23 |
There was a problem hiding this comment.
An alternative is to initialize this in init() and make this a let. I think that would work with the tests.
There was a problem hiding this comment.
Done in 7189db5. its a let set in init now, although the harness test was lowering it on a live connection so it builds the client's Protector insteaf which needed originalDCID to be private(set).
- Protector takes the limit in init, defaulting to 2^23 packets (RFC 9001, Section 6.6) - Tests pass a lower limit at init. The harness test builds the client's Protector before the handshake, so originalDCID becomes readable
|
Is it possible to write a test that reaches AEAD limit and closes the connection? I suspect that doesn't work because we will not be able to seal the CONNECTION_CLOSE. Should we limit it to 2 << 23 - 1? |
|
https://www.rfc-editor.org/rfc/rfc9001.html#ex-key-update https://www.rfc-editor.org/rfc/rfc9001.html#section-6.1 shows that we should wait for an ACK before accepting that the key update was successful:
We should probably implement that. |
yeah, I ran a test and the probe run hung with both sides connected and no error, although the close fires at half the budget so there's normally room to seal it, issue seems to be that no close is atetmpted in the first place. I did it for 4 packets though, seal refused for 5th packet 32 times before a single inbound packet was processed, transfer never finished --> aeadLimitReached never fired. switch keys at half budget and the close live only in receive side of things, sender never checks, the silent hang issue makes the requested test not pass today, gotta enforce the limit on the send path, when key has one packet of budget left, we close wtih .aeadLimitReached and server receive connection close,
Agree, that's a MUST, currently what I had is just a proxy. |
- A key update is only initiated once the peer has acknowledged a packet from the current key phase (RFC 9001, Section 6.1). The decision moves after the frames of a packet so that an ACK it carries counts, and an endpoint that only sends ACKs elicits one with a PING - The last packet a key may protect is kept for a CONNECTION_CLOSE. Once that is all that is left and no key update is possible, the connection closes with AEAD_LIMIT_REACHED (RFC 9001, Section 6.6) - Tests: a client that runs into the limit closes and the server receives its CONNECTION_CLOSE, and a client that only receives keeps updating
|
Also added a PING in |
2 participants
RFC 9001 Section 6.6 requires an endpoint to count packets per key, initiate a key update before an AES-GCM key protects more than 2^23 packets, and close with AEAD_LIMIT_REACHED once too many packets fail authentication. Today
aeadLimitReachedis never raised,Protectorkeeps no counts, and a key update is only ever followed, never initiated. 2^23 packets is about 10 GB at 1,200 bytes per packet.Changes
Protector: count the packets sealed with each key and refuse to seal once an AES-GCM key reaches 2^23 packets.QUICConnection: initiate a 1-RTT key update once the write key has used half of that limit. Close with AEAD_LIMIT_REACHED if the peer never responds, or when failed authentications exceed the integrity limit (2^52 for AES-GCM, 2^36 for ChaCha20-Poly1305).PacketParser: while a locally initiated update is pending, keep opening the peer's packets with the previous read keys instead of treating them as a new key update.Why
sealcovers the case where nothing arrives.aesGCMConfidentialityLimitis avarso tests can lower it.Not covered
Old read keys are not retained once a key update completes, so a reordered packet from the previous key phase is dropped (RFC 9001 Section 6.5). This is existing behaviour for peer-initiated updates and can be a separate change.
Testing
swift testpasses (1281 tests). New:ProtectorTests.testConfidentialityLimitandSwiftNetworkQUICHarnessTests.testQUICEchoWithKeyUpdates.