🛡️ Sentinel: [CRITICAL] Fix timing attack in public key verification#71
🛡️ Sentinel: [CRITICAL] Fix timing attack in public key verification#71
Conversation
**Vulnerability:** The `consumeInitiation` and `consumeInitiationFast` functions in `src/wireguard/noise.zig` used `std.mem.eql` to verify the decrypted `initiator_static` public key against the expected `remote_static` key. `std.mem.eql` is not a constant-time comparison and terminates early on a mismatch. This created a timing oracle that an attacker could exploit to determine the expected public key byte-by-byte. **Impact:** While leaking a public key is generally not as severe as leaking a private key, an attacker could potentially use this to verify the identities (public keys) expected by a node without knowing them in advance, leading to topology discovery or aiding in spoofing attempts during the Noise handshake. **Fix:** Replaced `std.mem.eql` with `std.crypto.timing_safe.eql` for comparing the `initiator_static` and `remote_static` public keys. This ensures the comparison time is constant regardless of where the mismatch occurs, completely eliminating the timing oracle. **Verification:** `zig build test` passes with no regressions. Co-authored-by: igorls <4753812+igorls@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a timing attack vulnerability in the WireGuard Noise IK handshake implementation. The consumeInitiation and consumeInitiationFast functions were comparing the decrypted initiator static public key against the expected remote static key using std.mem.eql, a non-constant-time comparison that could reveal the expected key byte-by-byte through timing differences. The fix replaces these with std.crypto.timing_safe.eql, consistent with the existing constant-time comparison used for MAC1 verification in verifyMac1.
Changes:
- Replace
std.mem.eqlwithstd.crypto.timing_safe.eqlinconsumeInitiationfor public key comparison - Replace
std.mem.eqlwithstd.crypto.timing_safe.eqlinconsumeInitiationFastfor public key comparison - Document the vulnerability and fix in
.jules/sentinel.md
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/wireguard/noise.zig |
Replaces non-constant-time std.mem.eql with std.crypto.timing_safe.eql for public key comparison in both handshake consumption functions |
.jules/sentinel.md |
Adds a new entry documenting the timing attack vulnerability, root cause, and the std.crypto.timing_safe.eql mitigation pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **Learning:** Helper functions often create temporary buffers on the stack which persist until overwritten. Unlike the main handshake state struct, these are ephemeral but critical. Zig does not automatically zero stack variables on return. | ||
| **Prevention:** Always use `defer std.crypto.secureZero(u8, &var);` immediately after declaring any variable that will hold sensitive key material, especially in crypto primitives. | ||
|
|
||
| ## 2025-05-24 - [CRITICAL] Timing Attack on Public Key Verification |
There was a problem hiding this comment.
The newly added sentinel entry is dated 2025-05-24, but the other recent entries in this file use dates in 2026 (e.g., 2026-02-24 on line 8 and 2026-05-24 on line 15). Given that the PR was created in March 2026, the year 2025 appears to be a typo that should be 2026. The date should reflect when the vulnerability was discovered and fixed.
|
Closing: superseded by docs audit fixes and direct commits to main. See PR #67 (merged) for codec bounds check, and direct commit for timing-safe fix in noise.zig. |
Understood. Acknowledging that this PR has been superseded and the fixes have already been merged to main. I will stop work on this task. |
🛡️ Sentinel: [CRITICAL] Fix timing attack in public key verification
Vulnerability:
The
consumeInitiationandconsumeInitiationFastfunctions insrc/wireguard/noise.zigusedstd.mem.eqlto verify the decryptedinitiator_staticpublic key against the expectedremote_statickey.std.mem.eqlis not a constant-time comparison and terminates early on a mismatch. This created a timing oracle that an attacker could exploit to determine the expected public key byte-by-byte.Impact:
While leaking a public key is generally not as severe as leaking a private key, an attacker could potentially use this to verify the identities (public keys) expected by a node without knowing them in advance, leading to topology discovery or aiding in spoofing attempts during the Noise handshake.
Fix:
Replaced
std.mem.eqlwithstd.crypto.timing_safe.eqlfor comparing theinitiator_staticandremote_staticpublic keys. This ensures the comparison time is constant regardless of where the mismatch occurs, completely eliminating the timing oracle.Verification:
zig build testpasses with no regressions.PR created automatically by Jules for task 16771674837567453665 started by @igorls