Reject non-contributory Diffie-Hellman results in handshake (all-zero shared secret)

CHANGELOG.md

@@ -9,11 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 - Add `noise::TimerParams` for tuning WireGuard timers. This is useful for obfuscation, but note
   that tweaking timers will cause the tunnel to deviate from the WireGuard spec.
+- Add `PeerPublicKey`, a peer static public key validated as contributory (not a
+  low-order Curve25519 point), and the `InvalidPeerKey` error returned when
+  validation fails. Construct one with `PeerPublicKey::new`, or
+  `PeerPublicKey::from_secret` for a key derived from a secret you hold.
 
 ### Changed
 - Enlarge the anti-replay sliding window from 1024 to 8192 packets, matching the
   Linux kernel and wireguard-go. Tolerates more packet reordering before dropping
   legitimate packets. Costs ~7 KiB more memory per peer.
+- Remove the redundant `static_public` parameter from `Tunn::set_static_private`;
+  it is now derived from the private key. This is a breaking change.
+- `Tunn::new`, `Tunn::new_with_rng` and `Peer::new` now take a `PeerPublicKey`
+  instead of an `x25519::PublicKey`, rejecting low-order (non-contributory) peer
+  keys up front. This is a breaking change.
 
 ### Fixed
 - Add missing jitter for handshakes initiated due to not receiving any packets.
@@ -30,6 +39,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Security
 - Enforce the `Reject-After-Messages` limit on the transport data counter, so a
   session is retired before its AEAD nonce can wrap and repeat.
+- Abort the handshake on a non-contributory (low-order / all-zero) Curve25519
+  Diffie-Hellman result, matching the Linux kernel and wireguard-go. This guards
+  the receive path, where the peer ephemeral key arrives from the wire. This adds
+  `WireGuardError::InvalidSharedSecret` which is a breaking change. Low-order peer
+  *static* keys are instead rejected when configuring a peer (see Changed).
 
 #### Linux
 - Fix a remotely triggerable denial of service in the `recvmmsg` receive path.

gotatun/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "gotatun"
 description = "an implementation of the WireGuard® protocol designed for portability and speed"
-version = "0.7.1"
+version = "0.8.0"
 authors.workspace = true
 rust-version.workspace = true
 edition.workspace = true

gotatun/src/device/configure.rs

