From 36f0b94514368d5dd3815743044a755d04c1b344 Mon Sep 17 00:00:00 2001 From: Dennis Schwerdel Date: Tue, 6 Oct 2020 22:52:14 +0200 Subject: [PATCH] Some improvements --- src/cloud.rs | 15 ++++----------- src/crypto/core.rs | 35 +++++++++++++++-------------------- src/crypto/init.rs | 35 +++++++++++++++-------------------- src/crypto/mod.rs | 2 +- src/error.rs | 5 ++--- 5 files changed, 37 insertions(+), 55 deletions(-) diff --git a/src/cloud.rs b/src/cloud.rs index 8eb83fe..1e351ef 100644 --- a/src/cloud.rs +++ b/src/cloud.rs @@ -738,22 +738,15 @@ impl GenericCloud { - info!("Closing connection to {} due to error", addr_nice(src)); - self.remove_peer(src); - } - Error::CryptoInit(_) => { - info!("Closing pending connection to {} due to error", addr_nice(src)); - self.pending_inits.remove(&src); - } - _ => () + if let Error::CryptoInit(_) = e { + info!("Closing pending connection to {} due to error", addr_nice(src)); + self.pending_inits.remove(&src); } } } fn handle_device_event(&mut self, buffer: &mut MsgBuffer) { - try_fail!(self.device.read(buffer), "Failed to read from tap device: {}"); + try_fail!(self.device.read(buffer), "Failed to read from device: {}"); if let Err(e) = self.handle_interface_data(buffer) { error!("Error: {}", e); } diff --git a/src/crypto/core.rs b/src/crypto/core.rs index 74aafa3..afd80c5 100644 --- a/src/crypto/core.rs +++ b/src/crypto/core.rs @@ -176,13 +176,13 @@ impl CryptoCore { fn decrypt_with_key<'a>(key: &mut CryptoKey, nonce: Nonce, data_and_tag: &'a mut [u8]) -> Result<(), Error> { if nonce < key.min_nonce { - return Err(Error::Unauthorized("Old nonce rejected")) + return Err(Error::Crypto("Old nonce rejected")) } // decrypt let crypto_nonce = aead::Nonce::assume_unique_for_key(*nonce.as_bytes()); key.key .open_in_place(crypto_nonce, aead::Aad::empty(), data_and_tag) - .map_err(|_| Error::Unauthorized("Failed to decrypt data"))?; + .map_err(|_| Error::Crypto("Failed to decrypt data"))?; // last seen nonce if key.seen_nonce < nonce { key.seen_nonce = nonce; @@ -230,12 +230,17 @@ impl CryptoCore { } +pub fn create_dummy_pair(algo: &'static aead::Algorithm) -> (CryptoCore, CryptoCore) { + let key_data = random_data(algo.key_len()); + let sender = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), true); + let receiver = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), false); + (sender, receiver) +} + pub fn test_speed(algo: &'static aead::Algorithm, max_time: &Duration) -> f64 { let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.set_length(1000); - let key_data = random_data(algo.key_len()); - let mut sender = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), true); - let mut receiver = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), false); + let (mut sender, mut receiver) = create_dummy_pair(algo); let mut iterations = 0; let start = Instant::now(); while (Instant::now() - start).as_nanos() < max_time.as_nanos() { @@ -256,14 +261,6 @@ mod tests { use super::*; use ring::aead::{self, LessSafeKey, UnboundKey}; - - fn setup_pair(algo: &'static aead::Algorithm) -> (CryptoCore, CryptoCore) { - let key = random_data(algo.key_len()); - let crypto1 = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key).unwrap()), false); - let crypto2 = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key).unwrap()), true); - (crypto1, crypto2) - } - #[test] fn test_nonce() { let mut nonce = Nonce::zero(); @@ -275,7 +272,7 @@ mod tests { } fn test_encrypt_decrypt(algo: &'static aead::Algorithm) { - let (mut sender, mut receiver) = setup_pair(algo); + let (mut sender, mut receiver) = create_dummy_pair(algo); let plain = random_data(1000); let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.clone_from(&plain); @@ -303,7 +300,7 @@ mod tests { fn test_tampering(algo: &'static aead::Algorithm) { - let (mut sender, mut receiver) = setup_pair(algo); + let (mut sender, mut receiver) = create_dummy_pair(algo); let plain = random_data(1000); let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.clone_from(&plain); @@ -343,7 +340,7 @@ mod tests { } fn test_nonce_pinning(algo: &'static aead::Algorithm) { - let (mut sender, mut receiver) = setup_pair(algo); + let (mut sender, mut receiver) = create_dummy_pair(algo); let plain = random_data(1000); let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.clone_from(&plain); @@ -384,7 +381,7 @@ mod tests { } fn test_key_rotation(algo: &'static aead::Algorithm) { - let (mut sender, mut receiver) = setup_pair(algo); + let (mut sender, mut receiver) = create_dummy_pair(algo); let plain = random_data(1000); let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.clone_from(&plain); @@ -466,9 +463,7 @@ mod benches { fn crypto_bench(b: &mut Bencher, algo: &'static aead::Algorithm) { let mut buffer = MsgBuffer::new(EXTRA_LEN); buffer.set_length(1400); - let key_data = random_data(algo.key_len()); - let mut sender = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), true); - let mut receiver = CryptoCore::new(LessSafeKey::new(UnboundKey::new(algo, &key_data).unwrap()), false); + let (mut sender, mut receiver) = create_dummy_pair(algo); b.iter(|| { sender.encrypt(&mut buffer); receiver.decrypt(&mut buffer).unwrap(); diff --git a/src/crypto/init.rs b/src/crypto/init.rs index 484d351..ba8fef1 100644 --- a/src/crypto/init.rs +++ b/src/crypto/init.rs @@ -84,7 +84,6 @@ pub type SaltedNodeIdHash = [u8; SALTED_NODE_ID_HASH_LEN]; #[allow(clippy::large_enum_variant)] pub enum InitMsg { - // TODO: include only salted hashes of node_id and use public_key for leader election Ping { salted_node_id_hash: SaltedNodeIdHash, ecdh_public_key: EcdhPublicKey, @@ -153,7 +152,7 @@ impl InitMsg { } } if !found_key { - return Err(Error::Unauthorized("untrusted peer")) + return Err(Error::Crypto("untrusted peer")) } let mut stage = None; @@ -233,7 +232,7 @@ impl InitMsg { let signed_data = &r.into_inner()[0..pos]; let public_key = signature::UnparsedPublicKey::new(&ED25519, &public_key_data); if public_key.verify(&signed_data, &signature).is_err() { - return Err(Error::Unauthorized("invalid signature")) + return Err(Error::Crypto("invalid signature")) } let stage = match stage { @@ -420,17 +419,15 @@ impl InitState

{ } } - pub fn send_ping(&mut self, out: &mut MsgBuffer) -> Result<(), Error> { + pub fn send_ping(&mut self, out: &mut MsgBuffer) { // create ecdh ephemeral key let (ecdh_private_key, ecdh_public_key) = self.create_ecdh_keypair(); self.ecdh_private_key = Some(ecdh_private_key); // create stage 1 msg - self.send_message(STAGE_PING, Some(ecdh_public_key), out)?; + self.send_message(STAGE_PING, Some(ecdh_public_key), out); self.next_stage = STAGE_PONG; - - Ok(()) } pub fn stage(&self) -> u8 { @@ -449,7 +446,7 @@ impl InitState

{ Ok(()) } else if self.failed_retries < 5 { self.failed_retries += 1; - self.repeat_last_message(out)?; + self.repeat_last_message(out); Ok(()) } else { Err(Error::CryptoInit("Initialization timeout")) @@ -473,13 +470,13 @@ impl InitState

{ (ecdh_private_key, ecdh_public_key) } - fn encrypt_payload(&mut self) -> Result { + fn encrypt_payload(&mut self) -> MsgBuffer { let mut buffer = MsgBuffer::new(EXTRA_LEN); self.payload.write_to(&mut buffer); if let Some(crypto) = &mut self.crypto { crypto.encrypt(&mut buffer); } - Ok(buffer) + buffer } fn decrypt(&mut self, data: &mut MsgBuffer) -> Result { @@ -499,7 +496,7 @@ impl InitState

{ fn send_message( &mut self, stage: u8, ecdh_public_key: Option, out: &mut MsgBuffer - ) -> Result<(), Error> { + ) { debug!("Sending init with stage={}", stage); assert!(out.is_empty()); let mut public_key = [0; ED25519_PUBLIC_KEY_LEN]; @@ -517,13 +514,13 @@ impl InitState

{ salted_node_id_hash: self.salted_node_id_hash, ecdh_public_key: ecdh_public_key.unwrap(), algorithms: self.algorithms.clone(), - encrypted_payload: self.encrypt_payload()? + encrypted_payload: self.encrypt_payload() } } STAGE_PENG => { InitMsg::Peng { salted_node_id_hash: self.salted_node_id_hash, - encrypted_payload: self.encrypt_payload()? + encrypted_payload: self.encrypt_payload() } } _ => unreachable!() @@ -532,17 +529,15 @@ impl InitState

{ let len = msg.write_to(&mut bytes, &self.key_pair).expect("Buffer too small"); self.last_message = Some(bytes[0..len].to_vec()); out.set_length(len); - Ok(()) } - fn repeat_last_message(&self, out: &mut MsgBuffer) -> Result<(), Error> { + fn repeat_last_message(&self, out: &mut MsgBuffer) { if let Some(ref bytes) = self.last_message { debug!("Repeating last init message"); let buffer = out.buffer(); buffer[0..bytes.len()].copy_from_slice(bytes); out.set_length(bytes.len()); } - Ok(()) } fn select_algorithm(&self, peer_algos: &Algorithms) -> Result, Error> { @@ -597,7 +592,7 @@ impl InitState

{ } else if self.next_stage == CLOSING { return Ok(InitResult::Continue) } else if self.last_message.is_some() { - self.repeat_last_message(out)?; + self.repeat_last_message(out); return Ok(InitResult::Continue) } else { return Err(Error::CryptoInit("Received invalid stage as first message")) @@ -617,7 +612,7 @@ impl InitState

{ } // create and send stage 2 reply - self.send_message(STAGE_PONG, Some(my_ecdh_public_key), out)?; + self.send_message(STAGE_PONG, Some(my_ecdh_public_key), out); self.next_stage = STAGE_PENG; Ok(InitResult::Continue) @@ -636,7 +631,7 @@ impl InitState

{ self.decrypt(&mut encrypted_payload).map_err(|_| Error::CryptoInit("Failed to decrypt payload"))?; // create and send stage 3 reply - self.send_message(STAGE_PENG, None, out)?; + self.send_message(STAGE_PENG, None, out); self.next_stage = WAITING_TO_CLOSE; self.close_time = 60; @@ -701,7 +696,7 @@ mod tests { fn normal_init() { let (mut sender, mut receiver) = create_pair(); let mut out = MsgBuffer::new(8); - sender.send_ping(&mut out).unwrap(); + sender.send_ping(&mut out); assert_eq!(sender.stage(), STAGE_PONG); let result = receiver.handle_init(&mut out).unwrap(); assert_eq!(receiver.stage(), STAGE_PENG); diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index c3df94c..558bf43 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -258,7 +258,7 @@ impl PeerCrypto

{ if init.stage() != init::STAGE_PING { Err(Error::InvalidCryptoState("Initialization already ongoing")) } else { - init.send_ping(out)?; + init.send_ping(out); out.prepend_byte(INIT_MESSAGE_FIRST_BYTE); Ok(()) } diff --git a/src/error.rs b/src/error.rs index 236a58b..c86695c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -5,12 +5,11 @@ use std::io; #[derive(Error, Debug)] pub enum Error { - #[error("Unauthorized message: {0}")] - Unauthorized(&'static str), - + /// Crypto init error, this is fatal and the init needs to be aborted #[error("Crypto initialization error: {0}")] CryptoInit(&'static str), + /// Crypto error with this one message, no permanent error #[error("Crypto error: {0}")] Crypto(&'static str),