🛡️ Sentinel: [CRITICAL] Fix timing attack in remote static key verification#58
🛡️ Sentinel: [CRITICAL] Fix timing attack in remote static key verification#58
Conversation
…cation 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
Addresses a timing side-channel in the WireGuard/Noise responder handshake by switching remote static key verification to a constant-time equality check, preventing early-exit comparisons from leaking information via timing.
Changes:
- Replaced
std.mem.eqlwithstd.crypto.timing_safe.eqlwhen validating the decrypted initiator static key inconsumeInitiationandconsumeInitiationFast. - Added inline security comments documenting why constant-time comparison is required.
- Logged the vulnerability and prevention guidance 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 |
Uses constant-time comparison for remote static key verification during handshake consumption and documents the rationale. |
.jules/sentinel.md |
Adds a Sentinel entry describing the timing-attack class and the required prevention pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 2023-10-27 - [CRITICAL] Timing Attack on Remote 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`. `std.mem.eql` terminates early on the first mismatch, creating a timing oracle. This could allow an attacker to perform a timing attack to determine the expected remote static key during the handshake processing. | ||
| **Learning:** It is easy to accidentally use `std.mem.eql` for comparing public keys or decrypted public keys when validating expected peers, especially when the key itself is public but the expected value is part of the secret handshake state. | ||
| **Prevention:** Always use `std.crypto.timing_safe.eql` for any comparison involving cryptographic keys, even public keys, when the comparison occurs within a cryptographic protocol or handshake where timing could leak information about the internal state or expected peer. |
There was a problem hiding this comment.
The new Sentinel entry is dated 2023-10-27 but is appended after 2026 entries, which breaks the apparent chronological ordering of this log and makes it harder to scan. Consider either using the actual discovery/fix date for this entry, or moving it to the correct position in the file based on date order.
|
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 attack in remote static key verification
Vulnerability:
The
consumeInitiationandconsumeInitiationFastfunctions insrc/wireguard/noise.zigusedstd.mem.eqlto compare the decryptedinitiator_statickey againstself.remote_static.std.mem.eqlis not constant-time and terminates early upon finding a mismatch, exposing a timing oracle. This could allow an attacker to determine the expected remote static key by observing the timing of handshake rejections.Impact:
A timing attack could theoretically leak the expected
remote_statickey during the handshake process, which is a critical security property of the Noise IK protocol.Fix:
Replaced
std.mem.eql(u8, ...)withstd.crypto.timing_safe.eql([32]u8, ...)to ensure that the static key comparison is performed in constant time, eliminating the timing side-channel. Added security documentation comments explaining the change.Verification:
zig buildpasseszig build testpasseszig build -Doptimize=ReleaseSafepassesPR created automatically by Jules for task 17018935198270756533 started by @igorls