From af5273a18204a3dc07423284f3dc8fa7829cbd13 Mon Sep 17 00:00:00 2001 From: boxdot Date: Sat, 9 Nov 2024 19:18:10 +0100 Subject: [PATCH] fix: rng state reusage (#292) StdRng was cloned, which created a new rng with the same state. This means that the same random numbers were generated: ``` use rand::{Rng, SeedableRng}; use rand::rngs::StdRng; let mut rng1 = StdRng::from_entropy(); let mut rng2 = rng1.clone(); assert_eq!(rng1.gen::(), rng2.gen::()); ``` This commit replaces the `StdRng` with `ThreadRng` in all place where it was used. It also does not store the rng in manager anymore. The credits for finding this bugs go to hrdl <31923882+hrdl-github@users.noreply.github.com>. --- presage/src/manager/confirmation.rs | 21 +++++++------- presage/src/manager/linking.rs | 6 ++-- presage/src/manager/mod.rs | 4 --- presage/src/manager/registered.rs | 44 ++++++++++++----------------- presage/src/manager/registration.rs | 6 ++-- presage/src/store.rs | 2 +- 6 files changed, 34 insertions(+), 49 deletions(-) diff --git a/presage/src/manager/confirmation.rs b/presage/src/manager/confirmation.rs index 5c032847d..27f29ab44 100644 --- a/presage/src/manager/confirmation.rs +++ b/presage/src/manager/confirmation.rs @@ -9,7 +9,7 @@ use libsignal_service::push_service::{ }; use libsignal_service::zkgroup::profiles::ProfileKey; use libsignal_service::AccountManager; -use rand::RngCore; +use rand::{thread_rng, RngCore}; use tracing::trace; use crate::manager::registered::RegistrationData; @@ -35,13 +35,15 @@ impl Manager { /// Returns a [registered manager](Manager::load_registered) that you can use /// to send and receive messages. pub async fn confirm_verification_code( - mut self, + self, confirmation_code: impl AsRef, ) -> Result, Error> { trace!("confirming verification code"); - let registration_id = generate_registration_id(&mut self.csprng); - let pni_registration_id = generate_registration_id(&mut self.csprng); + let mut rng = thread_rng(); + + let registration_id = generate_registration_id(&mut rng); + let pni_registration_id = generate_registration_id(&mut rng); let Confirmation { signal_servers, @@ -75,19 +77,19 @@ impl Manager { // generate a 52 bytes signaling key let mut signaling_key = [0u8; 52]; - self.csprng.fill_bytes(&mut signaling_key); + rng.fill_bytes(&mut signaling_key); // generate a 32 bytes profile key let mut profile_key = [0u8; 32]; - self.csprng.fill_bytes(&mut profile_key); + rng.fill_bytes(&mut profile_key); let profile_key = ProfileKey::generate(profile_key); // generate new identity keys used in `register_account` and below self.store - .set_aci_identity_key_pair(IdentityKeyPair::generate(&mut self.csprng)) + .set_aci_identity_key_pair(IdentityKeyPair::generate(&mut rng)) .await?; self.store - .set_pni_identity_key_pair(IdentityKeyPair::generate(&mut self.csprng)) + .set_pni_identity_key_pair(IdentityKeyPair::generate(&mut rng)) .await?; let skip_device_transfer = true; @@ -100,7 +102,7 @@ impl Manager { number: _, } = account_manager .register_account( - &mut self.csprng, + &mut rng, RegistrationMethod::SessionId(&session.id), AccountAttributes { signaling_key: Some(signaling_key.to_vec()), @@ -126,7 +128,6 @@ impl Manager { trace!("confirmed! (and registered)"); let mut manager = Manager { - csprng: self.csprng, store: self.store, state: Registered::with_data(RegistrationData { signal_servers: self.state.signal_servers, diff --git a/presage/src/manager/linking.rs b/presage/src/manager/linking.rs index d11cfa4d5..e6306c1d2 100644 --- a/presage/src/manager/linking.rs +++ b/presage/src/manager/linking.rs @@ -7,8 +7,7 @@ use libsignal_service::provisioning::{ link_device, NewDeviceRegistration, SecondaryDeviceProvisioning, }; use rand::distributions::{Alphanumeric, DistString}; -use rand::rngs::StdRng; -use rand::{RngCore, SeedableRng}; +use rand::{thread_rng, RngCore}; use tracing::info; use url::Url; @@ -68,7 +67,7 @@ impl Manager { store.clear_registration().await?; // generate a random alphanumeric 24 chars password - let mut rng = StdRng::from_entropy(); + let mut rng = thread_rng(); let password = Alphanumeric.sample_string(&mut rng, 24); // generate a 52 bytes signaling key @@ -158,7 +157,6 @@ impl Manager { ); let mut manager = Manager { - csprng: rng, store: store.clone(), state: Registered::with_data(registration_data), }; diff --git a/presage/src/manager/mod.rs b/presage/src/manager/mod.rs index 1bdef606b..62f334af8 100644 --- a/presage/src/manager/mod.rs +++ b/presage/src/manager/mod.rs @@ -7,8 +7,6 @@ mod registration; use std::fmt; -use rand::rngs::StdRng; - pub use self::confirmation::Confirmation; pub use self::linking::Linking; pub use self::registered::{ReceivingMode, Registered, RegistrationData, RegistrationType}; @@ -27,8 +25,6 @@ pub struct Manager { store: Store, /// Part of the manager which is persisted in the store. state: State, - /// Random number generator - csprng: StdRng, } impl fmt::Debug for Manager { diff --git a/presage/src/manager/registered.rs b/presage/src/manager/registered.rs index e6edb913b..b2ca91252 100644 --- a/presage/src/manager/registered.rs +++ b/presage/src/manager/registered.rs @@ -35,8 +35,8 @@ use libsignal_service::websocket::SignalWebSocket; use libsignal_service::zkgroup::groups::{GroupMasterKey, GroupSecretParams}; use libsignal_service::zkgroup::profiles::ProfileKey; use libsignal_service::{cipher, AccountManager, Profile, ServiceIdExt}; -use rand::rngs::StdRng; -use rand::{CryptoRng, Rng, SeedableRng}; +use rand::rngs::ThreadRng; +use rand::thread_rng; use serde::{Deserialize, Serialize}; use sha2::Digest; use tokio::sync::Mutex; @@ -49,7 +49,7 @@ use crate::store::{ContentsStore, Sticker, StickerPack, StickerPackManifest, Sto use crate::{model::groups::Group, AvatarBytes, Error, Manager}; type ServiceCipher = cipher::ServiceCipher; -type MessageSender = libsignal_service::prelude::MessageSender; +type MessageSender = libsignal_service::prelude::MessageSender; #[derive(Clone, Debug, PartialEq, Eq)] pub enum RegistrationType { @@ -138,7 +138,6 @@ impl Manager { .ok_or(Error::NotYetRegisteredError)?; let mut manager = Self { - csprng: StdRng::from_entropy(), store, state: Registered::with_data(registration_data), }; @@ -250,12 +249,14 @@ impl Manager { Some(self.state.data.profile_key), ); + let mut rng = thread_rng(); + account_manager .update_pre_key_bundle( &mut self.store.aci_protocol_store(), ServiceIdKind::Aci, true, - &mut self.csprng, + &mut rng, ) .await?; @@ -264,7 +265,7 @@ impl Manager { &mut self.store.pni_protocol_store(), ServiceIdKind::Pni, true, - &mut self.csprng, + &mut rng, ) .await?; @@ -284,7 +285,7 @@ impl Manager { pni_registration_id } else { info!("migrating to PNI"); - let pni_registration_id = generate_registration_id(&mut StdRng::from_entropy()); + let pni_registration_id = generate_registration_id(&mut thread_rng()); self.store.save_registration_data(&self.state.data).await?; pni_registration_id }; @@ -360,7 +361,7 @@ impl Manager { request: Some(sync_message::Request { r#type: Some(sync_message::request::Type::Contacts.into()), }), - ..SyncMessage::with_padding(&mut self.csprng) + ..SyncMessage::with_padding(&mut thread_rng()) }; let timestamp = SystemTime::now() @@ -493,7 +494,6 @@ impl Manager { let mut gm = self.groups_manager()?; let Some(group) = upsert_group( - &mut self.csprng, &self.store, &mut gm, context.master_key(), @@ -609,10 +609,10 @@ impl Manager { &mut self, mode: ReceivingMode, ) -> Result, Error> { - struct StreamState { + struct StreamState { store: Store, push_service: PushService, - csprng: R, + csprng: ThreadRng, encrypted_messages: Receiver, message_receiver: MessageReceiver, service_cipher_aci: ServiceCipher, @@ -626,7 +626,7 @@ impl Manager { let init = StreamState { store: self.store.clone(), push_service: push_service.clone(), - csprng: self.csprng.clone(), + csprng: thread_rng(), encrypted_messages: Box::pin(self.receive_messages_encrypted().await?), message_receiver: MessageReceiver::new(push_service), service_cipher_aci: self.new_service_cipher_aci(), @@ -780,7 +780,6 @@ impl Manager { // and the group changes, which are part of the protobuf messages // this means we kinda need our own internal representation of groups inside of presage? if let Ok(Some(group)) = upsert_group( - &mut state.csprng, &state.store, &mut state.groups_manager, master_key_bytes, @@ -940,14 +939,8 @@ impl Manager { let mut sender = self.new_message_sender().await?; let mut groups_manager = self.groups_manager()?; - let Some(group) = upsert_group( - &mut self.csprng, - &self.store, - &mut groups_manager, - &master_key_bytes, - &0, - ) - .await? + let Some(group) = + upsert_group(&self.store, &mut groups_manager, &master_key_bytes, &0).await? else { return Err(Error::UnknownGroup); }; @@ -1196,7 +1189,7 @@ impl Manager { unidentified_websocket, self.identified_push_service(), self.new_service_cipher_aci(), - self.csprng.clone(), + thread_rng(), aci_protocol_store, self.state.data.service_ids.aci, self.state.data.service_ids.pni, @@ -1275,7 +1268,7 @@ impl Manager { account_manager .link_device( - &mut self.csprng, + &mut thread_rng(), secondary, &self.store.aci_protocol_store(), &self.store.pni_protocol_store(), @@ -1324,8 +1317,7 @@ pub enum ReceivingMode { WaitForContacts, } -async fn upsert_group( - csprng: &mut R, +async fn upsert_group( store: &S, groups_manager: &mut GroupsManager, master_key_bytes: &[u8], @@ -1346,7 +1338,7 @@ async fn upsert_group( if upsert_group { debug!("fetching and saving group"); match groups_manager - .fetch_encrypted_group(csprng, master_key_bytes) + .fetch_encrypted_group(&mut thread_rng(), master_key_bytes) .await { Ok(encrypted_group) => { diff --git a/presage/src/manager/registration.rs b/presage/src/manager/registration.rs index 10f62f6f3..62fbd061c 100644 --- a/presage/src/manager/registration.rs +++ b/presage/src/manager/registration.rs @@ -2,8 +2,7 @@ use libsignal_service::configuration::{ServiceConfiguration, SignalServers}; use libsignal_service::prelude::phonenumber::PhoneNumber; use libsignal_service::push_service::{PushService, VerificationTransport}; use rand::distributions::{Alphanumeric, DistString}; -use rand::rngs::StdRng; -use rand::SeedableRng; +use rand::thread_rng; use tracing::trace; use crate::store::Store; @@ -82,7 +81,7 @@ impl Manager { store.clear_registration().await?; // generate a random alphanumeric 24 chars password - let mut rng = StdRng::from_entropy(); + let mut rng = thread_rng(); let password = Alphanumeric.sample_string(&mut rng, 24); let service_configuration: ServiceConfiguration = signal_servers.into(); @@ -136,7 +135,6 @@ impl Manager { password, session_id: session.id, }, - csprng: rng, }; Ok(manager) diff --git a/presage/src/store.rs b/presage/src/store.rs index 36196becc..499b5899d 100644 --- a/presage/src/store.rs +++ b/presage/src/store.rs @@ -542,7 +542,7 @@ pub async fn save_trusted_identity_message( let thread = Thread::Contact(sender.raw_uuid()); let verified_sync_message = Content { metadata: Metadata { - sender: sender, + sender, destination: sender, sender_device: 0, server_guid: None,