From 11e5f9c4e0c199ab4ed6a20102fb0b710f95bc93 Mon Sep 17 00:00:00 2001
From: boxdot <d@zerovolt.org>
Date: Sat, 9 Nov 2024 18:34:00 +0100
Subject: [PATCH] fix: rng state reusage
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::<u32>(), rng2.gen::<u32>());
```
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<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
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<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
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, 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
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<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
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<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
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<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,