From ded348d58656a6a3794181d6d010d5c4fb373944 Mon Sep 17 00:00:00 2001 From: Mathias Hall-Andersen Date: Sun, 29 Mar 2020 22:56:57 +0200 Subject: Added checks for zero shared-secret To mirror the behavior from the kernel module, as per private correspondence with Jason. --- Cargo.toml | 2 +- src/wireguard/handshake/noise.rs | 44 ++++++++++++++++++++++++++++++++-------- src/wireguard/handshake/peer.rs | 7 +++---- src/wireguard/handshake/types.rs | 2 ++ 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0402cd6..a7f033d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "wireguard-rs" -version = "0.1.2" +version = "0.1.3" authors = ["Mathias Hall-Andersen "] edition = "2018" diff --git a/src/wireguard/handshake/noise.rs b/src/wireguard/handshake/noise.rs index fb673eb..beb99c2 100644 --- a/src/wireguard/handshake/noise.rs +++ b/src/wireguard/handshake/noise.rs @@ -1,8 +1,7 @@ use std::time::Instant; // DH -use x25519_dalek::PublicKey; -use x25519_dalek::StaticSecret; +use x25519_dalek::{PublicKey, StaticSecret, SharedSecret}; // HASH & MAC use blake2::Blake2s; @@ -215,6 +214,21 @@ mod tests { } } +// Computes an X25519 shared secret. +// +// This function wraps dalek to add a zero-check. +// This is not recommended by the Noise specification, +// but implemented in the kernel with which we strive for absolute equivalent behavior. +#[inline(always)] +fn shared_secret(sk: &StaticSecret, pk: &PublicKey) -> Result { + let ss = sk.diffie_hellman(pk); + if ss.as_bytes().ct_eq(&[0u8; 32]).into() { + Err(HandshakeError::InvalidSharedSecret) + } else { + Ok(ss) + } +} + pub(super) fn create_initiation( rng: &mut R, keyst: &KeyState, @@ -224,6 +238,12 @@ pub(super) fn create_initiation( msg: &mut NoiseInitiation, ) -> Result<(), HandshakeError> { log::debug!("create initiation"); + + // check for zero shared-secret (see "shared_secret" note). + if peer.ss.ct_eq(&[0u8; 32]).into() { + return Err(HandshakeError::InvalidSharedSecret); + } + clear_stack_on_return(CLEAR_PAGES, || { // initialize state @@ -253,7 +273,7 @@ pub(super) fn create_initiation( // (C, k) := Kdf2(C, DH(E_priv, S_pub)) - let (ck, key) = KDF2!(&ck, eph_sk.diffie_hellman(&pk).as_bytes()); + let (ck, key) = KDF2!(&ck, shared_secret(&eph_sk, &pk)?.as_bytes()); // msg.static := Aead(k, 0, S_pub, H) @@ -270,6 +290,7 @@ pub(super) fn create_initiation( // (C, k) := Kdf2(C, DH(S_priv, S_pub)) + let (ck, key) = KDF2!(&ck, &peer.ss); // msg.timestamp := Aead(k, 0, Timestamp(), H) @@ -304,6 +325,7 @@ pub(super) fn consume_initiation<'a, O>( msg: &NoiseInitiation, ) -> Result<(&'a Peer, PublicKey, TemporaryState), HandshakeError> { log::debug!("consume initiation"); + clear_stack_on_return(CLEAR_PAGES, || { // initialize new state @@ -322,7 +344,7 @@ pub(super) fn consume_initiation<'a, O>( // (C, k) := Kdf2(C, DH(E_priv, S_pub)) let eph_r_pk = PublicKey::from(msg.f_ephemeral); - let (ck, key) = KDF2!(&ck, keyst.sk.diffie_hellman(&eph_r_pk).as_bytes()); + let (ck, key) = KDF2!(&ck, shared_secret(&keyst.sk, &eph_r_pk)?.as_bytes()); // msg.static := Aead(k, 0, S_pub, H) @@ -337,6 +359,12 @@ pub(super) fn consume_initiation<'a, O>( let peer = device.lookup_pk(&PublicKey::from(pk))?; + // check for zero shared-secret (see "shared_secret" note). + + if peer.ss.ct_eq(&[0u8; 32]).into() { + return Err(HandshakeError::InvalidSharedSecret); + } + // reset initiation state *peer.state.lock() = State::Reset; @@ -415,11 +443,11 @@ pub(super) fn create_response( // C := Kdf1(C, DH(E_priv, E_pub)) - let ck = KDF1!(&ck, eph_sk.diffie_hellman(&eph_r_pk).as_bytes()); + let ck = KDF1!(&ck, shared_secret(&eph_sk, &eph_r_pk)?.as_bytes()); // C := Kdf1(C, DH(E_priv, S_pub)) - let ck = KDF1!(&ck, eph_sk.diffie_hellman(&pk).as_bytes()); + let ck = KDF1!(&ck, shared_secret(&eph_sk, &pk)?.as_bytes()); // (C, tau, k) := Kdf3(C, Q) @@ -497,11 +525,11 @@ pub(super) fn consume_response<'a, O>( // C := Kdf1(C, DH(E_priv, E_pub)) let eph_r_pk = PublicKey::from(msg.f_ephemeral); - let ck = KDF1!(&ck, eph_sk.diffie_hellman(&eph_r_pk).as_bytes()); + let ck = KDF1!(&ck, shared_secret(&eph_sk, &eph_r_pk)?.as_bytes()); // C := Kdf1(C, DH(E_priv, S_pub)) - let ck = KDF1!(&ck, keyst.sk.diffie_hellman(&eph_r_pk).as_bytes()); + let ck = KDF1!(&ck, shared_secret(&keyst.sk, &eph_r_pk)?.as_bytes()); // (C, tau, k) := Kdf3(C, Q) diff --git a/src/wireguard/handshake/peer.rs b/src/wireguard/handshake/peer.rs index f4d15fc..1636e62 100644 --- a/src/wireguard/handshake/peer.rs +++ b/src/wireguard/handshake/peer.rs @@ -18,10 +18,9 @@ use super::types::*; const TIME_BETWEEN_INITIATIONS: Duration = Duration::from_millis(20); -/* Represents the recomputation and state of a peer. - * - * This type is only for internal use and not exposed. - */ +// Represents the state of a peer. +// +// This type is only for internal use and not exposed. pub(super) struct Peer { // opaque type which identifies a peer pub opaque: O, diff --git a/src/wireguard/handshake/types.rs b/src/wireguard/handshake/types.rs index ed2fcbb..ea827b0 100644 --- a/src/wireguard/handshake/types.rs +++ b/src/wireguard/handshake/types.rs @@ -40,6 +40,7 @@ pub enum HandshakeError { UnknownPublicKey, UnknownReceiverId, InvalidMessageFormat, + InvalidSharedSecret, OldTimestamp, InvalidState, InvalidMac1, @@ -50,6 +51,7 @@ pub enum HandshakeError { impl fmt::Display for HandshakeError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + HandshakeError::InvalidSharedSecret => write!(f, "Zero shared secret"), HandshakeError::DecryptionFailure => write!(f, "Failed to AEAD:OPEN"), HandshakeError::UnknownPublicKey => write!(f, "Unknown public key"), HandshakeError::UnknownReceiverId => { -- cgit v1.2.3-59-g8ed1b