From b59f8add0ee86beda050564fb561b88a9a814037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Sun, 3 Nov 2024 18:57:12 +0100 Subject: [PATCH] Make sure we use the provided CSPRNG everywhere relevant --- src/account_manager.rs | 61 ++++++++++++++++++------------------- src/groups_v2/manager.rs | 29 ++++++------------ src/master_key.rs | 9 +++--- src/profile_cipher.rs | 35 +++++++++++---------- src/proto.rs | 9 +++--- src/provisioning/cipher.rs | 21 ++++++------- src/provisioning/mod.rs | 2 +- src/provisioning/pipe.rs | 10 +++--- src/push_service/profile.rs | 5 +-- src/sender.rs | 31 +++++++++---------- 10 files changed, 101 insertions(+), 111 deletions(-) diff --git a/src/account_manager.rs b/src/account_manager.rs index af8fc1c88..09f02e012 100644 --- a/src/account_manager.rs +++ b/src/account_manager.rs @@ -1,5 +1,6 @@ use base64::prelude::*; use phonenumber::PhoneNumber; +use rand::{CryptoRng, Rng}; use reqwest::Method; use std::collections::HashMap; use std::convert::{TryFrom, TryInto}; @@ -51,7 +52,8 @@ use crate::{ type Aes256Ctr128BE = ctr::Ctr128BE; -pub struct AccountManager { +pub struct AccountManager { + csprng: R, service: PushService, profile_key: Option, } @@ -72,9 +74,14 @@ pub struct Profile { pub avatar: Option, } -impl AccountManager { - pub fn new(service: PushService, profile_key: Option) -> Self { +impl AccountManager { + pub fn new( + csprng: R, + service: PushService, + profile_key: Option, + ) -> Self { Self { + csprng, service, profile_key, } @@ -87,15 +94,11 @@ impl AccountManager { /// /// Equivalent to Java's RefreshPreKeysJob #[allow(clippy::too_many_arguments)] - #[tracing::instrument(skip(self, protocol_store, csprng))] - pub async fn update_pre_key_bundle< - R: rand::Rng + rand::CryptoRng, - P: PreKeysStore, - >( + #[tracing::instrument(skip(self, protocol_store))] + pub async fn update_pre_key_bundle( &mut self, protocol_store: &mut P, service_id_type: ServiceIdType, - csprng: &mut R, use_last_resort_key: bool, ) -> Result<(), ServiceError> { let prekey_status = match self @@ -154,7 +157,7 @@ impl AccountManager { crate::pre_keys::replenish_pre_keys( protocol_store, &identity_key_pair, - csprng, + &mut self.csprng, use_last_resort_key && !has_last_resort_key, PRE_KEY_BATCH_SIZE, PRE_KEY_BATCH_SIZE, @@ -343,7 +346,7 @@ impl AccountManager { let cipher = ProvisioningCipher::from_public(pub_key); - let encrypted = cipher.encrypt(msg)?; + let encrypted = cipher.encrypt(&mut self.csprng, msg)?; self.send_provisioning_message(ephemeral_id, encrypted) .await?; Ok(()) @@ -379,12 +382,10 @@ impl AccountManager { } pub async fn register_account< - R: rand::Rng + rand::CryptoRng, Aci: PreKeysStore + IdentityKeyStore, Pni: PreKeysStore + IdentityKeyStore, >( &mut self, - csprng: &mut R, registration_method: RegistrationMethod<'_>, account_attributes: AccountAttributes, aci_protocol_store: &mut Aci, @@ -408,7 +409,7 @@ impl AccountManager { ) = crate::pre_keys::replenish_pre_keys( aci_protocol_store, &aci_identity_key_pair, - csprng, + &mut self.csprng, true, 0, 0, @@ -423,7 +424,7 @@ impl AccountManager { ) = crate::pre_keys::replenish_pre_keys( pni_protocol_store, &pni_identity_key_pair, - csprng, + &mut self.csprng, true, 0, 0, @@ -504,7 +505,7 @@ impl AccountManager { .retrieve_profile_by_id(address, Some(profile_key)) .await?; - let profile_cipher = ProfileCipher::from(profile_key); + let profile_cipher = ProfileCipher::new(&mut self.csprng, profile_key); Ok(encrypted_profile.decrypt(profile_cipher)?) } @@ -527,7 +528,8 @@ impl AccountManager { ) -> Result, ProfileManagerError> { let profile_key = self.profile_key.expect("set profile key in AccountManager"); - let profile_cipher = ProfileCipher::from(profile_key); + let mut profile_cipher = + ProfileCipher::new(&mut self.csprng, profile_key); // Profile encryption let name = profile_cipher.encrypt_name(name.as_ref())?; @@ -576,11 +578,8 @@ impl AccountManager { device_name: &str, public_key: &IdentityKey, ) -> Result<(), ServiceError> { - let encrypted_device_name = encrypt_device_name( - &mut rand::thread_rng(), - device_name, - public_key, - )?; + let encrypted_device_name = + encrypt_device_name(&mut self.csprng, device_name, public_key)?; #[derive(Serialize)] #[serde(rename_all = "camelCase")] @@ -641,10 +640,9 @@ impl AccountManager { /// Should be called as the primary device to migrate from pre-PNI to PNI. /// /// This is the equivalent of Android's PnpInitializeDevicesJob or iOS' PniHelloWorldManager. - #[tracing::instrument(skip(self, aci_protocol_store, pni_protocol_store, sender, local_aci, csprng), fields(local_aci = %local_aci))] + #[tracing::instrument(skip(self, aci_protocol_store, pni_protocol_store, sender, local_aci), fields(local_aci = %local_aci))] pub async fn pnp_initialize_devices< // XXX So many constraints here, all imposed by the MessageSender - R: rand::Rng + rand::CryptoRng, Aci: PreKeysStore + SessionStoreExt, Pni: PreKeysStore, AciOrPni: ProtocolStore + SenderKeyStore + SessionStoreExt + Sync + Clone, @@ -655,7 +653,6 @@ impl AccountManager { mut sender: MessageSender, local_aci: ServiceAddress, e164: PhoneNumber, - csprng: &mut R, ) -> Result<(), MessageSenderError> { let pni_identity_key_pair = pni_protocol_store.get_identity_key_pair().await?; @@ -713,7 +710,7 @@ impl AccountManager { crate::pre_keys::replenish_pre_keys( pni_protocol_store, &pni_identity_key_pair, - csprng, + &mut self.csprng, true, 0, 0, @@ -721,16 +718,16 @@ impl AccountManager { .await? } else { // Generate a signed prekey - let signed_pre_key_pair = KeyPair::generate(csprng); + let signed_pre_key_pair = KeyPair::generate(&mut self.csprng); let signed_pre_key_public = signed_pre_key_pair.public_key; let signed_pre_key_signature = pni_identity_key_pair.private_key().calculate_signature( &signed_pre_key_public.serialize(), - csprng, + &mut self.csprng, )?; let signed_prekey_record = SignedPreKeyRecord::new( - csprng.gen_range::(0..0xFFFFFF).into(), + self.csprng.gen_range::(0..0xFFFFFF).into(), Timestamp::now(), &signed_pre_key_pair, &signed_pre_key_signature, @@ -739,7 +736,7 @@ impl AccountManager { // Generate a last-resort Kyber prekey let kyber_pre_key_record = KyberPreKeyRecord::generate( kem::KeyType::Kyber1024, - csprng.gen_range::(0..0xFFFFFF).into(), + self.csprng.gen_range::(0..0xFFFFFF).into(), pni_identity_key_pair.private_key(), )?; ( @@ -754,7 +751,7 @@ impl AccountManager { pni_protocol_store.get_local_registration_id().await? } else { loop { - let regid = generate_registration_id(csprng); + let regid = generate_registration_id(&mut self.csprng); if !pni_registration_ids.iter().any(|(_k, v)| *v == regid) { break regid; } @@ -802,7 +799,7 @@ impl AccountManager { e164.format().mode(phonenumber::Mode::E164).to_string(), ), }), - padding: Some(random_length_padding(csprng, 512)), + padding: Some(random_length_padding(&mut self.csprng, 512)), ..SyncMessage::default() }; let content: ContentBody = msg.into(); diff --git a/src/groups_v2/manager.rs b/src/groups_v2/manager.rs index 567c0edea..3d8cf9839 100644 --- a/src/groups_v2/manager.rs +++ b/src/groups_v2/manager.rs @@ -16,7 +16,7 @@ use base64::prelude::*; use bytes::Bytes; use chrono::{Days, NaiveDate, NaiveTime, Utc}; use futures::AsyncReadExt; -use rand::RngCore; +use rand::{CryptoRng, Rng}; use reqwest::Method; use serde::Deserialize; use zkgroup::{ @@ -152,8 +152,9 @@ impl GroupsManager { } } - pub async fn get_authorization_for_today( + pub async fn get_authorization_for_today( &mut self, + csprng: &mut R, group_secret_params: GroupSecretParams, ) -> Result { let (today, today_plus_7_days) = current_days_seconds(); @@ -190,14 +191,16 @@ impl GroupsManager { }; self.get_authorization_string( + csprng, group_secret_params, auth_credential_response.clone(), today, ) } - fn get_authorization_string( + fn get_authorization_string( &self, + csprng: &mut R, group_secret_params: GroupSecretParams, credential_response: AuthCredentialWithPniResponse, today: u64, @@ -219,7 +222,7 @@ impl GroupsManager { })?; let mut random_bytes = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut random_bytes); + csprng.fill_bytes(&mut random_bytes); let auth_credential_presentation = self .server_public_params @@ -241,21 +244,9 @@ impl GroupsManager { Ok(HttpAuth { username, password }) } - #[deprecated = "please use fetch_encrypted_group and decrypt_group separately, which hide more of the implementation details"] - pub async fn get_group( - &mut self, - group_secret_params: GroupSecretParams, - credentials: HttpAuth, - ) -> Result { - let encrypted_group = self.push_service.get_group(credentials).await?; - let decrypted_group = GroupOperations::new(group_secret_params) - .decrypt_group(encrypted_group)?; - - Ok(decrypted_group) - } - - pub async fn fetch_encrypted_group( + pub async fn fetch_encrypted_group( &mut self, + csprng: &mut R, master_key_bytes: &[u8], ) -> Result { let group_master_key = GroupMasterKey::new( @@ -266,7 +257,7 @@ impl GroupsManager { let group_secret_params = GroupSecretParams::derive_from_master_key(group_master_key); let authorization = self - .get_authorization_for_today(group_secret_params) + .get_authorization_for_today(csprng, group_secret_params) .await?; self.push_service.get_group(authorization).await } diff --git a/src/master_key.rs b/src/master_key.rs index 40e60a27a..ef3c4039e 100644 --- a/src/master_key.rs +++ b/src/master_key.rs @@ -1,3 +1,5 @@ +use rand::{CryptoRng, Rng}; + const MASTER_KEY_LEN: usize = 32; const STORAGE_KEY_LEN: usize = 32; @@ -7,13 +9,10 @@ pub struct MasterKey { } impl MasterKey { - pub fn generate() -> Self { - use rand::Rng; - + pub fn generate(csprng: &mut R) -> Self { // Create random bytes - let mut rng = rand::thread_rng(); let mut inner = [0_u8; MASTER_KEY_LEN]; - rng.fill(&mut inner); + csprng.fill(&mut inner); Self { inner } } diff --git a/src/profile_cipher.rs b/src/profile_cipher.rs index a568b09d6..c711f2076 100644 --- a/src/profile_cipher.rs +++ b/src/profile_cipher.rs @@ -1,6 +1,7 @@ use std::convert::TryInto; use aes_gcm::{aead::Aead, AeadCore, AeadInPlace, Aes256Gcm, KeyInit}; +use rand::{CryptoRng, Rng}; use zkgroup::profiles::ProfileKey; use crate::profile_name::ProfileName; @@ -25,16 +26,11 @@ use crate::profile_name::ProfileName; /// let decrypted = cipher.decrypt_name(&encrypted).unwrap().unwrap(); /// assert_eq!(decrypted.as_ref(), name); /// ``` -pub struct ProfileCipher { +pub struct ProfileCipher { + csprng: R, profile_key: ProfileKey, } -impl From for ProfileCipher { - fn from(profile_key: ProfileKey) -> Self { - ProfileCipher { profile_key } - } -} - const NAME_PADDED_LENGTH_1: usize = 53; const NAME_PADDED_LENGTH_2: usize = 257; const NAME_PADDING_BRACKETS: &[usize] = @@ -77,20 +73,27 @@ fn pad_plaintext( Ok(len) } -impl ProfileCipher { +impl ProfileCipher { + pub fn new(csprng: R, profile_key: ProfileKey) -> Self { + Self { + csprng, + profile_key, + } + } + pub fn into_inner(self) -> ProfileKey { self.profile_key } fn pad_and_encrypt( - &self, + &mut self, mut bytes: Vec, padding_brackets: &[usize], ) -> Result, ProfileCipherError> { let _len = pad_plaintext(&mut bytes, padding_brackets)?; let cipher = Aes256Gcm::new(&self.profile_key.get_bytes().into()); - let nonce = Aes256Gcm::generate_nonce(rand::thread_rng()); + let nonce = Aes256Gcm::generate_nonce(&mut self.csprng); cipher .encrypt_in_place(&nonce, b"", &mut bytes) @@ -137,7 +140,7 @@ impl ProfileCipher { } pub fn encrypt_name<'inp>( - &self, + &mut self, name: impl std::borrow::Borrow>, ) -> Result, ProfileCipherError> { let name = name.borrow(); @@ -157,7 +160,7 @@ impl ProfileCipher { } pub fn encrypt_about( - &self, + &mut self, about: String, ) -> Result, ProfileCipherError> { let bytes = about.into_bytes(); @@ -177,7 +180,7 @@ impl ProfileCipher { } pub fn encrypt_emoji( - &self, + &mut self, emoji: String, ) -> Result, ProfileCipherError> { let bytes = emoji.into_bytes(); @@ -222,7 +225,7 @@ mod tests { let mut rng = rand::thread_rng(); let some_randomness = rng.gen(); let profile_key = ProfileKey::generate(some_randomness); - let cipher = ProfileCipher::from(profile_key); + let mut cipher = ProfileCipher::new(rng, profile_key); for name in &names { let profile_name = ProfileName::<&str> { given_name: name, @@ -247,7 +250,7 @@ mod tests { let mut rng = rand::thread_rng(); let some_randomness = rng.gen(); let profile_key = ProfileKey::generate(some_randomness); - let cipher = ProfileCipher::from(profile_key); + let mut cipher = ProfileCipher::new(rng, profile_key); for &about in &abouts { let encrypted = cipher.encrypt_about(about.into()).unwrap(); @@ -264,7 +267,7 @@ mod tests { let mut rng = rand::thread_rng(); let some_randomness = rng.gen(); let profile_key = ProfileKey::generate(some_randomness); - let cipher = ProfileCipher::from(profile_key); + let mut cipher = ProfileCipher::new(rng, profile_key); for &emoji in &emojii { let encrypted = cipher.encrypt_emoji(emoji.into()).unwrap(); diff --git a/src/proto.rs b/src/proto.rs index f24d08037..4d678e727 100644 --- a/src/proto.rs +++ b/src/proto.rs @@ -1,6 +1,6 @@ #![allow(clippy::all)] -use rand::{Rng, RngCore}; +use rand::{CryptoRng, Rng}; include!(concat!(env!("OUT_DIR"), "/signalservice.rs")); include!(concat!(env!("OUT_DIR"), "/signal.rs")); @@ -69,11 +69,10 @@ impl WebSocketResponseMessage { } impl SyncMessage { - pub fn with_padding() -> Self { - let mut rng = rand::thread_rng(); - let random_size = rng.gen_range(1..=512); + pub fn with_padding(csprng: &mut R) -> Self { + let random_size = csprng.gen_range(1..=512); let mut padding: Vec = vec![0; random_size]; - rng.fill_bytes(&mut padding); + csprng.fill_bytes(&mut padding); Self { padding: Some(padding), diff --git a/src/provisioning/cipher.rs b/src/provisioning/cipher.rs index 7143385bf..526dc372c 100644 --- a/src/provisioning/cipher.rs +++ b/src/provisioning/cipher.rs @@ -7,7 +7,7 @@ use bytes::Bytes; use hmac::{Hmac, Mac}; use libsignal_protocol::{KeyPair, PublicKey}; use prost::Message; -use rand::Rng; +use rand::{CryptoRng, Rng}; use sha2::Sha256; pub use crate::proto::{ProvisionEnvelope, ProvisionMessage}; @@ -55,11 +55,10 @@ pub struct ProvisioningCipher { impl ProvisioningCipher { /// Generate a random key pair - pub fn generate(rng: &mut R) -> Result - where - R: rand::Rng + rand::CryptoRng, - { - let key_pair = libsignal_protocol::KeyPair::generate(rng); + pub fn generate( + mut csprng: R, + ) -> Result { + let key_pair = libsignal_protocol::KeyPair::generate(&mut csprng); Ok(Self { key_material: CipherMode::DecryptAndEncrypt(key_pair), }) @@ -81,14 +80,14 @@ impl ProvisioningCipher { self.key_material.public() } - pub fn encrypt( + pub fn encrypt( &self, + csprng: &mut R, msg: ProvisionMessage, ) -> Result { let msg = msg.encode_to_vec(); - let mut rng = rand::thread_rng(); - let our_key_pair = libsignal_protocol::KeyPair::generate(&mut rng); + let our_key_pair = libsignal_protocol::KeyPair::generate(csprng); let agreement = our_key_pair.calculate_agreement(self.public_key())?; let mut shared_secrets = [0; 64]; @@ -98,7 +97,7 @@ impl ProvisioningCipher { let aes_key = &shared_secrets[0..32]; let mac_key = &shared_secrets[32..]; - let iv: [u8; IV_LENGTH] = rng.gen(); + let iv: [u8; IV_LENGTH] = csprng.gen(); let cipher = cbc::Encryptor::::new(aes_key.into(), &iv.into()); let ciphertext = cipher.encrypt_padded_vec_mut::(&msg); @@ -197,7 +196,7 @@ mod tests { ); let msg = ProvisionMessage::default(); - let encrypted = encrypt_cipher.encrypt(msg.clone())?; + let encrypted = encrypt_cipher.encrypt(&mut rng, msg.clone())?; assert!(matches!( encrypt_cipher.decrypt(encrypted.clone()), diff --git a/src/provisioning/mod.rs b/src/provisioning/mod.rs index f1c16c589..cac4c1015 100644 --- a/src/provisioning/mod.rs +++ b/src/provisioning/mod.rs @@ -158,7 +158,7 @@ pub async fn link_device< let registration_id = csprng.gen_range(1..256); let pni_registration_id = csprng.gen_range(1..256); - let provisioning_pipe = ProvisioningPipe::from_socket(ws)?; + let provisioning_pipe = ProvisioningPipe::from_socket(csprng, ws)?; let provision_stream = provisioning_pipe.stream(); pin_mut!(provision_stream); diff --git a/src/provisioning/pipe.rs b/src/provisioning/pipe.rs index 0842ec895..594049ab1 100644 --- a/src/provisioning/pipe.rs +++ b/src/provisioning/pipe.rs @@ -7,6 +7,7 @@ use futures::{ }, prelude::*, }; +use rand::{CryptoRng, Rng}; use url::Url; pub use crate::proto::{ProvisionEnvelope, ProvisionMessage}; @@ -34,12 +35,13 @@ pub enum ProvisioningStep { } impl ProvisioningPipe { - pub fn from_socket(ws: SignalWebSocket) -> Result { + pub fn from_socket( + csprng: &mut R, + ws: SignalWebSocket, + ) -> Result { Ok(ProvisioningPipe { ws, - provisioning_cipher: ProvisioningCipher::generate( - &mut rand::thread_rng(), - )?, + provisioning_cipher: ProvisioningCipher::generate(csprng)?, }) } diff --git a/src/push_service/profile.rs b/src/push_service/profile.rs index c14b59b91..c427d7115 100644 --- a/src/push_service/profile.rs +++ b/src/push_service/profile.rs @@ -1,3 +1,4 @@ +use rand::{CryptoRng, Rng}; use reqwest::Method; use serde::{Deserialize, Serialize}; use zkgroup::profiles::{ProfileKeyCommitment, ProfileKeyVersion}; @@ -38,9 +39,9 @@ pub struct SignalServiceProfile { } impl SignalServiceProfile { - pub fn decrypt( + pub fn decrypt( &self, - profile_cipher: crate::profile_cipher::ProfileCipher, + profile_cipher: crate::profile_cipher::ProfileCipher, ) -> Result { // Profile decryption let name = self diff --git a/src/sender.rs b/src/sender.rs index 460e45900..8fb959955 100644 --- a/src/sender.rs +++ b/src/sender.rs @@ -196,12 +196,10 @@ where let len = contents.len(); // Encrypt let (key, iv) = { - use rand::RngCore; let mut key = [0u8; 64]; let mut iv = [0u8; 16]; - // thread_rng is guaranteed to be cryptographically secure - rand::thread_rng().fill_bytes(&mut key); - rand::thread_rng().fill_bytes(&mut iv); + self.csprng.fill_bytes(&mut key); + self.csprng.fill_bytes(&mut iv); (key, iv) }; @@ -371,8 +369,8 @@ where // only send a sync message when sending to self and skip the rest of the process if message_to_self && is_multi_device { debug!("sending note to self"); - let sync_message = - Self::create_multi_device_sent_transcript_content( + let sync_message = self + .create_multi_device_sent_transcript_content( Some(recipient), content_body, timestamp, @@ -414,8 +412,8 @@ where if needs_sync || is_multi_device { debug!("sending multi-device sync message"); - let sync_message = - Self::create_multi_device_sent_transcript_content( + let sync_message = self + .create_multi_device_sent_transcript_content( Some(recipient), content_body, timestamp, @@ -488,8 +486,8 @@ where // we only need to send a synchronization message once if needs_sync_in_results || self.is_multi_device().await { - let sync_message = - Self::create_multi_device_sent_transcript_content( + let sync_message = self + .create_multi_device_sent_transcript_content( None, content_body, timestamp, @@ -703,7 +701,7 @@ where blob: Some(ptr), complete: Some(complete), }), - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }; self.send_message( @@ -728,7 +726,7 @@ where ) -> Result<(), MessageSenderError> { let msg = SyncMessage { configuration: Some(configuration), - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }; let ts = Utc::now().timestamp_millis() as u64; @@ -775,7 +773,7 @@ where let msg = SyncMessage { message_request_response, - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }; let ts = Utc::now().timestamp_millis() as u64; @@ -794,7 +792,7 @@ where ) -> Result<(), MessageSenderError> { let msg = SyncMessage { keys: Some(keys), - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }; let ts = Utc::now().timestamp_millis() as u64; @@ -819,7 +817,7 @@ where request: Some(sync_message::Request { r#type: Some(request_type.into()), }), - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }; let ts = Utc::now().timestamp_millis() as u64; @@ -1041,6 +1039,7 @@ where } fn create_multi_device_sent_transcript_content<'a>( + &mut self, recipient: Option<&ServiceAddress>, content_body: ContentBody, timestamp: u64, @@ -1088,7 +1087,7 @@ where unidentified_status, ..Default::default() }), - ..SyncMessage::with_padding() + ..SyncMessage::with_padding(&mut self.csprng) }) } }