Better management of key material

Cargo.lock

@@ -1241,6 +1241,7 @@ dependencies = [
  "blake2 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "byteorder 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "chacha20poly1305 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "clear_on_drop 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "crossbeam-deque 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "digest 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "futures 0.1.28 (registry+https://github.com/rust-lang/crates.io-index)",

Cargo.toml

@@ -25,6 +25,7 @@ hjul = "0.1.2"
 ring = "0.16.7"
 chacha20poly1305 = "^0.1"
 aead = "^0.1.1"
+clear_on_drop = "0.2.3"
 
 [dependencies.x25519-dalek]
 version = "^0.5"

src/handshake/noise.rs

@@ -15,6 +15,8 @@ use rand::{CryptoRng, RngCore};
 use generic_array::typenum::*;
 use generic_array::*;
 
+use clear_on_drop::clear_stack_on_return;
+
 use super::device::Device;
 use super::messages::{NoiseInitiation, NoiseResponse};
 use super::messages::{TYPE_INITIATION, TYPE_RESPONSE};
@@ -39,6 +41,9 @@ const SIZE_HS: usize = 32;
 const SIZE_NONCE: usize = 8;
 const SIZE_TAG: usize = 16;
 
+// number of pages to clear after sensitive call
+const CLEAR_PAGES: usize = 1;
+
 // C := Hash(Construction)
 const INITIAL_CK: [u8; SIZE_CK] = [
     0x60, 0xe2, 0x6d, 0xae, 0xf3, 0x27, 0xef, 0xc0, 0x2e, 0xc3, 0x35, 0xe2, 0xa0, 0x25, 0xd2, 0xd0,
@@ -103,21 +108,21 @@ macro_rules! KDF3 {
 }
 
 macro_rules! SEAL {
-    ($key:expr, $ad:expr, $pt:expr, $ct:expr) => {{
-        let ct = ChaCha20Poly1305::new(*GenericArray::from_slice($key))
+    ($key:expr, $ad:expr, $pt:expr, $ct:expr) => {
+        ChaCha20Poly1305::new(*GenericArray::from_slice($key))
             .encrypt(&ZERO_NONCE.into(), Payload { msg: $pt, aad: $ad })
-            .unwrap();
-        $ct.copy_from_slice(&ct);
-    }};
+            .map(|ct| $ct.copy_from_slice(&ct))
+            .unwrap()
+    };
 }
 
 macro_rules! OPEN {
-    ($key:expr, $ad:expr, $pt:expr, $ct:expr) => {{
+    ($key:expr, $ad:expr, $pt:expr, $ct:expr) => {
         ChaCha20Poly1305::new(*GenericArray::from_slice($key))
             .decrypt(&ZERO_NONCE.into(), Payload { msg: $ct, aad: $ad })
             .map_err(|_| HandshakeError::DecryptionFailure)
             .map(|pt| $pt.copy_from_slice(&pt))
-    }};
+    };
 }
 
 #[cfg(test)]
@@ -211,6 +216,7 @@ pub fn create_initiation<T: Copy, R: RngCore + CryptoRng>(
     sender: u32,
     msg: &mut NoiseInitiation,
 ) -> Result<(), HandshakeError> {
+    clear_stack_on_return(CLEAR_PAGES, || {
         // initialize state
 
         let ck = INITIAL_CK;
@@ -281,12 +287,14 @@ pub fn create_initiation<T: Copy, R: RngCore + CryptoRng>(
         });
 
         Ok(())
+    })
 }
 
 pub fn consume_initiation<'a, T: Copy>(
     device: &'a Device<T>,
     msg: &NoiseInitiation,
 ) -> Result<(&'a Peer<T>, TemporaryState), HandshakeError> {
+    clear_stack_on_return(CLEAR_PAGES, || {
         // initialize state
 
         let ck = INITIAL_CK;
@@ -346,9 +354,13 @@ pub fn consume_initiation<'a, T: Copy>(
 
         let hs = HASH!(&hs, &msg.f_timestamp);
 
+        // clear initiation state
+        *peer.state.lock() = State::Reset;
+
         // return state (to create response)
 
         Ok((peer, (msg.f_sender.get(), eph_r_pk, hs, ck)))
+    })
 }
 
 pub fn create_response<T: Copy, R: RngCore + CryptoRng>(
@@ -358,6 +370,7 @@ pub fn create_response<T: Copy, R: RngCore + CryptoRng>(
     state: TemporaryState,   // state from "consume_initiation"
     msg: &mut NoiseResponse, // resulting response
 ) -> Result<KeyPair, HandshakeError> {
+    clear_stack_on_return(CLEAR_PAGES, || {
         // unpack state
 
         let (receiver, eph_r_pk, hs, ck) = state;
@@ -408,14 +421,10 @@ pub fn create_response<T: Copy, R: RngCore + CryptoRng>(
             &mut msg.f_empty  // \epsilon || tag
         );
 
-    /* not strictly needed
-     * // H := Hash(H || msg.empty)
-     * let hs = HASH!(&hs, &msg.f_empty_tag);
-     */
+        // Not strictly needed
+        // let hs = HASH!(&hs, &msg.f_empty_tag);
 
         // derive key-pair
-    // (verbose code, due to GenericArray -> [u8; 32] conversion)
-
         let (key_recv, key_send) = KDF2!(&ck, &[]);
 
         // return unconfirmed key-pair
@@ -432,23 +441,24 @@ pub fn create_response<T: Copy, R: RngCore + CryptoRng>(
                 key: key_recv.into(),
             },
         })
+    })
 }
 
 pub fn consume_response<T: Copy>(
     device: &Device<T>,
     msg: &NoiseResponse,
 ) -> Result<Output<T>, HandshakeError> {
-    // retrieve peer and associated state
-
+    clear_stack_on_return(CLEAR_PAGES, || {
+        // retrieve peer and copy associated state
         let peer = device.lookup_id(msg.f_receiver.get())?;
-    let (hs, ck, sender, eph_sk) = match peer.get_state() {
-        State::Reset => Err(HandshakeError::InvalidState),
+        let (hs, ck, sender, eph_sk) = match *peer.state.lock() {
             State::InitiationSent {
                 hs,
                 ck,
                 sender,
-            eph_sk,
-        } => Ok((hs, ck, sender, eph_sk)),
+                ref eph_sk,
+            } => Ok((hs, ck, sender, StaticSecret::from(eph_sk.to_bytes()))),
+            _ => Err(HandshakeError::InvalidState),
         }?;
 
         // C := Kdf1(C, E_pub)
@@ -489,10 +499,14 @@ pub fn consume_response<T: Copy>(
 
         let (key_send, key_recv) = KDF2!(&ck, &[]);
 
+        // null the state
+
+        *peer.state.lock() = State::Reset;
+
         // return confirmed key-pair
 
         Ok((
-        Some(peer.identifier), // proves overship of the public key (e.g. for updating the endpoint)
+            Some(peer.identifier), // proves ownership of the public key (e.g. for updating the endpoint)
             None,                  // no response message
             Some(KeyPair {
                 birth: Instant::now(),
@@ -507,4 +521,5 @@ pub fn consume_response<T: Copy>(
                 },
             }),
         ))
+    })
 }

src/handshake/peer.rs

@@ -9,6 +9,8 @@ use x25519_dalek::PublicKey;
 use x25519_dalek::SharedSecret;
 use x25519_dalek::StaticSecret;
 
+use clear_on_drop::clear::Clear;
+
 use super::device::Device;
 use super::macs;
 use super::timestamp;
@@ -27,9 +29,9 @@ pub struct Peer<T> {
     pub(crate) identifier: T,
 
     // mutable state
-    state: Mutex<State>,
-    timestamp: Mutex<Option<timestamp::TAI64N>>,
-    last_initiation_consumption: Mutex<Option<Instant>>,
+    pub(crate) state: Mutex<State>,
+    pub(crate) timestamp: Mutex<Option<timestamp::TAI64N>>,
+    pub(crate) last_initiation_consumption: Mutex<Option<Instant>>,
 
     // state related to DoS mitigation fields
     pub(crate) macs: Mutex<macs::Generator>,
@@ -50,21 +52,15 @@ pub enum State {
     },
 }
 
-impl Clone for State {
-    fn clone(&self) -> State {
+impl Drop for State {
+    fn drop(&mut self) {
         match self {
-            State::Reset => State::Reset,
-            State::InitiationSent {
-                sender,
-                eph_sk,
-                hs,
-                ck,
-            } => State::InitiationSent {
-                sender: *sender,
-                eph_sk: StaticSecret::from(eph_sk.to_bytes()),
-                hs: *hs,
-                ck: *ck,
+            State::InitiationSent{hs, ck, ..} => {
+                // eph_sk already cleared by dalek-x25519
+                hs.clear();
+                ck.clear();
             },
+            _ => ()
         }
     }
 }
@@ -90,13 +86,6 @@ where
         }
     }
 
-    /// Return the state of the peer
-    ///
-    /// # Arguments
-    pub fn get_state(&self) -> State {
-        self.state.lock().clone()
-    }
-
     /// Set the state of the peer unconditionally
     ///
     /// # Arguments

src/types/endpoint.rs

@@ -1,8 +1,5 @@
 use std::net::SocketAddr;
 
-/* The generic implementation (not supporting "sticky-sockets"),
- * is to simply use SocketAddr directly as the endpoint.
- */
-pub trait Endpoint: Into<SocketAddr> {}
+pub trait Endpoint: Into<SocketAddr> + From<SocketAddr> {}
 
-impl<T> Endpoint for T where T: Into<SocketAddr> {}
+impl<T> Endpoint for T where T: Into<SocketAddr> + From<SocketAddr> {}

src/types/keys.rs

@@ -1,15 +1,23 @@
+use clear_on_drop::clear::Clear;
 use std::time::Instant;
 
 /* This file holds types passed between components.
  * Whenever a type cannot be held local to a single module.
  */
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 pub struct Key {
     pub key: [u8; 32],
     pub id: u32,
 }
 
+// zero key on drop
+impl Drop for Key {
+    fn drop(&mut self) {
+        self.key.clear()
+    }
+}
+
 #[cfg(test)]
 impl PartialEq for Key {
     fn eq(&self, other: &Self) -> bool {
@@ -17,7 +25,7 @@ impl PartialEq for Key {
     }
 }
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 pub struct KeyPair {
     pub birth: Instant,  // when was the key-pair created
     pub initiator: bool, // has the key-pair been confirmed?

src/types/tun.rs

@@ -1,6 +1,6 @@
 use std::error;
 
-pub trait Tun: Send + Sync {
+pub trait Tun: Send + Sync + 'static {
     type Error: error::Error;
 
     /// Returns the MTU of the device
@@ -22,13 +22,13 @@ pub trait Tun: Send + Sync {
     ///
     /// # Arguments
     ///
-    /// - dst: Destination buffer (enough space for MTU bytes + header)
+    /// - buf: Destination buffer (enough space for MTU bytes + header)
     /// - offset: Offset for the beginning of the IP packet
     ///
     /// # Returns
     ///
     /// The size of the IP packet (ignoring the header) or an std::error::Error instance:
-    fn read(&self, dst: &mut [u8], offset: usize) -> Result<usize, Self::Error>;
+    fn read(&self, buf: &mut [u8], offset: usize) -> Result<usize, Self::Error>;
 
     /// Writes an IP packet to the tunnel device
     ///