Skip to content

Add ratchet counter DoS protection and RotateChannelKey authorization#134

Merged
intendednull merged 3 commits into
mainfrom
claude/code-quality-review-tracking-issue-60H24
Apr 11, 2026
Merged

Add ratchet counter DoS protection and RotateChannelKey authorization#134
intendednull merged 3 commits into
mainfrom
claude/code-quality-review-tracking-issue-60H24

Conversation

@intendednull
Copy link
Copy Markdown
Owner

@intendednull intendednull commented Apr 11, 2026

Closes #109
Closes #110
Progresses #108

Summary

This PR addresses two critical security findings from the code quality review tracked in #108:

  1. [state] RotateChannelKey has no permission or membership check #109: Restricts RotateChannelKey events to users with ManageChannels permission and adds defense-in-depth membership validation. Before this, any outsider with a valid signature could inject arbitrary encrypted-key bytes into ServerState.channel_keys.
  2. [crypto] Bound ratchet_counter in open_content to fix CPU DoS #110: Adds DoS protection against unbounded ratchet counter derivation by introducing a bounded decryption function with a configurable lookahead window. Before this, one malformed packet with ratchet_counter = u64::MAX could freeze every recipient's CPU for ~584 000 years on a single core.

Key Changes

Crypto Module (crates/crypto/src/lib.rs)

  • New error variant: RatchetCounterOutOfRange { claimed, max } to report when a sealed packet's counter exceeds acceptable bounds
  • New constant: MAX_RATCHET_LOOKAHEAD = 1024 defines the maximum counter advance a receiver will accept before AEAD verification
  • New function: open_content_bounded(sealed, key, current_counter) performs bounded decryption:
    • Rejects packets with ratchet_counter > current_counter + MAX_RATCHET_LOOKAHEAD before any HKDF work
    • Prevents CPU DoS where an attacker could force u64::MAX iterations of key derivation
    • Allows legitimate catch-up within the lookahead window
    • Bypasses bounds check when ratchet_counter == 0 (backwards compatibility)
  • Refactored: open_content() now wraps open_content_bounded() with current_counter = 0 for backwards compatibility
  • Comprehensive tests: Five regression tests covering huge counter rejection, lookahead boundary enforcement, legitimate catch-up, shim rejection above lookahead, and legacy zero-counter behavior

State Module (crates/state/src/materialize.rs)

  • Permission check: Added RotateChannelKey to the list of events requiring ManageChannels permission
  • Defense-in-depth: Added explicit membership validation in apply_mutation for RotateChannelKey events to reject non-members even if permission checks are bypassed
  • Documentation: Updated comments to reflect that encryption events now have permission requirements

State Tests (crates/state/src/tests.rs)

  • Three regression tests for [state] RotateChannelKey has no permission or membership check #109:
    • rotate_channel_key_by_outsider_is_rejected: Verifies outsiders cannot inject key material
    • rotate_channel_key_by_member_without_manage_channels_is_rejected: Verifies permission enforcement
    • rotate_channel_key_by_admin_still_works: Sanity check that legitimate admins can still rotate keys

Implementation Details

  • The ratchet counter bounds check uses saturating_add to safely handle current_counter near u64::MAX
  • The lookahead window (1024 messages) is deliberately generous to allow receivers to catch up after missing bursts
  • The zero-counter special case preserves backwards compatibility with legacy sealed content that doesn't use ratcheting
  • Permission checks are layered: required_permission() provides the primary gate, and membership validation provides defense-in-depth
  • Neither code path has production callers today — the features are dormant at the emission level, so these fixes pre-empt live vulnerabilities rather than patching an exploited surface

Test plan

  • cargo fmt --check — clean
  • cargo clippy --workspace -- -D warnings — clean
  • cargo test --workspace — 0 failures (30 crypto tests, 155 state tests, all other crates green)
  • cargo check --target wasm32-unknown-unknown — clean for all library crates
  • Five new crypto regression tests pass in < 10ms total (bounds check returns instantly even for u64::MAX)
  • Three new state regression tests pass

https://claude.ai/code/session_011cBQL95mF4j2DGCeLgxrsr

claude added 3 commits April 10, 2026 20:43
Closes #109. Before this, EventKind::RotateChannelKey fell into the
catch-all `_ => None` arm of required_permission(), so any author —
including a fresh outsider identity who had never interacted with the
server — could emit a signed RotateChannelKey and inject arbitrary
encrypted-key entries into ServerState.channel_keys.

Map RotateChannelKey to Permission::ManageChannels so only admins and
members with an explicit grant pass the check, and add a
defense-in-depth membership assertion inside the apply_mutation arm.

Regression coverage:
- rotate_channel_key_by_outsider_is_rejected: fresh Mallory identity
  attempts injection; her key never appears in state.channel_keys.
- rotate_channel_key_by_member_without_manage_channels_is_rejected:
  Bob has SendMessages but not ManageChannels; his rotate is rejected.
- rotate_channel_key_by_admin_still_works: sanity check the legitimate
  admin rotation path remains unchanged.

https://claude.ai/code/session_011cBQL95mF4j2DGCeLgxrsr
Post-#109, RotateChannelKey (the only encryption event) is
permission-checked rather than unrestricted, so it no longer belongs
in the fall-through comment.

https://claude.ai/code/session_011cBQL95mF4j2DGCeLgxrsr
Closes #110. Before this, open_content called derive_message_key with
a fully attacker-controlled ratchet_counter, which looped from 1 up
to the claimed value performing two HKDF-Expand operations per step,
all before AEAD verification. Measured cost: 1e6 counter = 1.0s,
u64::MAX ≈ 584 000 years. Any peer subscribed to a channel topic
could freeze every recipient with a single malformed packet.

Introduce open_content_bounded(sealed, key, current_counter) which
rejects sealed.ratchet_counter > current_counter + MAX_RATCHET_LOOKAHEAD
(1024) before any HKDF work. The existing open_content becomes a thin
shim that calls open_content_bounded with current_counter=0, so any
packet with a counter above 1024 is rejected without the caller
needing to track state. Callers that track per-channel ratchet state
can migrate to open_content_bounded to widen the accepted window as
they process messages.

A new CryptoError::RatchetCounterOutOfRange { claimed, max } variant
surfaces the rejection reason. The bounds check is skipped for
ratchet_counter == 0 (legacy no-ratchet path) to preserve backwards
compatibility with SealedContent that was never ratcheted.

Regression coverage:
- open_content_rejects_huge_ratchet_counter: u64::MAX counter is
  rejected with RatchetCounterOutOfRange and returns in well under
  500ms (the unbounded path could not complete in any test timeout).
- open_content_bounded_rejects_above_lookahead: verifies the exact
  error payload including claimed and max.
- open_content_bounded_accepts_within_lookahead: counter=50 still
  decrypts fine from current_counter=0.
- open_content_shim_rejects_counter_above_lookahead: the zero-arg
  shim rejects MAX_RATCHET_LOOKAHEAD+1.
- open_content_bounded_counter_zero_bypasses_bounds_check: legacy
  ratchet_counter=0 path unaffected.

No production code calls open_content today — the feature is dormant
at the emission level — so no callers needed migration.

https://claude.ai/code/session_011cBQL95mF4j2DGCeLgxrsr
@intendednull intendednull merged commit 5cbb4fd into main Apr 11, 2026
4 checks passed
@intendednull intendednull deleted the claude/code-quality-review-tracking-issue-60H24 branch April 11, 2026 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[crypto] Bound ratchet_counter in open_content to fix CPU DoS [state] RotateChannelKey has no permission or membership check

2 participants