diff --git a/sv2/codec-sv2/src/decoder.rs b/sv2/codec-sv2/src/decoder.rs index 6793c13b25..f439a38292 100644 --- a/sv2/codec-sv2/src/decoder.rs +++ b/sv2/codec-sv2/src/decoder.rs @@ -39,8 +39,6 @@ use framing_sv2::{ #[cfg(feature = "noise_sv2")] use framing_sv2::{ENCRYPTED_SV2_FRAME_HEADER_SIZE, SV2_FRAME_CHUNK_SIZE, SV2_FRAME_HEADER_SIZE}; #[cfg(feature = "noise_sv2")] -use noise_sv2::NoiseEngine; -#[cfg(feature = "noise_sv2")] use noise_sv2::NOISE_FRAME_HEADER_SIZE; #[cfg(feature = "noise_sv2")] @@ -49,7 +47,7 @@ use crate::error::Result; use crate::Error::MissingBytes; #[cfg(feature = "noise_sv2")] -use crate::State; +use crate::{State, TransportDecryptState}; #[cfg(not(feature = "with_buffer_pool"))] use buffer_sv2::{Buffer as IsBuffer, BufferFromSystemMemory as Buffer}; @@ -155,31 +153,49 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit } } } - State::Transport(noise_engine) => { - let hint = if IsBuffer::len(&self.sv2_buffer) < SV2_FRAME_HEADER_SIZE { - let len = IsBuffer::len(&self.noise_buffer); - let src = self.noise_buffer.get_data_by_ref(len); - if src.len() < ENCRYPTED_SV2_FRAME_HEADER_SIZE { - ENCRYPTED_SV2_FRAME_HEADER_SIZE - src.len() - } else { - 0 - } - } else { - let src = self.sv2_buffer.get_data_by_ref(SV2_FRAME_HEADER_SIZE); - let header = Header::from_bytes(src)?; - header.encrypted_len() - IsBuffer::len(&self.noise_buffer) - }; + State::Transport(engine) => { + self.next_transport(|buf| engine.decrypt(buf).map_err(Into::into)) + } + } + } - match hint { - 0 => { - self.missing_noise_b = ENCRYPTED_SV2_FRAME_HEADER_SIZE; - self.decode_noise_frame(noise_engine) - } - _ => { - self.missing_noise_b = hint; - Err(Error::MissingBytes(hint)) - } - } + /// Attempts to decode the next Noise frame with the decrypting half of a split [`State`]. + #[inline] + pub fn next_transport_frame( + &mut self, + state: &mut TransportDecryptState, + ) -> Result> { + self.next_transport(|buf| state.decrypt(buf)) + } + + // Decodes a transport-mode frame, decrypting through `decrypt`. + #[inline] + fn next_transport( + &mut self, + decrypt: impl FnMut(&mut B) -> Result<()>, + ) -> Result> { + let hint = if IsBuffer::len(&self.sv2_buffer) < SV2_FRAME_HEADER_SIZE { + let len = IsBuffer::len(&self.noise_buffer); + let src = self.noise_buffer.get_data_by_ref(len); + if src.len() < ENCRYPTED_SV2_FRAME_HEADER_SIZE { + ENCRYPTED_SV2_FRAME_HEADER_SIZE - src.len() + } else { + 0 + } + } else { + let src = self.sv2_buffer.get_data_by_ref(SV2_FRAME_HEADER_SIZE); + let header = Header::from_bytes(src)?; + header.encrypted_len() - IsBuffer::len(&self.noise_buffer) + }; + + match hint { + 0 => { + self.missing_noise_b = ENCRYPTED_SV2_FRAME_HEADER_SIZE; + self.decode_noise_frame(decrypt) + } + _ => { + self.missing_noise_b = hint; + Err(Error::MissingBytes(hint)) } } } @@ -255,7 +271,32 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit // additional bytes required to fully decrypt the frame. Once all bytes are available, the // decryption process completes and the frame can be successfully decoded. #[inline] - fn decode_noise_frame(&mut self, noise_engine: &mut NoiseEngine) -> Result> { + fn decode_noise_frame( + &mut self, + decrypt: impl FnMut(&mut B) -> Result<()>, + ) -> Result> { + let result = self.try_decode_noise_frame(decrypt); + + match &result { + // `MissingBytes` is the normal way out of the header round, so the buffer has to be + // left exactly as it is. + Err(Error::MissingBytes(_)) | Ok(_) => {} + Err(_) => { + // Not a no-op: the decrypt offset and the plaintext decrypted so far persist across + // calls, so without this the next frame is decrypted at the failing chunk's offset. + self.sv2_buffer.danger_set_start(0); + self.sv2_buffer.get_data_owned(); + } + } + + result + } + + #[inline] + fn try_decode_noise_frame( + &mut self, + mut decrypt: impl FnMut(&mut B) -> Result<()>, + ) -> Result> { match ( IsBuffer::len(&self.noise_buffer), IsBuffer::len(&self.sv2_buffer), @@ -268,7 +309,7 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit .get_writable(ENCRYPTED_SV2_FRAME_HEADER_SIZE); decrypted_header.copy_from_slice(src.as_ref()); self.sv2_buffer.as_ref(); - noise_engine.decrypt(&mut self.sv2_buffer)?; + decrypt(&mut self.sv2_buffer)?; let header = Header::from_bytes(self.sv2_buffer.get_data_by_ref(SV2_FRAME_HEADER_SIZE))?; self.missing_noise_b = header.encrypted_len(); @@ -292,7 +333,7 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit let decrypted_payload = self.sv2_buffer.get_writable(end - start); decrypted_payload.copy_from_slice(&encrypted_payload.as_ref()[start..end]); self.sv2_buffer.danger_set_start(decrypted_len); - noise_engine.decrypt(&mut self.sv2_buffer)?; + decrypt(&mut self.sv2_buffer)?; start = end; end = (start + SV2_FRAME_CHUNK_SIZE).min(encrypted_payload_len); decrypted_len += self.sv2_buffer.as_ref().len(); @@ -798,6 +839,47 @@ mod prop_tests { } } + #[cfg(feature = "noise_sv2")] + #[test] + fn noise_decoder_recovers_from_a_failed_decryption() { + let (mut sender_state, mut receiver_state) = make_transport_state_pair(); + let frame = Frame::Sv2( + Sv2Frame::::from_message(TestMessage { value: 7 }, 0, 0, false) + .unwrap(), + ); + let mut encoder = NoiseEncoder::::new(); + let encrypted = encoder.encode(frame, &mut sender_state).unwrap(); + let encrypted: &[u8] = encrypted.as_ref(); + + let mut decoder = StandardNoiseDecoder::::new(); + + // Fail on the encrypted header. The closure never touches `receiver_state`, so its nonce + // stays where it was and the same bytes can be replayed below. + let hint = decoder + .next_transport(|_| Err(crate::Error::UnexpectedNoiseState)) + .unwrap_err(); + assert!(matches!(hint, crate::Error::MissingBytes(_))); + let writable = decoder.writable(); + let len = writable.len(); + writable.copy_from_slice(&encrypted[..len]); + let failed = decoder + .next_transport(|_| Err(crate::Error::UnexpectedNoiseState)) + .unwrap_err(); + assert!(matches!(failed, crate::Error::UnexpectedNoiseState)); + assert_eq!(IsBuffer::len(&decoder.sv2_buffer), 0); + assert!(decoder.sv2_buffer.as_ref().is_empty()); + + // The same decoder must now decode the frame from the start. + let decoded = decode_noise_frame(&mut decoder, &mut receiver_state, encrypted); + match decoded { + Some(mut f) => assert_eq!( + binary_sv2::from_bytes::(f.payload()).unwrap(), + TestMessage { value: 7 } + ), + None => panic!("failed to decode the frame after a failed decryption"), + } + } + /// Verifies that a single `StandardNoiseDecoder` instance correctly /// decodes two consecutive noise-encrypted frames in sequence using /// the same shared transport state. diff --git a/sv2/codec-sv2/src/encoder.rs b/sv2/codec-sv2/src/encoder.rs index 08d2290858..957e2e75ca 100644 --- a/sv2/codec-sv2/src/encoder.rs +++ b/sv2/codec-sv2/src/encoder.rs @@ -40,7 +40,7 @@ use noise_sv2::AEAD_MAC_LEN; use tracing::error; #[cfg(feature = "noise_sv2")] -use crate::{Error, Result, State}; +use crate::{Error, Result, State, TransportEncryptState}; #[cfg(not(feature = "with_buffer_pool"))] use buffer_sv2::{Buffer as IsBuffer, BufferFromSystemMemory as Buffer}; @@ -130,45 +130,8 @@ impl WithNoise { #[inline] pub fn encode(&mut self, item: Item, state: &mut State) -> Result { match state { - State::Transport(noise_engine) => { - let len = item.encoded_length(); - let writable = self.sv2_buffer.get_writable(len); - - // ENCODE THE SV2 FRAME - let i: Sv2Frame = item.try_into().map_err(|e| { - #[cfg(feature = "tracing")] - error!("Error while encoding 1 frame: {:?}", e); - Error::FramingError(e) - })?; - i.serialize(writable)?; - - let sv2 = self.sv2_buffer.get_data_owned(); - let sv2: &[u8] = sv2.as_ref(); - - // ENCRYPT THE HEADER - let to_encrypt = self.noise_buffer.get_writable(SV2_FRAME_HEADER_SIZE); - to_encrypt.copy_from_slice(&sv2[..SV2_FRAME_HEADER_SIZE]); - noise_engine.encrypt(&mut self.noise_buffer)?; - - // ENCRYPT THE PAYLOAD IN CHUNKS - let mut start = SV2_FRAME_HEADER_SIZE; - let mut end = if sv2.len() - start < (SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN) { - sv2.len() - } else { - SV2_FRAME_CHUNK_SIZE + start - AEAD_MAC_LEN - }; - let mut encrypted_len = ENCRYPTED_SV2_FRAME_HEADER_SIZE; - - while start < sv2.len() { - let to_encrypt = self.noise_buffer.get_writable(end - start); - to_encrypt.copy_from_slice(&sv2[start..end]); - self.noise_buffer.danger_set_start(encrypted_len); - noise_engine.encrypt(&mut self.noise_buffer)?; - encrypted_len += self.noise_buffer.as_ref().len(); - start = end; - end = (start + SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN).min(sv2.len()); - } - self.noise_buffer.danger_set_start(0); + State::Transport(engine) => { + self.encrypt_frame(item, |buf| engine.encrypt(buf).map_err(Into::into))? } State::HandShake(_) => self.while_handshaking(item)?, State::NotInitialized(_) => self.while_handshaking(item)?, @@ -180,6 +143,90 @@ impl WithNoise { Ok(self.noise_buffer.get_data_owned()) } + /// Encodes an Sv2 frame and encrypts it with the encrypting half of a split [`State`]. + #[inline] + pub fn encode_transport( + &mut self, + item: Item, + state: &mut TransportEncryptState, + ) -> Result { + self.encrypt_frame(item, |buf| state.encrypt(buf))?; + + // Clear sv2_buffer + self.sv2_buffer.get_data_owned(); + // Return noise_buffer + Ok(self.noise_buffer.get_data_owned()) + } + + // Serializes `item` into an Sv2 frame and encrypts it in place through `encrypt`, leaving the + // buffers clean if any step of that fails. + #[inline] + fn encrypt_frame( + &mut self, + item: Item, + encrypt: impl FnMut(&mut B) -> Result<()>, + ) -> Result<()> { + let result = self.try_encrypt_frame(item, encrypt); + + if result.is_err() { + // Not a no-op: the write offset and the bytes written so far persist across calls, so + // without this the next frame is written at the failing chunk's offset with the remains + // of this one in front of it. + self.noise_buffer.danger_set_start(0); + self.noise_buffer.get_data_owned(); + self.sv2_buffer.get_data_owned(); + } + + result + } + + #[inline] + fn try_encrypt_frame( + &mut self, + item: Item, + mut encrypt: impl FnMut(&mut B) -> Result<()>, + ) -> Result<()> { + let len = item.encoded_length(); + let writable = self.sv2_buffer.get_writable(len); + + // ENCODE THE SV2 FRAME + let i: Sv2Frame = item.try_into().map_err(|e| { + #[cfg(feature = "tracing")] + error!("Error while encoding 1 frame: {:?}", e); + Error::FramingError(e) + })?; + i.serialize(writable)?; + + let sv2 = self.sv2_buffer.get_data_owned(); + let sv2: &[u8] = sv2.as_ref(); + + // ENCRYPT THE HEADER + let to_encrypt = self.noise_buffer.get_writable(SV2_FRAME_HEADER_SIZE); + to_encrypt.copy_from_slice(&sv2[..SV2_FRAME_HEADER_SIZE]); + encrypt(&mut self.noise_buffer)?; + + // ENCRYPT THE PAYLOAD IN CHUNKS + let mut start = SV2_FRAME_HEADER_SIZE; + let mut end = if sv2.len() - start < (SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN) { + sv2.len() + } else { + SV2_FRAME_CHUNK_SIZE + start - AEAD_MAC_LEN + }; + let mut encrypted_len = ENCRYPTED_SV2_FRAME_HEADER_SIZE; + + while start < sv2.len() { + let to_encrypt = self.noise_buffer.get_writable(end - start); + to_encrypt.copy_from_slice(&sv2[start..end]); + self.noise_buffer.danger_set_start(encrypted_len); + encrypt(&mut self.noise_buffer)?; + encrypted_len += self.noise_buffer.as_ref().len(); + start = end; + end = (start + SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN).min(sv2.len()); + } + self.noise_buffer.danger_set_start(0); + Ok(()) + } + // Encodes Sv2 frames during the handshake phase of the Noise protocol. // // Used when the encoder is in the handshake phase, before secure communication is fully @@ -296,7 +343,10 @@ mod prop_tests { use super::*; #[cfg(feature = "noise_sv2")] use crate::{HandshakeRole, State}; - use binary_sv2::Serialize; + use binary_sv2::{Deserialize, Serialize}; + // Not redundant: the glob import above brings in `crate::Result`, whose single type parameter + // the code generated by the `Deserialize` derive cannot use. + use core::result::Result; #[cfg(feature = "noise_sv2")] use framing_sv2::framing::Frame; use framing_sv2::framing::Sv2Frame; @@ -318,7 +368,7 @@ mod prop_tests { type Slice = ::Slice; - #[derive(Debug, Clone, Serialize, PartialEq, Eq)] + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] struct TestMessage { value: u16, } @@ -450,6 +500,62 @@ mod prop_tests { } } + #[cfg(feature = "noise_sv2")] + #[test] + fn noise_encoder_recovers_from_a_failed_chunk_encryption() { + let frame = Frame::Sv2( + Sv2Frame::::from_message(TestMessage { value: 1 }, 0, 0, false) + .unwrap(), + ); + + let mut encoder = NoiseEncoder::::new(); + + // Let the header through and fail on the first payload chunk: that is the point where the + // write offset has already been moved. + let mut calls = 0; + let result = encoder.encrypt_frame(frame, |buf| { + calls += 1; + if calls == 1 { + // Stand in for the header encryption, which grows the buffer by a MAC. + buf.get_writable(AEAD_MAC_LEN); + Ok(()) + } else { + Err(crate::Error::UnexpectedNoiseState) + } + }); + assert!(result.is_err()); + assert_eq!(IsBuffer::len(&encoder.noise_buffer), 0); + assert!(encoder.noise_buffer.as_ref().is_empty()); + + // The next frame must be one the peer can open, with nothing of the failed one in it. + let (mut sender_state, mut receiver_state) = make_transport_state_pair(); + let next = Frame::Sv2( + Sv2Frame::::from_message(TestMessage { value: 2 }, 0, 0, false) + .unwrap(), + ); + let encrypted = encoder.encode(next, &mut sender_state).unwrap(); + + let mut decoder = crate::StandardNoiseDecoder::::new(); + let mut offset = 0; + let mut decoded = loop { + let writable = decoder.writable(); + let len = writable.len(); + writable.copy_from_slice(&encrypted[offset..offset + len]); + offset += len; + + match decoder.next_frame(&mut receiver_state) { + Ok(Frame::Sv2(f)) => break f, + Ok(_) => panic!("expected an Sv2 frame"), + Err(crate::Error::MissingBytes(_)) => {} + Err(e) => panic!("failed to decode the frame after a failed encode: {e:?}"), + } + }; + assert_eq!( + binary_sv2::from_bytes::(decoded.payload()).unwrap(), + TestMessage { value: 2 } + ); + } + /// Verifies that `NoiseEncoder` can encrypt multiple frames /// sequentially with the same transport state without errors. #[cfg(feature = "noise_sv2")] diff --git a/sv2/codec-sv2/src/lib.rs b/sv2/codec-sv2/src/lib.rs index d001aca843..4659b55b8a 100644 --- a/sv2/codec-sv2/src/lib.rs +++ b/sv2/codec-sv2/src/lib.rs @@ -40,9 +40,11 @@ extern crate alloc; #[cfg(feature = "noise_sv2")] use alloc::boxed::Box; #[cfg(feature = "noise_sv2")] +use buffer_sv2::AeadBuffer; +#[cfg(feature = "noise_sv2")] use framing_sv2::framing::{handshake_message_to_frame as h2f, HandShakeFrame}; #[cfg(feature = "noise_sv2")] -use noise_sv2::NoiseEngine; +use noise_sv2::{NoiseDecryptor, NoiseEncryptor, NoiseEngine}; mod decoder; mod encoder; @@ -70,7 +72,7 @@ pub use encoder::NoiseEncoder; /// process accordingly. #[allow(clippy::large_enum_variant)] #[cfg(feature = "noise_sv2")] -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum HandshakeRole { /// The initiator role in the Noise handshake process. /// @@ -94,7 +96,7 @@ pub enum HandshakeRole { /// [`State::HandShake`] and finally to transport mode [`State::Transport`] as the encryption /// handshake is completed. #[cfg(feature = "noise_sv2")] -#[derive(Debug, Clone)] +#[derive(Debug)] #[allow(clippy::large_enum_variant)] pub enum State { /// The codec has not been initialized yet. @@ -119,6 +121,42 @@ pub enum State { Transport(NoiseEngine), } +/// The encrypting half of a transport-mode [`State`], used by the encoder's transport path. +#[cfg(feature = "noise_sv2")] +#[derive(Debug)] +pub struct TransportEncryptState { + encryption: NoiseEncryptor, +} + +#[cfg(feature = "noise_sv2")] +impl TransportEncryptState { + // Encrypts `msg` in place with the outgoing cipher. + pub(crate) fn encrypt( + &mut self, + msg: &mut T, + ) -> core::result::Result<(), Error> { + self.encryption.encrypt(msg).map_err(Into::into) + } +} + +/// The decrypting half of a transport-mode [`State`], used by the decoder's transport path. +#[cfg(feature = "noise_sv2")] +#[derive(Debug)] +pub struct TransportDecryptState { + decryption: NoiseDecryptor, +} + +#[cfg(feature = "noise_sv2")] +impl TransportDecryptState { + // Decrypts `msg` in place with the incoming cipher. + pub(crate) fn decrypt( + &mut self, + msg: &mut T, + ) -> core::result::Result<(), Error> { + self.decryption.decrypt(msg).map_err(Into::into) + } +} + #[cfg(feature = "noise_sv2")] impl State { /// Initiates the first step of the handshake process for the initiator. @@ -277,12 +315,230 @@ impl State { pub fn with_transport_mode(tm: NoiseEngine) -> Self { Self::Transport(tm) } + + /// Returns whether this state is in transport mode, and so can be split into its two halves. + /// + /// [`Self::split_transport`] consumes the state on its error path as well, so this is the way + /// to check before committing to the call. + pub fn is_transport(&self) -> bool { + matches!(self, Self::Transport(_)) + } + + /// Splits a transport-mode state into its encrypting and decrypting halves, consuming it. + /// + /// The state is consumed whatever the outcome: a state that is not in transport mode is dropped + /// rather than handed back, so callers that cannot guarantee the mode should check + /// [`Self::is_transport`] first. + pub fn split_transport( + self, + ) -> core::result::Result<(TransportEncryptState, TransportDecryptState), Error> { + match self { + Self::Transport(engine) => { + let (encryption, decryption) = engine.into_split(); + Ok(( + TransportEncryptState { encryption }, + TransportDecryptState { decryption }, + )) + } + _ => Err(Error::UnexpectedNoiseState), + } + } } #[cfg(test)] #[cfg(feature = "noise_sv2")] mod tests { - use super::*; + use crate::{ + Error, HandshakeRole, NoiseEncoder, StandardEitherFrame, StandardNoiseDecoder, + StandardSv2Frame, State, TransportDecryptState, TransportEncryptState, + }; + use binary_sv2::{Deserialize, Serialize, B064K}; + use framing_sv2::{framing::Sv2Frame, SV2_FRAME_CHUNK_SIZE}; + use key_utils::{Secp256k1PublicKey, Secp256k1SecretKey}; + use noise_sv2::{ + Initiator, Responder, AEAD_MAC_LEN, ELLSWIFT_ENCODING_SIZE, + INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE, + }; + + const AUTHORITY_PUBLIC_K: &str = "9auqWEzQDVyd2oe1JVGFLMLHZtCo2FFqZwtKA5gd9xbuEu7PH72"; + const AUTHORITY_PRIVATE_K: &str = "mkDLTBBRxdBv998612qipDYoTK3YUrqLe8uWw7gu3iXbSrn2n"; + const CERT_VALIDITY: core::time::Duration = core::time::Duration::from_secs(3600); + const MSG_TYPE: u8 = 0xff; + + #[derive(Debug, Clone, Serialize, Deserialize)] + struct TestMsg { + nonce: u16, + } + + // A message whose payload can be made large enough to span more than one chunk. + #[derive(Debug, Serialize, Deserialize)] + struct ChunkedMsg<'decoder> { + data: B064K<'decoder>, + } + + fn round_trip( + encoder: &mut NoiseEncoder, + decoder: &mut StandardNoiseDecoder, + enc: &mut TransportEncryptState, + dec: &mut TransportDecryptState, + nonce: u16, + ) -> u16 { + let frame = StandardEitherFrame::::Sv2( + Sv2Frame::from_message(TestMsg { nonce }, MSG_TYPE, 0, false).unwrap(), + ); + let encrypted = encoder.encode_transport(frame, enc).unwrap(); + + let mut offset = 0; + loop { + let writable = decoder.writable(); + let len = writable.len(); + writable.copy_from_slice(&encrypted[offset..offset + len]); + offset += len; + + match decoder.next_transport_frame(dec) { + Ok(frame) => { + let mut frame: StandardSv2Frame = frame.try_into().unwrap(); + assert_eq!(frame.get_header().unwrap().msg_type(), MSG_TYPE); + let msg: TestMsg = binary_sv2::from_bytes(frame.payload()).unwrap(); + return msg.nonce; + } + Err(Error::MissingBytes(_)) => {} + Err(e) => panic!("failed to decode a transport frame: {e:?}"), + } + } + } + + fn transport_halves() -> ( + TransportEncryptState, + TransportDecryptState, + TransportEncryptState, + TransportDecryptState, + ) { + let authority_public_k: Secp256k1PublicKey = + AUTHORITY_PUBLIC_K.to_string().try_into().unwrap(); + let authority_private_k: Secp256k1SecretKey = + AUTHORITY_PRIVATE_K.to_string().try_into().unwrap(); + + let mut initiator_state = State::initialized(HandshakeRole::Initiator( + Initiator::from_raw_k(authority_public_k.into_bytes()).unwrap(), + )); + let mut responder_state = State::initialized(HandshakeRole::Responder( + Responder::from_authority_kp( + &authority_public_k.into_bytes(), + &authority_private_k.into_bytes(), + CERT_VALIDITY, + ) + .unwrap(), + )); + + let first_message: [u8; ELLSWIFT_ENCODING_SIZE] = initiator_state + .step_0() + .unwrap() + .get_payload_when_handshaking() + .try_into() + .unwrap(); + let (second_message, responder_state) = responder_state.step_1(first_message).unwrap(); + let second_message: [u8; INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE] = second_message + .get_payload_when_handshaking() + .try_into() + .unwrap(); + let initiator_state = initiator_state.step_2(second_message).unwrap(); + + assert!(initiator_state.is_transport()); + assert!(responder_state.is_transport()); + + let (initiator_enc, initiator_dec) = initiator_state.split_transport().unwrap(); + let (responder_enc, responder_dec) = responder_state.split_transport().unwrap(); + (initiator_enc, initiator_dec, responder_enc, responder_dec) + } + + #[test] + fn split_transport_round_trips_in_both_directions() { + let (mut initiator_enc, mut initiator_dec, mut responder_enc, mut responder_dec) = + transport_halves(); + let mut encoder = NoiseEncoder::::new(); + + // One decoder per direction, kept for every frame, as a connection would: leftover buffer + // state carries from one frame to the next here. + let mut to_responder = StandardNoiseDecoder::::new(); + let mut to_initiator = StandardNoiseDecoder::::new(); + + // Each half keeps its own cipher and nonce counter, so the two sides only stay in step + // across repeated frames if every frame is sealed and opened by the matching direction. + for nonce in 0..8u16 { + assert_eq!( + round_trip( + &mut encoder, + &mut to_responder, + &mut initiator_enc, + &mut responder_dec, + nonce + ), + nonce + ); + assert_eq!( + round_trip( + &mut encoder, + &mut to_initiator, + &mut responder_enc, + &mut initiator_dec, + nonce + 100 + ), + nonce + 100 + ); + } + } + + #[test] + fn split_transport_round_trips_a_frame_that_spans_several_chunks() { + let (mut initiator_enc, _, _, mut responder_dec) = transport_halves(); + + // The encoder seals, and the decoder opens, at most `SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN` + // bytes of frame at a time, so a payload past that boundary is what makes both of them + // loop. Anything smaller only ever exercises the single-chunk path. + let mut data = vec![0xab; u16::MAX as usize]; + assert!(data.len() > SV2_FRAME_CHUNK_SIZE - AEAD_MAC_LEN); + let msg = ChunkedMsg { + data: (&mut data[..]).try_into().unwrap(), + }; + + let frame = StandardEitherFrame::::Sv2( + Sv2Frame::from_message(msg, MSG_TYPE, 0, false).unwrap(), + ); + let mut encoder = NoiseEncoder::::new(); + let encrypted = encoder.encode_transport(frame, &mut initiator_enc).unwrap(); + + let mut decoder = StandardNoiseDecoder::::new(); + let mut offset = 0; + loop { + let writable = decoder.writable(); + let len = writable.len(); + writable.copy_from_slice(&encrypted[offset..offset + len]); + offset += len; + + match decoder.next_transport_frame(&mut responder_dec) { + Ok(frame) => { + let mut frame: StandardSv2Frame = frame.try_into().unwrap(); + assert_eq!(frame.get_header().unwrap().msg_type(), MSG_TYPE); + let decoded: ChunkedMsg = binary_sv2::from_bytes(frame.payload()).unwrap(); + assert_eq!(decoded.data.as_bytes(), &vec![0xab; u16::MAX as usize][..]); + break; + } + Err(Error::MissingBytes(_)) => {} + Err(e) => panic!("failed to decode a chunked transport frame: {e:?}"), + } + } + } + + #[test] + fn is_transport_reports_whether_the_state_can_be_split() { + let state = State::NotInitialized(32); + assert!(!state.is_transport()); + assert_eq!( + state.split_transport().unwrap_err(), + Error::UnexpectedNoiseState + ); + } #[test] fn handshake_step_fails_if_state_is_not_initialized() { diff --git a/sv2/noise-sv2/src/aed_cipher.rs b/sv2/noise-sv2/src/aed_cipher.rs index 4908831be6..a42e118a92 100644 --- a/sv2/noise-sv2/src/aed_cipher.rs +++ b/sv2/noise-sv2/src/aed_cipher.rs @@ -21,7 +21,7 @@ use chacha20poly1305::{ aead::{Buffer, Error}, - AeadInPlace, ChaCha20Poly1305, ChaChaPoly1305, KeyInit, + AeadInPlace, ChaCha20Poly1305, ChaChaPoly1305, Key, KeyInit, }; // Defines the interface for AEAD ciphers. @@ -67,8 +67,10 @@ pub trait AeadCipher { } impl AeadCipher for ChaCha20Poly1305 { - fn from_key(k: [u8; 32]) -> Self { - ChaChaPoly1305::new(&k.into()) + fn from_key(mut k: [u8; 32]) -> Self { + let cipher = ChaChaPoly1305::new(Key::from_slice(&k[..])); + crate::zeroize_bytes(&mut k); + cipher } fn encrypt( diff --git a/sv2/noise-sv2/src/cipher_state.rs b/sv2/noise-sv2/src/cipher_state.rs index 5838cb81d6..31dad966dc 100644 --- a/sv2/noise-sv2/src/cipher_state.rs +++ b/sv2/noise-sv2/src/cipher_state.rs @@ -139,7 +139,6 @@ where // It stores the optional encryption key, the nonce, and the optional cipher instance itself. The // [`CipherState`] trait is implemented to provide a consistent interface for managing cipher // state across different AEAD ciphers. -#[derive(Clone)] pub struct Cipher { // Optional 32-byte encryption key. k: Option<[u8; 32]>, @@ -149,25 +148,24 @@ pub struct Cipher { cipher: Option, } -// Ensures that the `Cipher` type is not `Sync`, which prevents multiple threads from -// simultaneously accessing the same instance of `Cipher`. This eliminates the need to handle -// potential issues related to visibility of changes across threads. +// Nonce uniqueness rests on exclusive access, not on thread affinity: `Cipher` is both `Send` and +// `Sync`, so sharing one instance across threads is allowed and still requires the caller to +// synchronize. What prevents two encryptions from reusing a nonce is that `encrypt`/`decrypt` take +// `&mut self` and that the type is deliberately not `Clone`, so the key and its nonce counter can +// never be duplicated or advanced from two places at once. // -// After sending the `k` value, we immediately clear it to prevent the original thread from -// accessing the value again, thereby enhancing security by ensuring the sensitive data is no -// longer available in memory. -// -// The `Cipher` struct is neither `Sync` nor `Copy` due to its `cipher` field, which implements -// the `AeadCipher` trait. This trait requires mutable access, making the entire struct non-`Sync` -// and non-`Copy`, even though the key and nonce are simple types. +// The handshake key `k` is cleared as soon as the handshake no longer needs it (see `erase_k`), +// so it does not outlive its use even though the cipher itself lives for the whole session. impl Cipher { // Internal use only, we need k for handshake - pub fn from_key_and_cipher(k: [u8; 32], c: C) -> Self { - Self { + pub fn from_key_and_cipher(mut k: [u8; 32], c: C) -> Self { + let state = Self { k: Some(k), n: 0, cipher: Some(c), - } + }; + crate::zeroize_bytes(&mut k); + state } // Encrypts data in place using an empty additional associated data buffer. diff --git a/sv2/noise-sv2/src/handshake.rs b/sv2/noise-sv2/src/handshake.rs index f597750be8..fbd38fe7f3 100644 --- a/sv2/noise-sv2/src/handshake.rs +++ b/sv2/noise-sv2/src/handshake.rs @@ -33,7 +33,8 @@ use alloc::{string::String, vec::Vec}; use crate::{ - aed_cipher::AeadCipher, cipher_state::CipherState, AeadError, NOISE_HASHED_PROTOCOL_NAME_CHACHA, + aed_cipher::AeadCipher, cipher_state::CipherState, zeroize_bytes, AeadError, + NOISE_HASHED_PROTOCOL_NAME_CHACHA, }; use chacha20poly1305::ChaCha20Poly1305; use secp256k1::{ @@ -109,8 +110,11 @@ pub trait HandshakeOp: CipherState { Self::generate_key_with_rng(&mut rand::thread_rng()) } + // The `CryptoRng` bound requires generators that declare themselves suitable for cryptographic + // use. Seed quality and correct `CryptoRng` implementations remain the caller's + // responsibility. #[inline] - fn generate_key_with_rng(rng: &mut R) -> Keypair { + fn generate_key_with_rng(rng: &mut R) -> Keypair { let secp = Secp256k1::new(); let (mut secret_key, public_key) = secp.generate_keypair(rng); if public_key.x_only_public_key().1 == secp256k1::Parity::Odd { @@ -142,16 +146,26 @@ pub trait HandshakeOp: CipherState { opad[i] = key[i] ^ 0x5c; } - let mut to_hash = Vec::with_capacity(64 + data.len()); + // Sized for the larger of the two rounds (`opad` + a 32-byte digest) so neither one + // reallocates and abandons a copy of the padded key on the heap. + let mut to_hash = Vec::with_capacity(96 + data.len()); to_hash.extend_from_slice(&ipad); to_hash.extend_from_slice(data); - let temp = Sha256Hash::hash(&to_hash).to_byte_array(); + let mut temp = Sha256Hash::hash(&to_hash).to_byte_array(); + // Wiped before `clear`, while the length still spans the padded key. + zeroize_bytes(&mut to_hash); to_hash.clear(); to_hash.extend_from_slice(&opad); to_hash.extend_from_slice(&temp); - Sha256Hash::hash(&to_hash).to_byte_array() + let out = Sha256Hash::hash(&to_hash).to_byte_array(); + + zeroize_bytes(&mut ipad); + zeroize_bytes(&mut opad); + zeroize_bytes(&mut to_hash); + zeroize_bytes(&mut temp); + out } // Derives two new keys using the HKDF (HMAC-based Key Derivation Function) process. @@ -169,9 +183,11 @@ pub trait HandshakeOp: CipherState { // specific byte sequence (`0x02`). // 4. Returns both outputs. fn hkdf_2(chaining_key: &[u8; 32], input_key_material: &[u8]) -> ([u8; 32], [u8; 32]) { - let temp_key = Self::hmac_hash(chaining_key, input_key_material); + let mut temp_key = Self::hmac_hash(chaining_key, input_key_material); let out_1 = Self::hmac_hash(&temp_key, &[0x1]); let out_2 = Self::hmac_hash(&temp_key, &[&out_1[..], &[0x2][..]].concat()); + // Both outputs are recoverable from the pseudorandom key, so it must not outlive them. + zeroize_bytes(&mut temp_key); (out_1, out_2) } @@ -180,10 +196,11 @@ pub trait HandshakeOp: CipherState { chaining_key: &[u8; 32], input_key_material: &[u8], ) -> ([u8; 32], [u8; 32], [u8; 32]) { - let temp_key = Self::hmac_hash(chaining_key, input_key_material); + let mut temp_key = Self::hmac_hash(chaining_key, input_key_material); let out_1 = Self::hmac_hash(&temp_key, &[0x1]); let out_2 = Self::hmac_hash(&temp_key, &[&out_1[..], &[0x2][..]].concat()); let out_3 = Self::hmac_hash(&temp_key, &[&out_2[..], &[0x3][..]].concat()); + zeroize_bytes(&mut temp_key); (out_1, out_2, out_3) } @@ -196,9 +213,13 @@ pub trait HandshakeOp: CipherState { // use in the next step of the handshake. fn mix_key(&mut self, input_key_material: &[u8]) { let ck = self.get_ck(); - let (ck, temp_k) = Self::hkdf_2(ck, input_key_material); + let (mut ck, mut temp_k) = Self::hkdf_2(ck, input_key_material); self.set_ck(ck); self.initialize_key(temp_k); + // `set_ck` and `initialize_key` copy the material they are handed, so the locals holding + // the originals are wiped here. + zeroize_bytes(&mut ck); + zeroize_bytes(&mut temp_k); } // Encrypts the provided plaintext and updates the hash `h` value. @@ -266,7 +287,7 @@ pub trait HandshakeOp: CipherState { // Resets the nonce (`n`) to 0 and initializes the handshake cipher using the given 32-byte // encryption key. It also updates the internal key storage (`k`) with the new key, preparing // the cipher for encrypting or decrypting subsequent messages in the handshake. - fn initialize_key(&mut self, key: [u8; 32]) { + fn initialize_key(&mut self, mut key: [u8; 32]) { self.set_n(0); let cipher = ChaCha20Poly1305::from_key(key); self.set_handshake_cipher(cipher); @@ -276,6 +297,8 @@ pub trait HandshakeOp: CipherState { let set_k = self.get_k(); *set_k = Some(key); } + // The cipher and `k` hold their own copies now, so this one is wiped. + zeroize_bytes(&mut key); } fn set_handshake_cipher(&mut self, cipher: ChaCha20Poly1305); diff --git a/sv2/noise-sv2/src/initiator.rs b/sv2/noise-sv2/src/initiator.rs index 2361571cd1..771748a81e 100644 --- a/sv2/noise-sv2/src/initiator.rs +++ b/sv2/noise-sv2/src/initiator.rs @@ -49,7 +49,7 @@ use crate::{ ENCRYPTED_SIGNATURE_NOISE_MESSAGE_SIZE, INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE, SIGNATURE_NOISE_MESSAGE_SIZE, }; -use chacha20poly1305::{ChaCha20Poly1305, KeyInit}; +use chacha20poly1305::{ChaCha20Poly1305, Key, KeyInit}; use secp256k1::{ ellswift::{ElligatorSwift, ElligatorSwiftParty}, Keypair, PublicKey, XOnlyPublicKey, @@ -60,7 +60,6 @@ use secp256k1::{ /// exchanges, and maintains the handshake hash, chaining key, and nonce for message encryption. /// After the handshake, it facilitates secure communication using [`ChaCha20Poly1305`]. Sensitive /// data is securely erased when no longer needed. -#[derive(Clone)] pub struct Initiator { // Cipher used for encrypting and decrypting messages during the handshake. // @@ -94,17 +93,9 @@ impl core::fmt::Debug for Initiator { } } -// Ensures that the `Cipher` type is not `Sync`, which prevents multiple threads from -// simultaneously accessing the same instance of `Cipher`. This eliminates the need to handle -// potential issues related to visibility of changes across threads. -// -// After sending the `k` value, we immediately clear it to prevent the original thread from -// accessing the value again, thereby enhancing security by ensuring the sensitive data is no -// longer available in memory. -// -// The `Cipher` struct is neither `Sync` nor `Copy` due to its `cipher` field, which implements -// the `AeadCipher` trait. This trait requires mutable access, making the entire struct non-`Sync` -// and non-`Copy`, even though the key and nonce are simple types. +// Every handshake encryption goes through `encrypt_with_ad`, which takes `&mut self`, and +// `Initiator` is deliberately not `Clone`, so the handshake key and its nonce counter cannot be +// duplicated or advanced from two places at once. See `Cipher` for the transport-mode counterpart. impl CipherState for Initiator { fn get_k(&mut self) -> &mut Option<[u8; 32]> { &mut self.k @@ -173,7 +164,7 @@ impl Initiator { /// provided in order to not implicitely rely on `std` and allow `no_std` environments to /// provide a hardware random number generator for example. #[inline] - pub fn new_with_rng( + pub fn new_with_rng( pk: Option, rng: &mut R, ) -> Box { @@ -211,7 +202,7 @@ impl Initiator { /// `std` and allow `no_std` environments to provide a hardware random number generator for /// example. #[inline] - pub fn from_raw_k_with_rng( + pub fn from_raw_k_with_rng( key: [u8; 32], rng: &mut R, ) -> Result, Error> { @@ -239,7 +230,9 @@ impl Initiator { /// `std` and allow `no_std` environments to provide a hardware random number generator for /// example. #[inline] - pub fn without_pk_with_rng(rng: &mut R) -> Result, Error> { + pub fn without_pk_with_rng( + rng: &mut R, + ) -> Result, Error> { Ok(Self::new_with_rng(None, rng)) } @@ -319,7 +312,7 @@ impl Initiator { let elligatorswift_ours_ephemeral = ElligatorSwift::from_pubkey(self.e.public_key()); let elligatorswift_theirs_ephemeral = ElligatorSwift::from_array(elliswift_theirs_ephemeral_serialized); - let ecdh_ephemeral: [u8; 32] = ElligatorSwift::shared_secret( + let mut ecdh_ephemeral: [u8; 32] = ElligatorSwift::shared_secret( elligatorswift_ours_ephemeral, elligatorswift_theirs_ephemeral, e_private_key, @@ -328,6 +321,7 @@ impl Initiator { ) .to_secret_bytes(); self.mix_key(&ecdh_ephemeral); + crate::zeroize_bytes(&mut ecdh_ephemeral); // 5. decrypts next 80 bytes with `DecryptAndHash()` and stores the results as // `rs.public_key` which is **server's static public key** (note that 64 bytes is the @@ -343,7 +337,7 @@ impl Initiator { .expect("slice with incorrect length"); let elligatorswift_theirs_static = ElligatorSwift::from_array(elligatorswift_theirs_static_serialized); - let ecdh_static: [u8; 32] = ElligatorSwift::shared_secret( + let mut ecdh_static: [u8; 32] = ElligatorSwift::shared_secret( elligatorswift_ours_ephemeral, elligatorswift_theirs_static, e_private_key, @@ -352,6 +346,7 @@ impl Initiator { ) .to_secret_bytes(); self.mix_key(&ecdh_static); + crate::zeroize_bytes(&mut ecdh_static); // Decrypt and verify the SignatureNoiseMessage let mut to_decrypt = message[ELLSWIFT_ENCODING_SIZE + ENCRYPTED_ELLSWIFT_ENCODING_SIZE @@ -370,15 +365,19 @@ impl Initiator { .serialize(); let rs_pk_xonly = XOnlyPublicKey::from_slice(&rs_pub_key).unwrap(); if signature_message.verify_with_now(&rs_pk_xonly, &self.responder_authority_pk, now) { - let (temp_k1, temp_k2) = Self::hkdf_2(self.get_ck(), &[]); - let c1 = ChaCha20Poly1305::new(&temp_k1.into()); - let c2 = ChaCha20Poly1305::new(&temp_k2.into()); + let (mut temp_k1, mut temp_k2) = Self::hkdf_2(self.get_ck(), &[]); + let c1 = ChaCha20Poly1305::new(Key::from_slice(&temp_k1[..])); + let c2 = ChaCha20Poly1305::new(Key::from_slice(&temp_k2[..])); let c1: Cipher = Cipher::from_key_and_cipher(temp_k1, c1); let c2: Cipher = Cipher::from_key_and_cipher(temp_k2, c2); let mut encryptor = c1; let mut decryptor = c2; encryptor.erase_k(); decryptor.erase_k(); + // The ciphers keep the only copies of the session keys from here on; `erase_k` above + // wiped the copies the `Cipher`s carried, and these are the ones the handshake used. + crate::zeroize_bytes(&mut temp_k1); + crate::zeroize_bytes(&mut temp_k2); let engine = crate::NoiseEngine { encryptor, decryptor, @@ -391,21 +390,25 @@ impl Initiator { // Securely erases sensitive data from the [`Initiator`] memory. // - // Clears all sensitive cryptographic material within the [`Initiator`] to prevent any - // accidental leakage or misuse. It overwrites the stored keys, chaining key, handshake hash, - // and session ciphers with zeros. This method is typically - // called when the [`Initiator`] instance is no longer needed or before deallocation. + // Volatile-writes zeros over the handshake key (`k`), the chaining key (`ck`) and the handshake + // hash (`h`), and non-securely erases the ephemeral keypair. This method is typically called + // when the [`Initiator`] instance is no longer needed or before deallocation. + // + // It does not touch `handshake_cipher`: that key is wiped by [`ChaCha20Poly1305`]'s own + // zeroize-on-drop. Nor does it reach the transport session ciphers, which by then live in the + // [`NoiseEngine`] returned by [`Self::step_2`] and are wiped when that is dropped. fn erase(&mut self) { if let Some(k) = self.k.as_mut() { for b in k { unsafe { ptr::write_volatile(b, 0) }; } + self.k = None; } - for mut b in self.ck { - unsafe { ptr::write_volatile(&mut b, 0) }; + for b in &mut self.ck { + unsafe { ptr::write_volatile(b, 0) }; } - for mut b in self.h { - unsafe { ptr::write_volatile(&mut b, 0) }; + for b in &mut self.h { + unsafe { ptr::write_volatile(b, 0) }; } self.e.non_secure_erase(); } @@ -432,6 +435,20 @@ mod test { assert!(msg.iter().any(|b| *b != 0)); } + #[test] + #[cfg(feature = "std")] + #[cfg_attr(miri, ignore)] + fn initiator_erase_zeroes_ck_and_h() { + let mut initiator = Initiator::without_pk().unwrap(); + assert!(initiator.ck.iter().any(|b| *b != 0)); + assert!(initiator.h.iter().any(|b| *b != 0)); + + initiator.erase(); + + assert_eq!(initiator.ck, [0u8; 32]); + assert_eq!(initiator.h, [0u8; 32]); + } + #[test] #[cfg(feature = "std")] #[cfg_attr(miri, ignore)] diff --git a/sv2/noise-sv2/src/lib.rs b/sv2/noise-sv2/src/lib.rs index 2b456b804c..490c51eacd 100644 --- a/sv2/noise-sv2/src/lib.rs +++ b/sv2/noise-sv2/src/lib.rs @@ -99,12 +99,27 @@ pub const NOISE_HASHED_PROTOCOL_NAME_CHACHA: [u8; 32] = [ // In this case, `Parity::Even` is used. const PARITY: secp256k1::Parity = secp256k1::Parity::Even; +// Overwrites `bytes` with zeros, using volatile writes so that the compiler cannot elide the wipe +// as a dead store. +// +// Wiping a stack slot is best effort: the optimizer is free to keep the same material in registers +// or in spilled copies that this cannot reach, and intermediate heap buffers (such as the ones +// `HandshakeOp::hmac_hash` builds) are outside of its reach as well. It still removes the +// longest-lived copy of the material, which is the one that survives in the frame after the +// handshake has finished. +pub(crate) fn zeroize_bytes(bytes: &mut [u8]) { + for b in bytes { + unsafe { core::ptr::write_volatile(b, 0) }; + } +} + /// An engine for managing encrypted communication in the Noise protocol. /// /// Manages the encryption and decryption of messages between two parties, the [`Initiator`] and /// [`Responder`], using the Noise protocol. A symmetric cipher is used for both encrypting /// outgoing messages and decrypting incoming messages. -#[derive(Clone)] +/// +/// Call [`Self::into_split`] to divide the engine into its two directional halves. pub struct NoiseEngine { // Cipher to encrypt outgoing messages. encryptor: Cipher, @@ -129,6 +144,54 @@ impl NoiseEngine { pub fn decrypt(&mut self, msg: &mut T) -> Result<(), AeadError> { self.decryptor.decrypt(msg) } + + /// Splits the engine into its sending and receiving halves; consuming it prevents nonce reuse. + pub fn into_split(self) -> (NoiseEncryptor, NoiseDecryptor) { + ( + NoiseEncryptor { + cipher: self.encryptor, + }, + NoiseDecryptor { + cipher: self.decryptor, + }, + ) + } +} + +/// The sending half of a [`NoiseEngine`], owning only the outgoing cipher and its nonce counter. +pub struct NoiseEncryptor { + cipher: Cipher, +} + +impl core::fmt::Debug for NoiseEncryptor { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("NoiseEncryptor").finish() + } +} + +impl NoiseEncryptor { + /// Encrypts a message (`msg`) in place using the stored cipher. + pub fn encrypt(&mut self, msg: &mut T) -> Result<(), AeadError> { + self.cipher.encrypt(msg) + } +} + +/// The receiving half of a [`NoiseEngine`], owning only the incoming cipher and its nonce counter. +pub struct NoiseDecryptor { + cipher: Cipher, +} + +impl core::fmt::Debug for NoiseDecryptor { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("NoiseDecryptor").finish() + } +} + +impl NoiseDecryptor { + /// Decrypts a message (`msg`) in place using the stored cipher. + pub fn decrypt(&mut self, msg: &mut T) -> Result<(), AeadError> { + self.cipher.decrypt(msg) + } } pub use error::Error; diff --git a/sv2/noise-sv2/src/responder.rs b/sv2/noise-sv2/src/responder.rs index e0f5623b8a..f480684915 100644 --- a/sv2/noise-sv2/src/responder.rs +++ b/sv2/noise-sv2/src/responder.rs @@ -49,7 +49,7 @@ use alloc::{ string::{String, ToString}, vec::Vec, }; -use chacha20poly1305::{ChaCha20Poly1305, KeyInit}; +use chacha20poly1305::{ChaCha20Poly1305, Key, KeyInit}; use secp256k1::{ellswift::ElligatorSwift, Keypair, Secp256k1, SecretKey}; const VERSION: u16 = 0; @@ -59,7 +59,6 @@ const VERSION: u16 = 0; /// a connection with the initiator. The responder manages key generation, Diffie-Hellman exchanges, /// message decryption, and state transitions, ensuring secure communication. Sensitive /// cryptographic material is securely erased when no longer needed. -#[derive(Clone)] pub struct Responder { // Cipher used for encrypting and decrypting messages during the handshake. // @@ -99,18 +98,9 @@ impl core::fmt::Debug for Responder { } } -// Ensures that the `Cipher` type is not `Sync`, which prevents multiple threads from -// simultaneously accessing the same instance of `Cipher`. This eliminates the need to handle -// potential issues related to visibility of changes across threads. -// -// After sending the `k` value, we immediately clear it to prevent the original thread from -// accessing the value again, thereby enhancing security by ensuring the sensitive data is no -// longer available in memory. -// -// The `Cipher` struct is neither `Sync` nor `Copy` due to its `cipher` field, which implements -// the `AeadCipher` trait. This trait requires mutable access, making the entire struct non-`Sync` -// and non-`Copy`, even though the key and nonce are simple types. - +// Every handshake encryption goes through `encrypt_with_ad`, which takes `&mut self`, and +// `Responder` is deliberately not `Clone`, so the handshake key and its nonce counter cannot be +// duplicated or advanced from two places at once. See `Cipher` for the transport-mode counterpart. impl CipherState for Responder { fn get_k(&mut self) -> &mut Option<[u8; 32]> { &mut self.k @@ -181,7 +171,7 @@ impl Responder { /// `std` and allow `no_std` environments to provide a hardware random number generator for /// example. #[inline] - pub fn new_with_rng( + pub fn new_with_rng( a: Keypair, cert_validity: u32, rng: &mut R, @@ -225,7 +215,7 @@ impl Responder { /// `std` and allow `no_std` environments to provide a hardware random number generator for /// example. #[inline] - pub fn from_authority_kp_with_rng( + pub fn from_authority_kp_with_rng( public: &[u8; 32], private: &[u8; 32], cert_validity: Duration, @@ -311,7 +301,7 @@ impl Responder { let e_private_key = keypair.secret_key(); let elligatorswift_theirs_ephemeral = ElligatorSwift::from_array(elligatorswift_theirs_ephemeral_serialized); - let ecdh_ephemeral = ElligatorSwift::shared_secret( + let mut ecdh_ephemeral = ElligatorSwift::shared_secret( elligatorswift_theirs_ephemeral, elligatorswitf_ours_ephemeral, e_private_key, @@ -320,6 +310,7 @@ impl Responder { ) .to_secret_bytes(); Self::mix_key(self, &ecdh_ephemeral); + crate::zeroize_bytes(&mut ecdh_ephemeral); // 5. appends `EncryptAndHash(s.public_key)` (64 bytes encrypted elligatorswift public key, // 16 bytes MAC) @@ -336,7 +327,7 @@ impl Responder { // 6. calls `MixKey(ECDH(s.private_key, re.public_key))` let s_private_key = self.s.secret_key(); - let ecdh_static = ElligatorSwift::shared_secret( + let mut ecdh_static = ElligatorSwift::shared_secret( elligatorswift_theirs_ephemeral, elligatorswift_ours_static, s_private_key, @@ -345,6 +336,7 @@ impl Responder { ) .to_secret_bytes(); Self::mix_key(self, &ecdh_static[..]); + crate::zeroize_bytes(&mut ecdh_static); // 7. appends `EncryptAndHash(SIGNATURE_NOISE_MESSAGE)` to the buffer let valid_from = now; @@ -361,9 +353,9 @@ impl Responder { // 9. return pair of CipherState objects, the first for encrypting transport messages from // initiator to responder, and the second for messages in the other direction: let ck = Self::get_ck(self); - let (temp_k1, temp_k2) = Self::hkdf_2(ck, &[]); - let c1 = ChaCha20Poly1305::new(&temp_k1.into()); - let c2 = ChaCha20Poly1305::new(&temp_k2.into()); + let (mut temp_k1, mut temp_k2) = Self::hkdf_2(ck, &[]); + let c1 = ChaCha20Poly1305::new(Key::from_slice(&temp_k1[..])); + let c2 = ChaCha20Poly1305::new(Key::from_slice(&temp_k2[..])); let c1: Cipher = Cipher::from_key_and_cipher(temp_k1, c1); let c2: Cipher = Cipher::from_key_and_cipher(temp_k2, c2); let to_send = out; @@ -371,6 +363,10 @@ impl Responder { let mut decryptor = c1; encryptor.erase_k(); decryptor.erase_k(); + // The ciphers keep the only copies of the session keys from here on; `erase_k` above wiped + // the copies the `Cipher`s carried, and these are the ones the handshake used. + crate::zeroize_bytes(&mut temp_k1); + crate::zeroize_bytes(&mut temp_k2); let engine = crate::NoiseEngine { encryptor, decryptor, @@ -412,21 +408,26 @@ impl Responder { // Securely erases sensitive data in the responder's memory. // - // Clears all sensitive cryptographic material within the [`Responder`] to prevent any - // accidental leakage or misuse. It overwrites the stored keys, chaining key, handshake hash, - // and session ciphers with zeros. This function is typically - // called when the [`Responder`] instance is no longer needed or before deallocation. + // Volatile-writes zeros over the handshake key (`k`), the chaining key (`ck`) and the handshake + // hash (`h`), and non-securely erases the ephemeral, static and authority keypairs. This + // function is typically called when the [`Responder`] instance is no longer needed or before + // deallocation. + // + // It does not touch `handshake_cipher`: that key is wiped by [`ChaCha20Poly1305`]'s own + // zeroize-on-drop. Nor does it reach the transport session ciphers, which by then live in the + // [`NoiseEngine`] returned by [`Self::step_1`] and are wiped when that is dropped. fn erase(&mut self) { if let Some(k) = self.k.as_mut() { for b in k { unsafe { ptr::write_volatile(b, 0) }; } + self.k = None; } - for mut b in self.ck { - unsafe { ptr::write_volatile(&mut b, 0) }; + for b in &mut self.ck { + unsafe { ptr::write_volatile(b, 0) }; } - for mut b in self.h { - unsafe { ptr::write_volatile(&mut b, 0) }; + for b in &mut self.h { + unsafe { ptr::write_volatile(b, 0) }; } self.e.non_secure_erase(); self.s.non_secure_erase(); @@ -477,6 +478,20 @@ mod test { assert_eq!(msg.len(), INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE); } + #[test] + #[cfg(feature = "std")] + #[cfg_attr(miri, ignore)] + fn responder_erase_zeroes_ck_and_h() { + let mut responder = make_responder(); + assert!(responder.ck.iter().any(|b| *b != 0)); + assert!(responder.h.iter().any(|b| *b != 0)); + + responder.erase(); + + assert_eq!(responder.ck, [0u8; 32]); + assert_eq!(responder.h, [0u8; 32]); + } + #[test] #[cfg(feature = "std")] #[cfg_attr(miri, ignore)]