Reject non-contributory Diffie-Hellman results in handshake (all-zero shared secret) - #142
Reject non-contributory Diffie-Hellman results in handshake (all-zero shared secret)#142faern wants to merge 6 commits into
Conversation
e717700 to
fc01bda
Compare
hulthe
left a comment
hulthe
left a comment
There was a problem hiding this comment.
@hulthe made 1 comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on faern).
gotatun/src/noise/mod.rs line 403 at r1 (raw file):
Err(error) => { log::debug!("Not sending handshake_initiation: {error:?}"); return None;
I realize that kernel wireguard also opted to check for low-order peer keys in the handshake initiation step, instead of when adding the peer (linux#11a7686a). I would like to discuss it IRL though, since I don't quite get the reasoning behind it.
|
It might make more sense to return a wrapped type here so we can be aware of shared secret checking at the type level. Functions could then potentially be modified to take the checked type ensuring we couldn't accidentally pass an unchecked secret key. |
fc01bda to
f6b84f4
Compare
faern
left a comment
faern
left a comment
There was a problem hiding this comment.
@faern made 1 comment.
Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on hulthe and zorusty).
gotatun/src/noise/handshake.rs line 138 at r1 (raw file):
Previously, zorusty wrote…
It might make more sense to return a wrapped type here so we can be aware of shared secret checking at the type level. Functions could then potentially be modified to take the checked type ensuring we couldn't accidentally pass an unchecked secret key.
I took a stab at this. Became quite a lot of code changes. Regard the currently pushed commits as draft and ideas for how this could be expressed at the type level. Not sure yet if I like this change or not.
|
Previously, faern (Linus Färnstrand) wrote…
I think the current draft handles the type level enforcement great. By storing the checked type in NoiseParams we guarantee that the check has happened during the handshake process. |
5e8788f to
b4acd62
Compare
Add tests asserting that a handshake is aborted when a Diffie-Hellman yields a non-contributory (low-order / all-zero) result: one for a low-order peer static key (the initiator must emit no initiation) and one for a low-order peer ephemeral key in a received initiation. The test vectors are the Curve25519 low-order points from the WireGuard kernel selftest. These tests assert only that the handshake fails; they pass once the next commit adds the checks. They are committed first so the failure (a handshake wrongly succeeding) can be reproduced on the unfixed code.
Guard every X25519 DH in the Noise handshake against non-contributory (low-order / all-zero) shared secrets, aborting the handshake when one occurs. Previously no DH output was checked, so a low-order static or ephemeral peer key was accepted and fed into key derivation, unlike the Linux kernel WireGuard implementation, which rejects every such DH. Covers all ephemeral DHs (es/ee/se) plus the precomputed static-static (ss). The static-static is still computed and cached at configuration time and only checked at handshake time. As a result, a low-order peer static key is accepted at config and instead fails the handshake (no initiation is emitted). This keeps the API unchanged, and it mirrors the kernel mechanism (mix_dh / mix_precomputed_dh). The decision to add this check weighs several inputs: the Noise Protocol spec (section 12.1) permits but discourages the all-zero check; RFC 7748 (sections 6 and 7) permits it; the WireGuard whitepaper does not specify any behaviour here; and both reference implementations, the Linux kernel and wireguard-go, perform it as an extra security measure. We follow the reference implementations. Adds the WireGuardError::InvalidSharedSecret variant returned by the checks, and tightens the ephemeral test assertion to match on it. This is a breaking change, since the WireGuardError enum is not non_exhaustive
The public key is always derivable from the private key, so passing it separately only enabled a consistency assert that can no longer fail. Derive it internally instead, matching how NoiseParams::new already works. Removes the parameter from Tunn::set_static_private down through Handshake and NoiseParams. This changes the public Tunn::set_static_private signature, a breaking change.
d485f50 to
4ac040c
Compare
parse_handshake_anon cloned the device static secret on every inbound handshake-init packet to wrap it in dh::StaticSecret. Take a reference to an already-wrapped key instead, and convert once per receive task in handle_incoming, outside the packet loop. This drops the per-packet, attacker-reachable secret copy. The function becomes pub(crate) (dh::StaticSecret is a crate-internal type) and is gated on its only consumers, the device feature and tests.
4ac040c to
20a22d4
Compare
Introduce `PeerPublicKey`, a newtype over `x25519::PublicKey` whose only constructor (`PeerPublicKey::new`) rejects low-order (non-contributory) Curve25519 points. Holding the type proves the key passed the check, so low-order peer static keys are now rejected up front when configuring a peer instead of silently failing every handshake. Because the peer static key is proven contributory, the handshake code treats it as infallible: the precomputed static-static secret is no longer an `Option`, DH against it uses the new infallible `dh_validated` (no `.expect()`), and `format_handshake_initiation` no longer returns a `Result`. Wire-supplied ephemeral keys keep the fallible checked DH path, since they arrive from the wire and cannot be validated in advance. `Tunn::new`, `Tunn::new_with_rng` and `Peer::new` now take a `PeerPublicKey`. This is a breaking change.
20a22d4 to
468604f
Compare
4 participants
Adds a check that rejects non-contributory (low-order / all-zero) X25519 Diffie-Hellman results during the Noise handshake. Every DH is now guarded at handshake time: the ephemeral DHs (es, ee, se) and the
precomputed static-static (ss). A non-contributory result aborts the handshake (
WireGuardError::InvalidSharedSecret); on the initiator side this means no initiation is emitted.Previously no DH output was validated, so a low-order peer key (static or ephemeral) was accepted and fed into key derivation.
Why
A low-order input point collapses the DH to a fixed, publicly-known value, contributing no entropy. The Noise spec (§12.1) permits but actually discourages this all-zero check, and the WireGuard whitepaper is silent - but both reference implementations (Linux kernel and wireguard-go) perform it as an extra hardening measure, so we match them. This is hardening / kernel parity, not a remotely exploitable confidentiality break.
Links to where the Linux kernel and wireguard-go implementations implement this check:
This change is