From 623ea191c11dbfb34887635a03c3dc5ff08ef9f1 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 16 Oct 2023 11:48:43 +1100 Subject: [PATCH 1/2] Make `Tunn::new` infallible --- boringtun/src/device/mod.rs | 3 +-- boringtun/src/ffi/mod.rs | 7 ++----- boringtun/src/noise/handshake.rs | 14 +++++++------- boringtun/src/noise/mod.rs | 15 ++++++--------- 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/boringtun/src/device/mod.rs b/boringtun/src/device/mod.rs index bc14c896..8cd1c7c8 100644 --- a/boringtun/src/device/mod.rs +++ b/boringtun/src/device/mod.rs @@ -334,8 +334,7 @@ impl Device { keepalive, next_index, None, - ) - .unwrap(); + ); let peer = Peer::new(tunn, next_index, endpoint, allowed_ips, preshared_key); diff --git a/boringtun/src/ffi/mod.rs b/boringtun/src/ffi/mod.rs index 3b4e3bb3..1e5a2a9f 100644 --- a/boringtun/src/ffi/mod.rs +++ b/boringtun/src/ffi/mod.rs @@ -292,17 +292,14 @@ pub unsafe extern "C" fn new_tunnel( Some(keep_alive) }; - let tunnel = match Tunn::new( + let tunnel = Box::new(Mutex::new(Tunn::new( private_key, public_key, preshared_key, keep_alive, index, None, - ) { - Ok(t) => Box::new(Mutex::new(t)), - Err(_) => return ptr::null_mut(), - }; + ))); PANIC_HOOK.call_once(|| { // FFI won't properly unwind on panic, but it will if we cause a segmentation fault diff --git a/boringtun/src/noise/handshake.rs b/boringtun/src/noise/handshake.rs index b7c93731..2ee6711f 100644 --- a/boringtun/src/noise/handshake.rs +++ b/boringtun/src/noise/handshake.rs @@ -375,19 +375,19 @@ impl NoiseParams { static_public: x25519::PublicKey, peer_static_public: x25519::PublicKey, preshared_key: Option<[u8; 32]>, - ) -> Result { + ) -> NoiseParams { let static_shared = static_private.diffie_hellman(&peer_static_public); let initial_sending_mac_key = b2s_hash(LABEL_MAC1, peer_static_public.as_bytes()); - Ok(NoiseParams { + NoiseParams { static_public, static_private, peer_static_public, static_shared, sending_mac1_key: initial_sending_mac_key, preshared_key, - }) + } } /// Set a new private key @@ -415,15 +415,15 @@ impl Handshake { peer_static_public: x25519::PublicKey, global_idx: u32, preshared_key: Option<[u8; 32]>, - ) -> Result { + ) -> Handshake { let params = NoiseParams::new( static_private, static_public, peer_static_public, preshared_key, - )?; + ); - Ok(Handshake { + Handshake { params, next_index: global_idx, previous: HandshakeState::None, @@ -432,7 +432,7 @@ impl Handshake { stamper: TimeStamper::new(), cookies: Default::default(), last_rtt: None, - }) + } } pub(crate) fn is_in_progress(&self) -> bool { diff --git a/boringtun/src/noise/mod.rs b/boringtun/src/noise/mod.rs index 79a6b923..62cec983 100644 --- a/boringtun/src/noise/mod.rs +++ b/boringtun/src/noise/mod.rs @@ -198,18 +198,17 @@ impl Tunn { persistent_keepalive: Option, index: u32, rate_limiter: Option>, - ) -> Result { + ) -> Self { let static_public = x25519::PublicKey::from(&static_private); - let tunn = Tunn { + Tunn { handshake: Handshake::new( static_private, static_public, peer_static_public, index << 8, preshared_key, - ) - .map_err(|_| "Invalid parameters")?, + ), sessions: Default::default(), current: Default::default(), tx_bytes: Default::default(), @@ -221,9 +220,7 @@ impl Tunn { rate_limiter: rate_limiter.unwrap_or_else(|| { Arc::new(RateLimiter::new(&static_public, PEER_HANDSHAKE_RATE_LIMIT)) }), - }; - - Ok(tunn) + } } /// Update the private key and clear existing sessions @@ -606,10 +603,10 @@ mod tests { let their_public_key = x25519_dalek::PublicKey::from(&their_secret_key); let their_idx = OsRng.next_u32(); - let my_tun = Tunn::new(my_secret_key, their_public_key, None, None, my_idx, None).unwrap(); + let my_tun = Tunn::new(my_secret_key, their_public_key, None, None, my_idx, None); let their_tun = - Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None).unwrap(); + Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None); (my_tun, their_tun) } From 7ac527309d47c5cf6e9843ffe68c435a899e8111 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 16 Oct 2023 13:15:25 +1100 Subject: [PATCH 2/2] Make `Tunn::set_static_private` infallible --- boringtun/src/device/mod.rs | 27 +++++---------------------- boringtun/src/noise/handshake.rs | 5 ++--- boringtun/src/noise/mod.rs | 8 +++----- 3 files changed, 10 insertions(+), 30 deletions(-) diff --git a/boringtun/src/device/mod.rs b/boringtun/src/device/mod.rs index 8cd1c7c8..04cd7285 100644 --- a/boringtun/src/device/mod.rs +++ b/boringtun/src/device/mod.rs @@ -452,8 +452,6 @@ impl Device { } fn set_key(&mut self, private_key: x25519::StaticSecret) { - let mut bad_peers = vec![]; - let public_key = x25519::PublicKey::from(&private_key); let key_pair = Some((private_key.clone(), public_key)); @@ -466,30 +464,15 @@ impl Device { let rate_limiter = Arc::new(RateLimiter::new(&public_key, HANDSHAKE_RATE_LIMIT)); for peer in self.peers.values_mut() { - let mut peer_mut = peer.lock(); - - if peer_mut - .tunnel - .set_static_private( - private_key.clone(), - public_key, - Some(Arc::clone(&rate_limiter)), - ) - .is_err() - { - // In case we encounter an error, we will remove that peer - // An error will be a result of bad public key/secret key combination - bad_peers.push(Arc::clone(peer)); - } + peer.lock().tunnel.set_static_private( + private_key.clone(), + public_key, + Some(Arc::clone(&rate_limiter)), + ) } self.key_pair = key_pair; self.rate_limiter = Some(rate_limiter); - - // Remove all the bad peers - for _ in bad_peers { - unimplemented!(); - } } #[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))] diff --git a/boringtun/src/noise/handshake.rs b/boringtun/src/noise/handshake.rs index 2ee6711f..40ed8037 100644 --- a/boringtun/src/noise/handshake.rs +++ b/boringtun/src/noise/handshake.rs @@ -395,7 +395,7 @@ impl NoiseParams { &mut self, static_private: x25519::StaticSecret, static_public: x25519::PublicKey, - ) -> Result<(), WireGuardError> { + ) { // Check that the public key indeed matches the private key let check_key = x25519::PublicKey::from(&static_private); assert_eq!(check_key.as_bytes(), static_public.as_bytes()); @@ -404,7 +404,6 @@ impl NoiseParams { self.static_public = static_public; self.static_shared = self.static_private.diffie_hellman(&self.peer_static_public); - Ok(()) } } @@ -475,7 +474,7 @@ impl Handshake { &mut self, private_key: x25519::StaticSecret, public_key: x25519::PublicKey, - ) -> Result<(), WireGuardError> { + ) { self.params.set_static_private(private_key, public_key) } diff --git a/boringtun/src/noise/mod.rs b/boringtun/src/noise/mod.rs index 62cec983..76e377b6 100644 --- a/boringtun/src/noise/mod.rs +++ b/boringtun/src/noise/mod.rs @@ -229,17 +229,16 @@ impl Tunn { static_private: x25519::StaticSecret, static_public: x25519::PublicKey, rate_limiter: Option>, - ) -> Result<(), WireGuardError> { + ) { self.timers.should_reset_rr = rate_limiter.is_none(); self.rate_limiter = rate_limiter.unwrap_or_else(|| { Arc::new(RateLimiter::new(&static_public, PEER_HANDSHAKE_RATE_LIMIT)) }); self.handshake - .set_static_private(static_private, static_public)?; + .set_static_private(static_private, static_public); for s in &mut self.sessions { *s = None; } - Ok(()) } /// Encapsulate a single packet from the tunnel interface. @@ -605,8 +604,7 @@ mod tests { let my_tun = Tunn::new(my_secret_key, their_public_key, None, None, my_idx, None); - let their_tun = - Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None); + let their_tun = Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None); (my_tun, their_tun) }