🛡️ Sentinel: [CRITICAL] Fix timing leak in initiator static key verification#63
🛡️ Sentinel: [CRITICAL] Fix timing leak in initiator static key verification#63
Conversation
…ication 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 fixes a timing side-channel vulnerability in WireGuard's Noise IK handshake by replacing std.mem.eql with std.crypto.timing_safe.eql when comparing the decrypted initiator static key against the expected remote static key. This prevents a timing oracle that could leak information about the expected public key during the Handshake Initiation phase.
Changes:
- Replaced non-constant-time
std.mem.eqlcomparisons withstd.crypto.timing_safe.eqlin bothconsumeInitiationandconsumeInitiationFastfunctions - Added a new entry to
.jules/sentinel.mddocumenting the vulnerability, learning, and prevention strategy
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/wireguard/noise.zig |
Replaced std.mem.eql with std.crypto.timing_safe.eql for constant-time comparison of static keys in two handshake consumption functions |
.jules/sentinel.md |
Added security audit entry documenting the timing attack vulnerability and its fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 2026-05-25 - [CRITICAL] Timing Attack on Initiator Static Key Verification | ||
| **Vulnerability:** The functions `consumeInitiation` and `consumeInitiationFast` in `src/wireguard/noise.zig` used `std.mem.eql` to compare the decrypted `initiator_static` key against `self.remote_static`. This exposed a timing oracle during the Handshake Initiation phase, potentially allowing an attacker to deduce the expected static key via a timing side-channel. | ||
| **Learning:** Checking the equality of sensitive values, including cryptographic public keys, should always be done in constant time to prevent leaking information about the expected state. Standard library memory equality checks terminate early. | ||
| **Prevention:** Always use `std.crypto.timing_safe.eql` to compare cryptographic key materials such as decrypted identity keys, shared secrets, signatures, and MACs. |
There was a problem hiding this comment.
Minor inconsistency: The earlier entry on line 6 refers to the API as std.crypto.utils.timingSafeEql, while this new entry on line 23 refers to it as std.crypto.timing_safe.eql. The correct API (as used in the actual code at line 634 of noise.zig) is std.crypto.timing_safe.eql. The earlier entry (line 6) appears to be the one that's wrong, but since this new entry will be read alongside it, it may be worth noting the discrepancy or correcting line 6 in this PR for consistency.
|
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 work is now obsolete and stopping work on this task. |
🛡️ Sentinel: [CRITICAL] Fix timing leak in initiator static key verification
Vulnerability: The functions
consumeInitiationandconsumeInitiationFastinsrc/wireguard/noise.zigusedstd.mem.eqlto compare the decryptedinitiator_statickey againstself.remote_static. This exposed a timing oracle during the Handshake Initiation phase, potentially allowing an attacker to deduce the expected static key via a timing side-channel.Fix: Replaced
std.mem.eql(u8, ...)withstd.crypto.timing_safe.eql([32]u8, ...)to ensure that cryptographic key material is compared in constant time, thus removing the timing side-channel.Verification: Verified via
zig build,zig build test, andzig build -Doptimize=ReleaseSafe. All tests passed, ensuring no regressions.PR created automatically by Jules for task 8421512774388262819 started by @igorls