Skip to content

fix(rpc): use constant-time comparison for cookie auth#10567

Open
dingledropper wants to merge 1 commit into
ZcashFoundation:mainfrom
dingledropper:fix/rpc-cookie-ct-eq
Open

fix(rpc): use constant-time comparison for cookie auth#10567
dingledropper wants to merge 1 commit into
ZcashFoundation:mainfrom
dingledropper:fix/rpc-cookie-ct-eq

Conversation

@dingledropper
Copy link
Copy Markdown
Contributor

Motivation

Closes #10546.

Cookie::authenticate in zebra-rpc/src/server/cookie.rs compared the supplied password against the server cookie using String == String, which lowers to a memcmp-style block-wise compare with early-exit on the first mismatching word. A network-side adversary observing response timing could recover the 44-character base64 cookie byte-by-byte.

Cookie auth is the only gate covering all 64 RPC methods (including mining control), and enable_cookie_auth: true is the default whenever RPC is enabled, so the timing oracle is reachable pre-auth from anyone with TCP access to the RPC port.

CWE-208 (Observable Timing Discrepancy).

Solution

Replace the comparison with subtle::ConstantTimeEq, per @mpguerra's exact suggestion in #10546:

use subtle::ConstantTimeEq;

pub fn authenticate(&self, passwd: String) -> bool {
    if passwd.len() != self.0.len() {
        return false;
    }
    passwd.as_bytes().ct_eq(self.0.as_bytes()).into()
}

The length short-circuit is intentional — only the cookie contents are secret, not its length, which is fixed at 44 bytes by Cookie::default (base64 of 32 random bytes).

This mirrors zcashd's TimingResistantEqual in src/httprpc.cpp (originally Bitcoin Core PR #6390, 2015).

subtle as a direct dep

subtle was previously available transitively via bellmansapling-cryptozcash_primitiveszcash_proofszebra-consensus, but Rust's import-visibility rule requires direct declaration in Cargo.toml for use subtle::ConstantTimeEq; to compile. Adding it to [workspace.dependencies] and consuming it via subtle = { workspace = true } in zebra-rpc/Cargo.toml follows the workspace's existing dep-management convention; no version churn (already pinned to 2.6.1 in Cargo.lock from the transitive chain).

Test evidence

  • cargo fmt --all -- --check
  • cargo build -p zebra-rpc
  • cargo test -p zebra-rpc --lib server::cookie ✓ (4 new tests pass: exact match, wrong content same length, wrong length, prefix-only match)
  • cargo clippy -p zebra-rpc --all-targets -- -D warnings
  • Verified locally on origin/main (commit d4cd662c).

The new cookie.rs test module (#[cfg(test)] mod tests) covers:

  • authenticate_accepts_exact_match — sanity check.
  • authenticate_rejects_wrong_content_same_length — three same-length-but-wrong guesses; this is the bug class — without ct_eq the compare would short-circuit on the first wrong byte.
  • authenticate_rejects_wrong_length — shorter, longer, and empty guesses; verifies the length short-circuit.
  • authenticate_rejects_prefix_only_match — regression guard for the timing-oracle class: a 43-byte prefix match with a different last byte must be rejected exactly like any other wrong guess.

Context

CHANGELOG

Updated CHANGELOG.md under [Unreleased] → Security with the standard form referencing #10546.

AI disclosure

Patch and PR description drafted with assistance from Claude (Anthropic), following @mpguerra's exact code snippet in #10546 plus the test-coverage shape ZF reviewers typically request. I reviewed each step and am the responsible author.

The previous `*passwd == self.0` is byte-wise with early-exit on the first
mismatch, leaking the cookie one byte at a time via response timing. With
cookie auth as the only gate covering all 64 RPC methods (including mining
control), and `enable_cookie_auth: true` as the default whenever RPC is
enabled, the timing oracle is reachable pre-auth from anyone with TCP access
to the RPC port.

Switch to `subtle::ConstantTimeEq` over the cookie byte slice. The length
check is intentionally early-exit — only the cookie *contents* are secret,
not its length, which is fixed by `Cookie::default`. `subtle` is already in
`Cargo.lock` (v2.6.1) as a transitive dep, so this only adds a top-level
declaration.

CWE-208 (Observable Timing Discrepancy).
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use constant-time comparison for RPC cookie authentication

2 participants