quic: allow new Client Initial on reused flow

[jiangshaoqi]

Contribution style

• I have read the contributing guidelines.

Contribution agreement

• I have signed the Open Information Security Foundation contribution agreement.

Changes

• User Guide update (not applicable; no user-facing configuration changes)
• JSON schema update (not applicable; no logging schema changes)
• Public Redmine ticket: https://redmine.openinfosecfoundation.org/issues/8776

Describe changes:

• Track the client SCID used by the current QUIC Client Initial on a UDP flow.
• When a Client Initial with a different SCID is received on the same flow, reset only the Initial parsing state and derive Initial keys from its DCID.
• Allow the later ClientHello and SNI to be parsed instead of being skipped by the existing hello-direction guard.

This is the minimal replacement for #15975. It preserves the existing QUIC flow and post-Initial handling while allowing a reused UDP five-tuple to inspect a later QUIC connection. The public reproducer and Suricata-Verify test are available in OISF/suricata-verify#3324.

Validation:

• Rust formatting check passed.
• Rust library tests passed (620 tests).
• The QUIC Suricata-Verify suite, including the new five-tuple reuse test, passed with 20 passed, 0 failed, and 1 version-dependent skip.
• On unpatched Suricata main, the new test fails the second-SNI and matching-alert checks as expected.
• Linux, macOS, Windows, and FreeBSD builds passed in the personal-fork matrix; unrelated personal-repository workflow failures were limited to credentials, hard-coded repository paths, or external download failures.

SV_REPO=https://github.com/jiangshaoqi/suricata-verify
SV_BRANCH=bug-8776-quic-five-tuple-reuse
SU_REPO=
SU_BRANCH=

[victorjulien]

Could you add a SV test to show the issue as well?

There also seems to be an issue with CI not running properly for this PR, I don't see why that happened. If you do a rebased PR we will try the CI again.

Thanks!

[xhon-pelushi]

Traced this against master. The direction makes sense — keying the "is this a new connection" decision off the client SCID is the natural signal available on a reused five-tuple. Three things I think need a second look before this lands, since the new block runs on attacker-controlled fields before anything validates them.

1. A bad version on a fresh SCID now clears working keys. quic_keys_initial() returns None for any version outside its known list (rust/src/quic/crypto.rs:158, the _ => return None arm), and the new block assigns it unconditionally:

self.keys = quic_keys_initial(u32::from(header.version), &header.dcid);

On master keys are only ever installed under if self.keys.is_none() && header.ty == QuicType::Initial (quic.rs:356), so a garbage version could never remove keys that already worked — the only teardown path was the server-direction Retry branch, and that one is latched by has_retried. After this change, a single client-direction Initial carrying an unknown version and any SCID not currently tracked sets self.keys = None, and from then on if self.keys.is_some() && !framebuf.is_empty() never runs, so the rest of the handshake goes uninspected. That looks like it hands back an evasion of roughly the shape this PR is closing. Guarding the assignment — only replace the keys when the new derivation actually succeeds, and only reset the rest of the state in that case — would avoid it:

if let Some(keys) = quic_keys_initial(u32::from(header.version), &header.dcid) {
    self.keys = Some(keys);
    // ...reset the rest here...
}

2. Zero-length SCIDs are not covered. header.scid is a Vec<u8> and RFC 9000 §17.2 explicitly permits a zero-length Source Connection ID. For a client that uses one, every Client Initial carries scid == [], so after the first packet self.client_scid is Some([]) and the comparison never fires again — a genuinely new connection on the reused five-tuple is not detected and the original problem stays. The first packet works only because None != Some([]). Whether that matters depends on how common zero-length client SCIDs are in the traffic you care about, but it is worth stating explicitly as a known limit rather than leaving it implicit, since the check reads as if it covers all clients.

3. The reset is reachable from the wrong direction. crypto_frag_tc and crypto_fraglen_tc are the server-direction CRYPTO reassembly, and they are cleared here on a client-direction packet. For a real new connection that is correct. But the trigger is an unauthenticated field on a UDP flow, so an injected client Initial with a random SCID flushes partial server-side reassembly at will — interleave one while a fragmented ServerHello is in flight and the reassembly never completes, so no SNI/JA3S comes out of it. Same reasoning applies to has_retried = false: that latch encodes the RFC 9000 17.2.5.2 "discard subsequent Retry packets" rule, and clearing it from the client direction re-opens the Retry branch, which itself sets keys = None. If the new-connection decision could be made on something with a bit more commitment behind it — say requiring that the Initial actually decrypts under the newly derived keys before tearing down the old state — the reset would no longer be free to trigger.

