fix: rng state reusage

StdRng is sometimes cloned, which creates a new rng with the same rng state. This means that the same random numbers are generated. Also remove rng from the manager, as it is not needed there. The credits for finding this bugs go to hrdl <31923882+hrdl-github@users.noreply.github.com>.
whisperfish · Nov 9, 2024 · 0ba3334 · 0ba3334
1 parent a8582e5
commit 0ba3334
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 49 deletions.
diff --git 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<S: Store> Manager<S, Confirmation> {
     /// 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<str>,
     ) -> Result<Manager<S, Registered>, Error<S::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<S: Store> Manager<S, Confirmation> {
 
         // 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<S: Store> Manager<S, Confirmation> {
             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<S: Store> Manager<S, Confirmation> {
         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
@@ -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<S: Store> Manager<S, Linking> {
         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<S: Store> Manager<S, Linking> {
                 );
 
                 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
@@ -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, State> {
     store: Store,
     /// Part of the manager which is persisted in the store.
     state: State,
-    /// Random number generator
-    csprng: StdRng,
 }
 
 impl<Store, State: fmt::Debug> fmt::Debug for Manager<Store, State> {

diff --git 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<S> = cipher::ServiceCipher<S>;
-type MessageSender<S> = libsignal_service::prelude::MessageSender<S, StdRng>;
+type MessageSender<S> = libsignal_service::prelude::MessageSender<S, ThreadRng>;
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum RegistrationType {
@@ -138,7 +138,6 @@ impl<S: Store> Manager<S, Registered> {
             .ok_or(Error::NotYetRegisteredError)?;
 
         let mut manager = Self {
-            csprng: StdRng::from_entropy(),
             store,
             state: Registered::with_data(registration_data),
         };
@@ -250,12 +249,14 @@ impl<S: Store> Manager<S, Registered> {
             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<S: Store> Manager<S, Registered> {
                 &mut self.store.pni_protocol_store(),
                 ServiceIdKind::Pni,
                 true,
-                &mut self.csprng,
+                &mut rng,
             )
             .await?;
 
@@ -284,7 +285,7 @@ impl<S: Store> Manager<S, Registered> {
                 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<S: Store> Manager<S, Registered> {
             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<S: Store> Manager<S, Registered> {
 
         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<S: Store> Manager<S, Registered> {
         &mut self,
         mode: ReceivingMode,
     ) -> Result<impl Stream<Item = Content>, Error<S::Error>> {
-        struct StreamState<Receiver, Store, AciStore, PniStore, R> {
+        struct StreamState<Receiver, Store, AciStore, PniStore> {
             store: Store,
             push_service: PushService,
-            csprng: R,
+            csprng: ThreadRng,
             encrypted_messages: Receiver,
             message_receiver: MessageReceiver,
             service_cipher_aci: ServiceCipher<AciStore>,
@@ -626,7 +626,7 @@ impl<S: Store> Manager<S, Registered> {
         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<S: Store> Manager<S, Registered> {
                                     // 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<S: Store> Manager<S, Registered> {
         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<S: Store> Manager<S, Registered> {
             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<S: Store> Manager<S, Registered> {
 
         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<R: Rng + CryptoRng, S: Store>(
-    csprng: &mut R,
+async fn upsert_group<S: Store>(
     store: &S,
     groups_manager: &mut GroupsManager<InMemoryCredentialsCache>,
     master_key_bytes: &[u8],
@@ -1346,7 +1338,7 @@ async fn upsert_group<R: Rng + CryptoRng, S: Store>(
     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
@@ -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<S: Store> Manager<S, Registration> {
         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<S: Store> Manager<S, Registration> {
                 password,
                 session_id: session.id,
             },
-            csprng: rng,
         };
 
         Ok(manager)

diff --git a/presage/src/store.rs b/presage/src/store.rs
@@ -542,7 +542,7 @@ pub async fn save_trusted_identity_message<S: Store>(
     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,