diff --git a/src/conn.rs b/src/conn.rs index d63b346511fb89ef527d54724c8b7864a75439e8..2edbeef646e111564ef222d196ff43578e1b01ee 100644 --- a/src/conn.rs +++ b/src/conn.rs @@ -286,12 +286,15 @@ impl Conn { Packet::NewKeys(_) => { match self.state { ConnState::InKex { done_auth, ref mut output } => { - // NewKeys shouldn't be received before kexdhinit/kexdhreply - let mut ko = output.take().ok_or(Error::PacketWrong)?; - s.rekey(ko.keys.take().trap()?); + // NewKeys shouldn't be received before kexdhinit/kexdhreply. + + // .as_ref() rather than .take(), so we can zeroize later + let ko = output.as_ref().ok_or(Error::PacketWrong)?; + // keys.take() leaves remnant memory in output, but output will zeroize soon + s.rekey(ko.keys.clone()); self.sess_id.get_or_insert(ko.h.clone()); - // force ZeroizeOnDrop to run - *output = Default::default(); + // zeroize output.keys via ZeroizeOnDrop + *output = None; self.state = if done_auth { ConnState::Authed } else { diff --git a/src/encrypt.rs b/src/encrypt.rs index fb0f1978c836ad9acfb2db240570f91f2a6e533f..8bca668b7d78370ce18037a6ffcbcaeb5ec67871 100644 --- a/src/encrypt.rs +++ b/src/encrypt.rs @@ -134,7 +134,9 @@ impl KeyState { } } -#[derive(Debug, ZeroizeOnDrop)] +// Clone is required so we can clone() then drop the original in place, +// avoiding issues with Option::take(). This could be revisited. +#[derive(Debug, Clone, ZeroizeOnDrop)] pub(crate) struct Keys { pub(crate) enc: EncKey, pub(crate) dec: DecKey, @@ -532,7 +534,7 @@ impl Cipher { } } -#[derive(ZeroizeOnDrop)] +#[derive(Clone, ZeroizeOnDrop)] pub(crate) enum EncKey { ChaPoly(SSHChaPoly), Aes256Ctr(Aes256Ctr32BE), @@ -585,7 +587,7 @@ impl EncKey { } } -#[derive(ZeroizeOnDrop)] +#[derive(Clone, ZeroizeOnDrop)] pub(crate) enum DecKey { ChaPoly(SSHChaPoly), Aes256Ctr(Aes256Ctr32BE), @@ -670,6 +672,7 @@ impl fmt::Display for Integ { } } +#[derive(Clone)] pub(crate) enum IntegKey { ChaPoly, HmacSha256([u8; 32]), diff --git a/src/kex.rs b/src/kex.rs index 252c79e2721ab8986c2618e297e108ee163afd8a..1b8bc63d14b21e653552497ee17048de1e195756 100644 --- a/src/kex.rs +++ b/src/kex.rs @@ -425,14 +425,13 @@ impl SharedSecret { mut kex: Kex, p: &packets::KexDHReply<'f>, sess_id: &Option<SessId>, b: &mut dyn CliBehaviour ) -> Result<KexOutput> { - // let mut algos = kex.algos.take().trap()?; - let mut algos = kex.algos.trap()?; + let mut algos = kex.algos.as_mut().trap()?; let mut kex_hash = kex.kex_hash.take().trap()?; kex_hash.prefinish(&p.k_s.0, algos.kex.pubkey(), p.q_s.0)?; // consumes the sharedsecret private key in algos let kex_out = match algos.kex { SharedSecret::KexCurve25519(_) => { - KexCurve25519::secret(&mut algos, p.q_s.0, kex_hash, sess_id)? + KexCurve25519::secret(algos, p.q_s.0, kex_hash, sess_id)? } }; @@ -452,7 +451,7 @@ impl SharedSecret { mut kex: Kex, p: &packets::KexDHInit, sess_id: &Option<SessId>, s: &mut TrafSend, b: &mut dyn ServBehaviour, ) -> Result<KexOutput> { - let mut algos = kex.algos.trap()?; + let mut algos = kex.algos.as_mut().trap()?; // hostkeys list must contain the signature type let hostkey = b.hostkeys()?.iter().find(|k| k.can_sign(algos.hostsig)).trap()?; @@ -460,7 +459,7 @@ impl SharedSecret { kex_hash.prefinish(&hostkey.pubkey(), p.q_c.0, algos.kex.pubkey())?; let (kex_out, kex_pub) = match algos.kex { SharedSecret::KexCurve25519(_) => { - let kex_out = KexCurve25519::secret(&mut algos, p.q_c.0, kex_hash, sess_id)?; + let kex_out = KexCurve25519::secret(algos, p.q_c.0, kex_hash, sess_id)?; (kex_out, algos.kex.pubkey()) } }; @@ -489,7 +488,7 @@ impl SharedSecret { pub(crate) struct KexOutput { pub h: SessId, - pub keys: Option<Keys>, + pub keys: Keys, } impl fmt::Debug for KexOutput { @@ -506,7 +505,7 @@ impl<'a> KexOutput { let h = kex_hash.finish(k); let sess_id = sess_id.as_ref().unwrap_or(&h); - let keys = Some(Keys::new_from(k, &h, &sess_id, algos)?); + let keys = Keys::new_from(k, &h, &sess_id, algos)?; Ok(KexOutput { h, keys }) } @@ -555,10 +554,11 @@ impl KexCurve25519 { } else { return Err(Error::bug()); }; - let ours = kex.ours.take().trap()?; + let ours = kex.ours.as_mut().trap()?; let theirs: [u8; 32] = theirs.try_into().map_err(|_| Error::BadKex)?; let theirs = theirs.try_into().map_err(|_| Error::BadKex)?; let shsec = ours.agree(&theirs); + kex.ours = None; KexOutput::make(&shsec.to_bytes(), algos, kex_hash, sess_id) } } diff --git a/src/ssh_chapoly.rs b/src/ssh_chapoly.rs index 1e6ba76ff86542f355243457426a1bb967d93f3f..a378a6e3fc142c992c1b952417c8b2bb8fe737cc 100644 --- a/src/ssh_chapoly.rs +++ b/src/ssh_chapoly.rs @@ -14,7 +14,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use crate::*; use encrypt::SSH_LENGTH_SIZE; -#[derive(Zeroize, ZeroizeOnDrop)] +#[derive(Clone, ZeroizeOnDrop)] pub struct SSHChaPoly { k1: [u8; 32], k2: [u8; 32],