From fbf397faae17fa025751992a35c7e8cb61ee4d97 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sat, 3 Dec 2022 23:55:51 +0800
Subject: [PATCH] Avoid Option::take() when we use ZeroizeOnDrop
The Option's contents won't ever get zeroed,
instead we make a copy (or use a reference) and set Option to
None, forcing the drop.
---
src/conn.rs | 13 ++++++++-----
src/encrypt.rs | 9 ++++++---
src/kex.rs | 16 ++++++++--------
src/ssh_chapoly.rs | 2 +-
4 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/src/conn.rs b/src/conn.rs
index d63b346..2edbeef 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 fb0f197..8bca668 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 252c79e..1b8bc63 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 1e6ba76..a378a6e 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],
--
GitLab