fix: constant-time equality for secret-bearing types (#43)#48
Conversation
Secret-bearing types in the public API used derived (non-constant-time) `PartialEq`, allowing timing side-channels when comparing secrets. Replace the derived `PartialEq` with manual implementations backed by `subtle::ConstantTimeEq`: - `SharedSecret` in `src/kem/mod.rs` - `SecretKey` and `UserSecretKey` in `boyen_waters`, `waters` and `waters_naccache` Each type now implements `ConstantTimeEq` (delegating to the field(s)' `ct_eq`) and `PartialEq` via `self.ct_eq(other).into()`, matching the pattern already used for `CipherText` in the `cgw` module. Refs GHSA-whr9-835c-9m5j, closes #43 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 1)
Review Dobby 2 found no actionable code bugs (build + 21/21 tests + clippy all green). The ConstantTimeEq/PartialEq implementations are correct, constant-time, and match both the requested pattern and the cgw reference. The per-rule compliance pass surfaced 2 issues to fix before this can be approved:
1. PR title fails the Conventional Commit check — blocking, CI is red.
The title security: constant-time equality for secret-bearing types (#43) uses the type security:, which is not one of the default conventional-commit types accepted by amannn/action-semantic-pull-request@v6 (build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test) — the repo's pr-title.yml sets no custom types. The "Conventional Commit" check is currently failing. Rename to a valid type:
fix: constant-time equality for secret-bearing types (#43)
Fix via gh api -X PATCH repos/encryption4all/ibe/pulls/48 -f title="fix: ..." (the action re-runs on title edit).
2. No reviewer assigned (pr-review-assignments).
This bot-authored PR has an empty reviewer list — assign @rubenhensen.
Compliant on everything else: cargo fmt --check clean; the existing eq_serialize_deserialize macro tests already exercise the equality path (behavior-preserving refactor — new tests not required); Closes #43 present; PR body appropriately concise; no scope drift vs. the request; consistent with the ibe deep-dive notes. Draft state is fine to keep while the title check is red.
Note: cgw::SecretKey/cgw::UserSecretKey still derive variable-time Eq/PartialEq, but that is pre-existing and out of scope for #48 — not a blocker here.
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 2) — ✅ sign-off
Both cycle-1 blockers are resolved: the PR title was renamed to a valid Conventional Commit type (fix: — the "Conventional Commit" check is now green), and reviewer @rubenhensen is assigned. All CI is green (Clippy, Lint / cargo fmt --check, Tests on ubuntu/macOS/windows, both wasm no-std targets, Conventional Commit).
Per-rule compliance pass (Haiku fan-out across the rule set) merged with Review Dobby 2's diff review: no actionable findings. The diff implements exactly the requested pattern — the derived PartialEq replaced by a manual impl subtle::ConstantTimeEq delegating to the fields' ct_eq, plus an impl PartialEq whose eq calls self.ct_eq(other).into() — on SharedSecret (src/kem/mod.rs) and SecretKey/UserSecretKey in boyen_waters.rs, waters.rs, and waters_naccache.rs, mirroring the ibe::cgw reference. subtle was already a dependency (no Cargo.toml change). Behaviour-preserving refactor; the existing eq_serialize_deserialize macro tests already exercise the equality path (21/21 pass). Closes #43 present; PR body concise; scope matches the request.
Non-blocking follow-up note (out of scope for #43): cgw::SecretKey/cgw::UserSecretKey (src/ibe/cgw.rs) and kv1::SecretKey/kv1::UserSecretKey (src/kem/kiltz_vahlis_one.rs) still #[derive(.., PartialEq)] (variable-time). These types are untouched by this PR and outside issue #43's scope, so they are not blockers here — flagging for a possible follow-up under advisory GHSA-whr9-835c-9m5j; maintainer to decide.
Signing off and flipping this out of draft to ready-for-review.
Summary
Secret-bearing types in the public API used the derived, non-constant-time
PartialEq, which can leak information about secret material through comparison timing. This PR replaces those derives with manualsubtle::ConstantTimeEq-backed implementations.Tracked in security advisory GHSA-whr9-835c-9m5j. Closes #43.
Changes
For each affected type, the derived
PartialEqis replaced by:impl ConstantTimeEqthat delegates to the field(s)'ct_eq, andimpl PartialEqwhoseeqcallsself.ct_eq(other).into().Types updated:
SharedSecret—src/kem/mod.rsSecretKey,UserSecretKey—src/ibe/boyen_waters.rsSecretKey,UserSecretKey—src/ibe/waters.rsSecretKey,UserSecretKey—src/ibe/waters_naccache.rsThis mirrors the pattern already applied to
CipherTextin theibe::cgwmodule.subtlewas already a dependency, so noCargo.tomlchange was needed.These types are still compared via
assert_eq!in the shared test macros (eq_serialize_deserialize), soPartialEqis retained (implemented in constant time) rather than removed.Testing
cargo build --all-features— cleancargo test --all-features— 21/21 passcargo clippy --all-features— no warnings🤖 Generated with Claude Code