From d0baa23f9a15152a370092e955c2cf87891c26e7 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Fri, 11 Nov 2022 10:55:04 +0100 Subject: [PATCH 1/2] Use constant size generic parameter for random bytes generation All uses of `get_random()` were in the form of: `&get_random(vec![0u8; SIZE])` with `SIZE` being a constant. Building a `Vec` is unnecessary for two reasons. First, it uses a very short-lived dynamic memory allocation. Second, a `Vec` is a resizable object, which is useless in those context when random data have a fixed size and will only be read. `get_random_bytes()` takes a constant as a generic parameter and returns an array with the requested number of random bytes. Stack safety analysis: the random bytes will be allocated on the caller stack for a very short time (until the encoding function has been called on the data). In some cases, the random bytes take less room than the `Vec` did (a `Vec` is 24 bytes on a 64 bit computer). The maximum used size is 180 bytes, which makes it for 0.008% of the default stack size for a Rust thread (2MiB), so this is a non-issue. Also, most of the uses of those random bytes are to encode them using an `Encoding`. The function `crypto::encode_random_bytes()` generates random bytes and encode them with the provided `Encoding`, leading to code deduplication. `generate_id()` has also been converted to use a constant generic parameter as well since the length of the requested String is always a constant. --- src/api/core/two_factor/authenticator.rs | 2 +- src/api/core/two_factor/mod.rs | 2 +- src/api/notifications.rs | 2 +- src/crypto.rs | 21 ++++++++++++++------- src/db/models/device.rs | 4 ++-- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/api/core/two_factor/authenticator.rs b/src/api/core/two_factor/authenticator.rs index 7373b581..68342e95 100644 --- a/src/api/core/two_factor/authenticator.rs +++ b/src/api/core/two_factor/authenticator.rs @@ -34,7 +34,7 @@ async fn generate_authenticator(data: JsonUpcase, headers: Headers let (enabled, key) = match twofactor { Some(tf) => (true, tf.data), - _ => (false, BASE32.encode(&crypto::get_random(vec![0u8; 20]))), + _ => (false, crypto::encode_random_bytes::<20>(BASE32)), }; Ok(Json(json!({ diff --git a/src/api/core/two_factor/mod.rs b/src/api/core/two_factor/mod.rs index 78f1318e..3d5eee83 100644 --- a/src/api/core/two_factor/mod.rs +++ b/src/api/core/two_factor/mod.rs @@ -105,7 +105,7 @@ async fn recover(data: JsonUpcase, mut conn: DbConn) -> JsonRe async fn _generate_recover_code(user: &mut User, conn: &mut DbConn) { if user.totp_recover.is_none() { - let totp_recover = BASE32.encode(&crypto::get_random(vec![0u8; 20])); + let totp_recover = crypto::encode_random_bytes::<20>(BASE32); user.totp_recover = Some(totp_recover); user.save(conn).await.ok(); } diff --git a/src/api/notifications.rs b/src/api/notifications.rs index 2657d312..cd53c96d 100644 --- a/src/api/notifications.rs +++ b/src/api/notifications.rs @@ -56,7 +56,7 @@ fn negotiate(_headers: Headers) -> Json { use crate::crypto; use data_encoding::BASE64URL; - let conn_id = BASE64URL.encode(&crypto::get_random(vec![0u8; 16])); + let conn_id = crypto::encode_random_bytes::<16>(BASE64URL); let mut available_transports: Vec = Vec::new(); if CONFIG.websocket_enabled() { diff --git a/src/crypto.rs b/src/crypto.rs index be9680cb..daf5124d 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -3,7 +3,7 @@ // use std::num::NonZeroU32; -use data_encoding::HEXLOWER; +use data_encoding::{Encoding, HEXLOWER}; use ring::{digest, hmac, pbkdf2}; static DIGEST_ALG: pbkdf2::Algorithm = pbkdf2::PBKDF2_HMAC_SHA256; @@ -38,17 +38,24 @@ pub fn hmac_sign(key: &str, data: &str) -> String { // pub fn get_random_64() -> Vec { - get_random(vec![0u8; 64]) + get_random_bytes::<64>().to_vec() } -pub fn get_random(mut array: Vec) -> Vec { +/// Return an array holding `N` random bytes. +pub fn get_random_bytes() -> [u8; N] { use ring::rand::{SecureRandom, SystemRandom}; + let mut array = [0; N]; SystemRandom::new().fill(&mut array).expect("Error generating random values"); array } +/// Encode random bytes using the provided function. +pub fn encode_random_bytes(e: Encoding) -> String { + e.encode(&get_random_bytes::()) +} + /// Generates a random string over a specified alphabet. pub fn get_random_string(alphabet: &[u8], num_chars: usize) -> String { // Ref: https://rust-lang-nursery.github.io/rust-cookbook/algorithms/randomness.html @@ -77,18 +84,18 @@ pub fn get_random_string_alphanum(num_chars: usize) -> String { get_random_string(ALPHABET, num_chars) } -pub fn generate_id(num_bytes: usize) -> String { - HEXLOWER.encode(&get_random(vec![0; num_bytes])) +pub fn generate_id() -> String { + encode_random_bytes::(HEXLOWER) } pub fn generate_send_id() -> String { // Send IDs are globally scoped, so make them longer to avoid collisions. - generate_id(32) // 256 bits + generate_id::<32>() // 256 bits } pub fn generate_attachment_id() -> String { // Attachment IDs are scoped to a cipher, so they can be smaller. - generate_id(10) // 80 bits + generate_id::<10>() // 80 bits } /// Generates a numeric token for email-based verifications. diff --git a/src/db/models/device.rs b/src/db/models/device.rs index e8a933cb..e47ccadc 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -48,7 +48,7 @@ impl Device { use crate::crypto; use data_encoding::BASE64; - let twofactor_remember = BASE64.encode(&crypto::get_random(vec![0u8; 180])); + let twofactor_remember = crypto::encode_random_bytes::<180>(BASE64); self.twofactor_remember = Some(twofactor_remember.clone()); twofactor_remember @@ -69,7 +69,7 @@ impl Device { use crate::crypto; use data_encoding::BASE64URL; - self.refresh_token = BASE64URL.encode(&crypto::get_random_64()); + self.refresh_token = crypto::encode_random_bytes::<64>(BASE64URL); } // Update the expiration of the device and the last update date From 7445ee40f8930c2f72e87d5809d2ff584b0c4d30 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sun, 13 Nov 2022 10:03:04 +0100 Subject: [PATCH 2/2] Remove get_random_64() Its uses are replaced by get_randm_bytes() or encode_random_bytes(). --- src/config.rs | 3 +-- src/crypto.rs | 4 ---- src/db/models/send.rs | 2 +- src/db/models/user.rs | 2 +- 4 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/config.rs b/src/config.rs index 9c31d231..4f2779d9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -981,8 +981,7 @@ impl Config { if let Some(akey) = self._duo_akey() { akey } else { - let akey = crate::crypto::get_random_64(); - let akey_s = data_encoding::BASE64.encode(&akey); + let akey_s = crate::crypto::encode_random_bytes::<64>(data_encoding::BASE64); // Save the new value let builder = ConfigBuilder { diff --git a/src/crypto.rs b/src/crypto.rs index daf5124d..99f0fb91 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -37,10 +37,6 @@ pub fn hmac_sign(key: &str, data: &str) -> String { // Random values // -pub fn get_random_64() -> Vec { - get_random_bytes::<64>().to_vec() -} - /// Return an array holding `N` random bytes. pub fn get_random_bytes() -> [u8; N] { use ring::rand::{SecureRandom, SystemRandom}; diff --git a/src/db/models/send.rs b/src/db/models/send.rs index effc5dfc..49756125 100644 --- a/src/db/models/send.rs +++ b/src/db/models/send.rs @@ -81,7 +81,7 @@ impl Send { if let Some(password) = password { self.password_iter = Some(PASSWORD_ITER); - let salt = crate::crypto::get_random_64(); + let salt = crate::crypto::get_random_bytes::<64>().to_vec(); let hash = crate::crypto::hash_password(password.as_bytes(), &salt, PASSWORD_ITER as u32); self.password_salt = Some(salt); self.password_hash = Some(hash); diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 826e00fa..68fb96f6 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -93,7 +93,7 @@ impl User { email_new_token: None, password_hash: Vec::new(), - salt: crypto::get_random_64(), + salt: crypto::get_random_bytes::<64>().to_vec(), password_iterations: CONFIG.password_iterations(), security_stamp: crate::util::get_uuid(),