Two smaller notes:

• The pre-existing if self.keys.is_none() && header.ty == QuicType::Initial at quic.rs:356 is now mostly dead for the to_server case, since the new block has already set the keys — except when the derivation returned None, where it recomputes the same failing call. Once the guard in point 1 is in, these two paths probably want to be one.
• has_retried is reset but hello_tc/hello_ts are reset too, which is right; worth a comment saying explicitly that this is "forget everything about the previous connection on this five-tuple", so the field list does not drift out of sync with QuicState as fields get added.

I have not seen the private reproduction, so the above is from reading the state machine only — if any of it is already ruled out by the private test material, ignore accordingly.

[catenacyber]

Thanks for the PR.

Could you please create a redmine ticket for this work ?

[catenacyber]

Could you please create a redmine ticket for this work ?

Oh, there exists already https://redmine.openinfosecfoundation.org/issues/8776

[jiangshaoqi]

Could you add a SV test to show the issue as well?

There also seems to be an issue with CI not running properly for this PR, I don't see why that happened. If you do a rebased PR we will try the CI again.

Thanks!

Hello, Vector,

I shared a SV test previously through email. Is it ok to publish it in your official SV repository?

[catenacyber]

Hello, Vector,

I shared a SV test previously through email. Is it ok to publish it in your official SV repository?

Yes, you can share the SV test with a GitHub PR (I remember there is no confidential data in it)

[jiangshaoqi]

Added the Suricata-Verify reproducer in OISF/suricata-verify#3324. It fails on unpatched main because the second QUIC SNI event and matching alert are missing, and passes with this PR. I also updated SV_REPO and SV_BRANCH in the PR description.

[xhon-pelushi]

Good to see the reproducer land as OISF/suricata-verify#3324 — that covers the direction this PR fixes.

One thing worth pairing with it: the branch is still on 00e6ff20 from 2026-08-04, so the point I raised above about the unconditional key assignment is unchanged, and the new SV test would not catch it. The test asserts that a second SNI event appears; the regression I'm worried about is one where an event that used to appear stops appearing.

self.keys = quic_keys_initial(u32::from(header.version), &header.dcid);

quic_keys_initial() still ends in _ => { return None; } for any version outside its table (rust/src/quic/crypto.rs), and here the result is assigned unconditionally. So a single client-direction Initial carrying an unknown version and any SCID not currently tracked sets self.keys = None, after which if self.keys.is_some() && !framebuf.is_empty() never runs and the rest of the handshake goes uninspected. On master that cannot happen, because keys are only ever installed under if self.keys.is_none() && header.ty == QuicType::Initial.

If it's useful, the second SV case is a small variation on the one you just wrote:

1. Client Initial, valid version, SCID A → SNI event fires (same as today).
2. Client Initial on the same five-tuple, version 0x0a0a0a0a (or anything not in the salt table), SCID B.
3. Continue the first connection's handshake.

On master step 3 still produces its SNI/JA3S. With this PR as written, step 2 clears the keys and step 3 produces nothing. That makes the guard explicit rather than something a future refactor can quietly undo:

if let Some(keys) = quic_keys_initial(u32::from(header.version), &header.dcid) {
    self.keys = Some(keys);
    // ...reset the rest here...
}

Not blocking, and entirely your call whether it belongs in this PR or a follow-up — but since you're adding SV coverage anyway, this is the cheap moment to pin it.

[victorjulien]

@xhon-pelushi what model are you / do you use?

[xhon-pelushi]

I use all the available models, but for this one I reviewed it with Opus 5.

[victorjulien]

Please do a rebased new PR for this work, with the SV test correctly linked in the PR body so the test(s) get run. Don't forget the SV PR may need a rebase as well.

Thanks!

[jiangshaoqi]

Superseded by the rebased PR #16209, which addresses the review feedback and links the rebased SV PR OISF/suricata-verify#3324 through SV_BRANCH so the test is included in CI. Thank you for the review.