Skip to content

Commit

Permalink
fix: rng state reusage (#292)
Browse files Browse the repository at this point in the history 
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>.
  • Loading branch information
@boxdot
boxdot authored Nov 9, 2024
1 parent a8582e5 commit af5273a
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 49 deletions.
21 changes: 11 additions & 10 deletions presage/src/manager/confirmation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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()),
Expand All @@ -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,
Expand Down
6 changes: 2 additions & 4 deletions presage/src/manager/linking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
};
Expand Down
4 changes: 0 additions & 4 deletions presage/src/manager/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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> {
Expand Down
44 changes: 18 additions & 26 deletions presage/src/manager/registered.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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),
};
Expand Down Expand Up @@ -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?;

Expand All @@ -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?;

Expand All @@ -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
};
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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>,
Expand All @@ -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(),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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],
Expand All @@ -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) => {
Expand Down
6 changes: 2 additions & 4 deletions presage/src/manager/registration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -136,7 +135,6 @@ impl<S: Store> Manager<S, Registration> {
password,
session_id: session.id,
},
csprng: rng,
};

Ok(manager)
Expand Down
2 changes: 1 addition & 1 deletion presage/src/store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit af5273a

Please sign in to comment.