@@ -154,7 +154,7 @@ impl<T: DeviceTransports> DeviceRead<'_, T> {
     /// Return all peers on the device
     pub async fn peers(&self) -> Vec<PeerStats> {
         let mut peers = vec![];
-        for (pubkey, peer) in self.device.peers.iter() {
+        for (pubkey, peer) in &self.device.peers {
             let p = peer.lock().await;
 
             #[cfg(feature = "daita")]
@@ -270,7 +270,7 @@ impl<T: DeviceTransports> DeviceWrite<'_, T> {
     /// All fields of the peer will be overwritten. Returns `false` if no peer with this public key
     /// exists. See also [`Self::add_or_update_peer`] and [`Self::modify_peer`].
     pub async fn update_peer(&mut self, peer: Peer) -> bool {
-        self.modify_peer(&peer.public_key, |peer_mut| {
+        self.modify_peer(peer.public_key.as_public_key(), |peer_mut| {
             peer_mut.clear_allowed_ips();
             peer_mut.add_allowed_ips(peer.allowed_ips);
             peer_mut.set_endpoint(peer.endpoint);

gotatun/src/device/mod.rs

@@ -38,6 +38,8 @@ use tokio::join;
 use tokio::sync::RwLock;
 use tokio::sync::{Mutex, watch};
 
+use crate::key::PeerPublicKey;
+use crate::noise::dh;
 use crate::noise::errors::WireGuardError;
 use crate::noise::handshake::parse_handshake_anon;
 use crate::noise::rate_limiter::RateLimiter;
@@ -117,7 +119,7 @@ pub(crate) struct DeviceState<T: DeviceTransports> {
     /// MTU watcher of the TUN device.
     tun_rx_mtu: MtuWatcher,
 
-    peers: HashMap<x25519::PublicKey, Arc<Mutex<PeerState>>>,
+    peers: HashMap<PeerPublicKey, Arc<Mutex<PeerState>>>,
     peers_by_ip: AllowedIps<Arc<Mutex<PeerState>>>,
     peers_by_idx: parking_lot::Mutex<HashMap<u32, Arc<Mutex<PeerState>>>>,
     index_table: IndexTable,
@@ -438,11 +440,10 @@ impl<T: DeviceTransports> DeviceState<T> {
         let rate_limiter = Arc::new(RateLimiter::new(&public_key, HANDSHAKE_RATE_LIMIT));
 
         for peer in self.peers.values_mut() {
-            peer.lock().await.tunnel.set_static_private(
-                private_key.clone(),
-                public_key,
-                Arc::clone(&rate_limiter),
-            )
+            peer.lock()
+                .await
+                .tunnel
+                .set_static_private(private_key.clone(), Arc::clone(&rate_limiter))
         }
 
         self.key_pair = Some((private_key, public_key));
@@ -569,7 +570,12 @@ impl<T: DeviceTransports> DeviceState<T> {
             let (private_key, public_key) = device.key_pair.clone().expect("Key not set");
             let rate_limiter = device.rate_limiter.clone().unwrap();
             let tun_mtu = device.tun_rx_mtu.clone();
-            (private_key, public_key, rate_limiter, tun_mtu)
+            (
+                dh::StaticSecret::from(private_key),
+                public_key,
+                rate_limiter,
+                tun_mtu,
+            )
         };
 
         while let Ok((src_buf, addr)) = udp_rx.recv_from(&mut packet_pool).await {

gotatun/src/device/peer.rs

@@ -12,18 +12,18 @@
 use std::net::SocketAddr;
 
 use ipnetwork::IpNetwork;
-use x25519_dalek::PublicKey;
 
 #[cfg(feature = "daita")]
 use crate::device::daita::DaitaSettings;
+use crate::key::PeerPublicKey;
 use crate::noise::TimerParams;
 
 /// Peer data. Used to construct and update peers in a [`Device`](crate::device::Device).
 #[derive(Clone, Debug)]
 #[non_exhaustive]
 pub struct Peer {
     /// The peer's public key.
-    pub public_key: PublicKey,
+    pub public_key: PeerPublicKey,
     /// The peer's endpoint address (IP and port).
     ///
     /// An incoming handshake from the peer will overwrite the endpoint to the source
@@ -52,7 +52,7 @@ impl Peer {
     /// Create a new peer with the given public key.
     ///
     /// All other fields are set to their default values.
-    pub const fn new(public_key: PublicKey) -> Self {
+    pub const fn new(public_key: PeerPublicKey) -> Self {
         Self {
             public_key,
             endpoint: None,

gotatun/src/device/tests.rs

@@ -192,19 +192,19 @@ async fn test_endpoint_roaming() {
 #[test_log::test]
 async fn reverse_path_rejects_source_owned_by_another_peer() {
     use crate::device::Peer;
+    use crate::key::PeerPublicKey;
     use ipnetwork::Ipv4Network;
     use std::net::Ipv4Addr;
-    use x25519_dalek::{PublicKey, StaticSecret};
+    use x25519_dalek::StaticSecret;
 
     // Alice has 0.0.0.0/0 as it's allowed IP range
     let (alice, mut bob, _eve) = mock::device_pair().await;
 
     // Set up a third peer "Carol" with 10.0.0.5/32 on Bob's device
-    let carol_pub = PublicKey::from(&StaticSecret::random());
     let added = bob
         .device
         .add_peer(
-            Peer::new(carol_pub).with_allowed_ip(
+            Peer::new(PeerPublicKey::from_secret(&StaticSecret::random())).with_allowed_ip(
                 Ipv4Network::new(Ipv4Addr::new(10, 0, 0, 5), 32)
                     .unwrap()
                     .into(),
@@ -240,7 +240,7 @@ async fn modify_peer_preshared_key_reaches_tunnel() {
         let peer = get_first_and_only_peer(device).await;
         let updated = device
             .device
-            .modify_peer(&peer.public_key, |peer_mut| {
+            .modify_peer(peer.public_key.as_public_key(), |peer_mut| {
                 peer_mut.set_preshared_key(Some(preshared_key));
             })
             .await

gotatun/src/device/tests/mock.rs

@@ -25,9 +25,11 @@ use tokio::sync::{
     mpsc::{self, Receiver, Sender},
 };
 use tokio_stream::wrappers::BroadcastStream;
-use x25519_dalek::{PublicKey, StaticSecret};
+use x25519_dalek::StaticSecret;
 use zerocopy::IntoBytes;
 
+use crate::key::PeerPublicKey;
+
 use crate::{
     device::{Device, DeviceBuilder, Peer},
     noise::index_table::IndexTable,
@@ -127,14 +129,11 @@ pub async fn device_pair() -> (MockDevice, MockDevice, MockEavesdropper) {
     let privkey_a = StaticSecret::random();
     let privkey_b = StaticSecret::random();
 
-    let pubkey_a = PublicKey::from(&privkey_a);
-    let pubkey_b = PublicKey::from(&privkey_b);
-
-    let peer_a = Peer::new(pubkey_a)
+    let peer_a = Peer::new(PeerPublicKey::from_secret(&privkey_a))
         .with_endpoint((endpoint_a, port).into())
         .with_allowed_ip(Ipv4Network::new(Ipv4Addr::UNSPECIFIED, 0).unwrap().into());
 
-    let peer_b = Peer::new(pubkey_b)
+    let peer_b = Peer::new(PeerPublicKey::from_secret(&privkey_b))
         .with_endpoint((endpoint_b, port).into())
         .with_allowed_ip(Ipv4Network::new(Ipv4Addr::UNSPECIFIED, 0).unwrap().into());
 

gotatun/src/device/uapi/mod.rs

@@ -39,6 +39,7 @@ use super::{Connection, DeviceState, Reconfigure};
 use crate::device::DeviceTransports;
 #[cfg(feature = "daita-uapi")]
 use crate::device::uapi::command::SetUnset;
+use crate::key::PeerPublicKey;
 use crate::serialization::KeyBytes;
 use command::{Get, GetPeer, GetResponse, Peer, Request, Response, Set, SetPeer, SetResponse};
 use eyre::{Context, bail, eyre};
@@ -499,7 +500,7 @@ async fn on_api_get(_: Get, d: &DeviceState<impl DeviceTransports>) -> GetRespon
 
         peers.push(GetPeer {
             peer: Peer {
-                public_key: KeyBytes(*public_key.as_bytes()),
+                public_key: KeyBytes(*public_key.as_public_key().as_bytes()),
                 preshared_key: peer
                     .preshared_key()
                     .map(|key| command::SetUnset::Set(KeyBytes(key))),
@@ -632,17 +633,27 @@ async fn on_api_set(
             continue;
         }
 
+        // Reject low-order (non-contributory) peer keys up front; a handshake
+        // with such a peer could never succeed.
+        let peer_public_key = match PeerPublicKey::new(public_key) {
+            Ok(key) => key,
+            Err(error) => {
+                log::debug!("Rejecting peer: {error}");
+                return (SetResponse { errno: EINVAL }, reconfigure);
+            }
+        };
+
         let mut new_peer = match device.remove_peer(&public_key).await {
             None => {
                 // New peer
-                crate::device::Peer::new(public_key)
+                crate::device::Peer::new(peer_public_key)
             }
             Some(old_peer) => {
                 // Take existing peer
                 let peer = old_peer.lock().await;
 
                 crate::device::Peer {
-                    public_key,
+                    public_key: peer_public_key,
                     preshared_key: peer.preshared_key(),
                     endpoint: peer.endpoint().addr,
                     keepalive: peer.persistent_keepalive(),