From 2cdfd65acc4b92e5b74b9a9eaf559a9afc63873b Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Thu, 15 May 2025 21:42:40 +0300 Subject: [PATCH 1/6] feat: add secure-string dependency and integrate into Password struct - Updated struct to use SecureString for enhanced security. - Implemented PartialEq and Eq to be able to compare 2 Password instances - Added tests for equality comparison of instances. --- Cargo.lock | 11 +++++++++ Cargo.toml | 1 + cot/Cargo.toml | 1 + cot/src/common_types.rs | 54 +++++++++++++++++++++++++++++------------ 4 files changed, 51 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ebce5601..1fdecb92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -617,6 +617,7 @@ dependencies = [ "schemars", "sea-query", "sea-query-binder", + "secure-string", "serde", "serde_html_form", "serde_json", @@ -2537,6 +2538,16 @@ dependencies = [ "sqlx", ] +[[package]] +name = "secure-string" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "548ba8c9ff631f7bb3a64de1e8ad73fe20f6d04090724f2b496ed45314ad7488" +dependencies = [ + "libc", + "zeroize", +] + [[package]] name = "security-framework" version = "2.11.1" diff --git a/Cargo.toml b/Cargo.toml index bb0b081e..53f8311b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,6 +94,7 @@ insta-cmd = "0.6" mime_guess = { version = "2", default-features = false } mockall = "0.13" password-auth = { version = "1", default-features = false } +secure-string = "0.3.0" petgraph = { version = "0.8", default-features = false } pin-project-lite = "0.2" prettyplease = "0.2" diff --git a/cot/Cargo.toml b/cot/Cargo.toml index 26004189..20b30d1f 100644 --- a/cot/Cargo.toml +++ b/cot/Cargo.toml @@ -42,6 +42,7 @@ humantime.workspace = true indexmap.workspace = true mime_guess.workspace = true password-auth = { workspace = true, features = ["std", "argon2"] } +secure-string.workspace = true pin-project-lite.workspace = true schemars = { workspace = true, optional = true } sea-query = { workspace = true, optional = true } diff --git a/cot/src/common_types.rs b/cot/src/common_types.rs index 6de40eb6..128bb3ea 100644 --- a/cot/src/common_types.rs +++ b/cot/src/common_types.rs @@ -5,6 +5,7 @@ //! general-purpose newtype wrappers and associated trait implementations to //! ensure consistent and safe processing of form data. +use secure_string::SecureString; use std::fmt::Debug; use std::str::FromStr; @@ -45,12 +46,13 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// compare it with the other password. This method uses constant-time /// equality comparison, which protects against timing attacks. /// -/// 2. An alternative is to use the [`Password::as_str`] method and compare the -/// strings directly. This approach uses non-constant-time comparison, which -/// is less secure but may be acceptable in certain legitimate use cases -/// where the security tradeoff is understood, e.g., when you're creating a -/// user registration form with the "retype your password" field, where both -/// passwords come from the same source anyway. +/// 2. An alternative is to compare 2 instances of the [`Password`] type directly +/// because this password struct implements the [`PartialEq`] trait. You can also +/// use the [`Password::as_str`] method to compare the strings directly. This +/// approach uses non-constant-time comparison, which is less secure but may be +/// acceptable in certain legitimate use cases where the security tradeoff is +/// understood, e.g., when you're creating a user registration form with the +/// "retype your password" field, where both passwords come from the same source anyway. /// /// # Examples /// @@ -58,17 +60,23 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// use cot::auth::Password; /// /// let password = Password::new("pass"); -/// assert_eq!(&format!("{:?}", password), "Password(\"**********\")"); +/// assert_eq!(&format!("{:?}", password), "Password(***SECRET***)"); +/// +/// let another_password = Password::new("pass"); +/// assert_eq!(password, another_password); +/// /// ``` -#[derive(Clone)] -pub struct Password(String); +#[derive(Clone, Debug)] +pub struct Password(SecureString); -impl Debug for Password { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("Password").field(&"**********").finish() +impl PartialEq for Password { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 } } +impl Eq for Password {} + impl Password { /// Creates a new password object. /// @@ -80,7 +88,7 @@ impl Password { /// let password = Password::new("password"); /// ``` #[must_use] - pub fn new>(password: T) -> Self { + pub fn new>(password: T) -> Self { Self(password.into()) } @@ -96,7 +104,7 @@ impl Password { /// ``` #[must_use] pub fn as_str(&self) -> &str { - &self.0 + self.0.unsecure() } /// Consumes the object and returns the password as a string. @@ -111,7 +119,7 @@ impl Password { /// ``` #[must_use] pub fn into_string(self) -> String { - self.0 + self.0.into_unsecure() } } @@ -422,7 +430,21 @@ mod tests { #[test] fn password_debug() { let password = Password::new("password"); - assert_eq!(format!("{password:?}"), "Password(\"**********\")"); + assert_eq!(format!("{password:?}"), "Password(***SECRET***)"); + } + + #[test] + fn password_eq() { + let password1 = Password::new("password"); + let password2 = Password::new("password"); + assert_eq!(password1, password2); + } + + #[test] + fn password_ne() { + let password1 = Password::new("password"); + let password2 = Password::new("password2"); + assert_ne!(password1, password2); } #[test] From 8ee2f46afcbfde54e190e828f46cb59d817184e6 Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Thu, 15 May 2025 22:54:51 +0300 Subject: [PATCH 2/6] feat: replace Box<[u8]> with SecureBytes in SecretKey for enhanced security - Updated SecretKey struct to use SecureBytes for improved security handling. - Adjusted implementations of PartialEq and Debug to align with SecureBytes. - Implemented serialization for SecretKey since we cannot derive Serialize since SecureBytes doesn't implement it . - Included tests for JSON serialization of SecretKey. --- cot/src/config.rs | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/cot/src/config.rs b/cot/src/config.rs index 37024064..22880af5 100644 --- a/cot/src/config.rs +++ b/cot/src/config.rs @@ -19,8 +19,8 @@ use std::time::Duration; use derive_builder::Builder; use derive_more::with_trait::{Debug, From}; +use secure_string::SecureBytes; use serde::{Deserialize, Serialize}; -use subtle::ConstantTimeEq; /// The configuration for a project. /// @@ -819,11 +819,12 @@ impl SessionMiddlewareConfigBuilder { /// /// # Security /// -/// The implementation of the [`PartialEq`] trait for this type is constant-time -/// to prevent timing attacks. +/// The implementation of the [`PartialEq`] trait for this type is inherited from +/// [`SecureBytes`], which is constant-time to prevent timing attacks. /// -/// The implementation of the [`Debug`] trait for this type hides the secret key -/// to prevent it from being leaked in logs or other debug output. +/// The implementation of the [`Debug`] trait for this type is inherited from +/// [`SecureBytes`], which hides the secret key to prevent it from being leaked +/// in logs or other debug output. /// /// # Examples /// @@ -834,9 +835,18 @@ impl SessionMiddlewareConfigBuilder { /// assert_eq!(key.as_bytes(), &[1, 2, 3]); /// ``` #[repr(transparent)] -#[derive(Clone, Serialize, Deserialize)] +#[derive(Clone, Deserialize, Debug)] #[serde(from = "String")] -pub struct SecretKey(Box<[u8]>); +pub struct SecretKey(SecureBytes); + +impl Serialize for SecretKey { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_bytes(self.as_bytes()) + } +} impl SecretKey { /// Create a new [`SecretKey`] from a byte array. @@ -851,7 +861,7 @@ impl SecretKey { /// ``` #[must_use] pub fn new(hash: &[u8]) -> Self { - Self(Box::from(hash)) + Self(SecureBytes::new(hash.to_vec())) } /// Get the byte array stored in the [`SecretKey`]. @@ -866,7 +876,7 @@ impl SecretKey { /// ``` #[must_use] pub fn as_bytes(&self) -> &[u8] { - &self.0 + self.0.unsecure() } /// Consume the [`SecretKey`] and return the byte array stored in it. @@ -881,7 +891,7 @@ impl SecretKey { /// ``` #[must_use] pub fn into_bytes(self) -> Box<[u8]> { - self.0 + self.0.unsecure().to_vec().into_boxed_slice() } } @@ -905,19 +915,12 @@ impl From<&str> for SecretKey { impl PartialEq for SecretKey { fn eq(&self, other: &Self) -> bool { - self.0.ct_eq(&other.0).into() + self.0 == other.0 } } impl Eq for SecretKey {} -impl Debug for SecretKey { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - // write in single line, regardless whether alternate mode was used or not - write!(f, "SecretKey(\"**********\")") - } -} - impl Default for SecretKey { fn default() -> Self { Self::new(&[]) @@ -1005,6 +1008,7 @@ impl Debug for DatabaseUrl { #[cfg(test)] mod tests { use super::*; + use serde_json; #[test] fn from_toml_valid() { @@ -1078,4 +1082,12 @@ mod tests { StaticFilesPathRewriteMode::QueryParam ); } + + #[test] + fn secret_key_serialize_json() { + let key = SecretKey::from("abc123"); + let serialized = serde_json::to_string(&key).unwrap(); + // Should serialize as a byte array + assert_eq!(serialized, "[97,98,99,49,50,51]"); + } } From c7dc276039e4435888bea5a1390f72c78d1226a9 Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Tue, 27 May 2025 22:59:22 +0300 Subject: [PATCH 3/6] chore: Refactor Password and SecretKey structs for Miri compatibility - Refactored Password and SecretKey structs to provide Miri-compatible versions, using String and Box<[u8]> respectively. - Adjusted methods for creating and accessing passwords and secret keys to ensure compatibility with Miri's limitations. --- .gitignore | 3 ++ cot/src/common_types.rs | 61 +++++++++++++++++++++++++++++++++-------- cot/src/config.rs | 47 ++++++++++++++++++++++++++++--- 3 files changed, 96 insertions(+), 15 deletions(-) diff --git a/.gitignore b/.gitignore index a611abcd..a07843ed 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,6 @@ target/ *.db *.sqlite3 *.sqlite3-journal + +# VSCode +.vscode/ diff --git a/cot/src/common_types.rs b/cot/src/common_types.rs index 128bb3ea..dec1ef5f 100644 --- a/cot/src/common_types.rs +++ b/cot/src/common_types.rs @@ -5,7 +5,6 @@ //! general-purpose newtype wrappers and associated trait implementations to //! ensure consistent and safe processing of form data. -use secure_string::SecureString; use std::fmt::Debug; use std::str::FromStr; @@ -16,6 +15,8 @@ use cot::db::impl_postgres::PostgresValueRef; #[cfg(feature = "sqlite")] use cot::db::impl_sqlite::SqliteValueRef; use email_address::EmailAddress; +#[cfg(not(miri))] +use secure_string::SecureString; #[cfg(feature = "db")] use crate::db::{ColumnType, DatabaseField, DbValue, FromDbValue, SqlxValueRef, ToDbValue}; @@ -46,13 +47,14 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// compare it with the other password. This method uses constant-time /// equality comparison, which protects against timing attacks. /// -/// 2. An alternative is to compare 2 instances of the [`Password`] type directly -/// because this password struct implements the [`PartialEq`] trait. You can also -/// use the [`Password::as_str`] method to compare the strings directly. This -/// approach uses non-constant-time comparison, which is less secure but may be -/// acceptable in certain legitimate use cases where the security tradeoff is -/// understood, e.g., when you're creating a user registration form with the -/// "retype your password" field, where both passwords come from the same source anyway. +/// 2. An alternative is to compare 2 instances of the [`Password`] type +/// directly because this password struct implements the [`PartialEq`] trait. +/// You can also use the [`Password::as_str`] method to compare the strings +/// directly. This approach uses non-constant-time comparison, which is less +/// secure but may be acceptable in certain legitimate use cases where the +/// security tradeoff is understood, e.g., when you're creating a user +/// registration form with the "retype your password" field, where both +/// passwords come from the same source anyway. /// /// # Examples /// @@ -64,9 +66,9 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// /// let another_password = Password::new("pass"); /// assert_eq!(password, another_password); -/// /// ``` #[derive(Clone, Debug)] +#[cfg(not(miri))] pub struct Password(SecureString); impl PartialEq for Password { @@ -77,6 +79,15 @@ impl PartialEq for Password { impl Eq for Password {} +/// A password - simplified version for Miri testing. +/// +/// When running under Miri, we use a simple String wrapper instead of +/// SecureString to avoid the mlock system call that Miri doesn't support. +#[derive(Clone)] +#[cfg(miri)] +pub struct Password(String); + +#[cfg(not(miri))] impl Password { /// Creates a new password object. /// @@ -88,8 +99,8 @@ impl Password { /// let password = Password::new("password"); /// ``` #[must_use] - pub fn new>(password: T) -> Self { - Self(password.into()) + pub fn new>(password: T) -> Self { + Self(SecureString::from(password.into())) } /// Returns the password as a string. @@ -123,6 +134,27 @@ impl Password { } } +#[cfg(miri)] +impl Password { + /// Creates a new password object - Miri version. + #[must_use] + pub fn new>(password: T) -> Self { + Self(password.into()) + } + + /// Returns the password as a string - Miri version. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Consumes the object and returns the password as a string - Miri version. + #[must_use] + pub fn into_string(self) -> String { + self.0 + } +} + impl From<&Password> for Password { fn from(password: &Password) -> Self { password.clone() @@ -141,6 +173,13 @@ impl From for Password { } } +#[cfg(miri)] +impl Debug for Password { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Password(***SECRET***)") + } +} + /// A validated email address. /// /// This is a newtype wrapper around diff --git a/cot/src/config.rs b/cot/src/config.rs index 22880af5..9cb4c433 100644 --- a/cot/src/config.rs +++ b/cot/src/config.rs @@ -19,8 +19,10 @@ use std::time::Duration; use derive_builder::Builder; use derive_more::with_trait::{Debug, From}; +#[cfg(not(miri))] use secure_string::SecureBytes; use serde::{Deserialize, Serialize}; +use subtle::ConstantTimeEq; /// The configuration for a project. /// @@ -819,8 +821,8 @@ impl SessionMiddlewareConfigBuilder { /// /// # Security /// -/// The implementation of the [`PartialEq`] trait for this type is inherited from -/// [`SecureBytes`], which is constant-time to prevent timing attacks. +/// The implementation of the [`PartialEq`] trait for this type is inherited +/// from [`SecureBytes`], which is constant-time to prevent timing attacks. /// /// The implementation of the [`Debug`] trait for this type is inherited from /// [`SecureBytes`], which hides the secret key to prevent it from being leaked @@ -837,8 +839,19 @@ impl SessionMiddlewareConfigBuilder { #[repr(transparent)] #[derive(Clone, Deserialize, Debug)] #[serde(from = "String")] +#[cfg(not(miri))] pub struct SecretKey(SecureBytes); +/// A secret key - simplified version for Miri testing. +/// +/// When running under Miri, we use a simple Box<[u8]> wrapper instead of +/// SecureBytes to avoid the mlock system call that Miri doesn't support. +#[repr(transparent)] +#[derive(Clone, Deserialize, Debug)] +#[serde(from = "String")] +#[cfg(miri)] +pub struct SecretKey(Box<[u8]>); + impl Serialize for SecretKey { fn serialize(&self, serializer: S) -> Result where @@ -860,10 +873,18 @@ impl SecretKey { /// assert_eq!(key.as_bytes(), &[1, 2, 3]); /// ``` #[must_use] + #[cfg(not(miri))] pub fn new(hash: &[u8]) -> Self { Self(SecureBytes::new(hash.to_vec())) } + /// Create a new [`SecretKey`] from a byte array - Miri version. + #[must_use] + #[cfg(miri)] + pub fn new(hash: &[u8]) -> Self { + Self(Box::from(hash)) + } + /// Get the byte array stored in the [`SecretKey`]. /// /// # Examples @@ -875,10 +896,18 @@ impl SecretKey { /// assert_eq!(key.as_bytes(), &[1, 2, 3]); /// ``` #[must_use] + #[cfg(not(miri))] pub fn as_bytes(&self) -> &[u8] { self.0.unsecure() } + /// Get the byte array stored in the [`SecretKey`] - Miri version. + #[must_use] + #[cfg(miri)] + pub fn as_bytes(&self) -> &[u8] { + &self.0 + } + /// Consume the [`SecretKey`] and return the byte array stored in it. /// /// # Examples @@ -890,9 +919,18 @@ impl SecretKey { /// assert_eq!(key.into_bytes(), Box::from([1, 2, 3])); /// ``` #[must_use] + #[cfg(not(miri))] pub fn into_bytes(self) -> Box<[u8]> { self.0.unsecure().to_vec().into_boxed_slice() } + + /// Consume the [`SecretKey`] and return the byte array stored in it - Miri + /// version. + #[must_use] + #[cfg(miri)] + pub fn into_bytes(self) -> Box<[u8]> { + self.0 + } } impl From<&[u8]> for SecretKey { @@ -915,7 +953,7 @@ impl From<&str> for SecretKey { impl PartialEq for SecretKey { fn eq(&self, other: &Self) -> bool { - self.0 == other.0 + self.as_bytes().ct_eq(other.as_bytes()).into() } } @@ -1007,9 +1045,10 @@ impl Debug for DatabaseUrl { #[cfg(test)] mod tests { - use super::*; use serde_json; + use super::*; + #[test] fn from_toml_valid() { let toml_content = r#" From e255fd17a6bc642421c8d479035d7c66cf3aba4c Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Tue, 27 May 2025 23:06:53 +0300 Subject: [PATCH 4/6] Fix inconsistency in docs about the implementation of PartialEq --- cot/src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cot/src/config.rs b/cot/src/config.rs index 9cb4c433..6a6bf726 100644 --- a/cot/src/config.rs +++ b/cot/src/config.rs @@ -821,8 +821,8 @@ impl SessionMiddlewareConfigBuilder { /// /// # Security /// -/// The implementation of the [`PartialEq`] trait for this type is inherited -/// from [`SecureBytes`], which is constant-time to prevent timing attacks. +/// The implementation of the [`PartialEq`] trait for this type uses +/// constant-time comparison to prevent timing attacks. /// /// The implementation of the [`Debug`] trait for this type is inherited from /// [`SecureBytes`], which hides the secret key to prevent it from being leaked From c2d06c0440acbecf43fc2b9e124171783c90323c Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Sat, 31 May 2025 13:20:28 +0300 Subject: [PATCH 5/6] Add Unlicense to cargo-deny's allowed list of licenses --- deny.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/deny.toml b/deny.toml index 6c253ceb..33a87555 100644 --- a/deny.toml +++ b/deny.toml @@ -20,4 +20,5 @@ allow = [ "0BSD", "BSD-3-Clause", "Zlib", + "Unlicense", ] From 767a1f6dc8189fb47698156b9645a67c81c3d230 Mon Sep 17 00:00:00 2001 From: "Ibrahim E.Gad" Date: Sun, 1 Jun 2025 13:47:40 +0300 Subject: [PATCH 6/6] Use constant time comparison for Password comparison --- cot/src/common_types.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/cot/src/common_types.rs b/cot/src/common_types.rs index dec1ef5f..be60fb10 100644 --- a/cot/src/common_types.rs +++ b/cot/src/common_types.rs @@ -17,6 +17,7 @@ use cot::db::impl_sqlite::SqliteValueRef; use email_address::EmailAddress; #[cfg(not(miri))] use secure_string::SecureString; +use subtle::ConstantTimeEq; #[cfg(feature = "db")] use crate::db::{ColumnType, DatabaseField, DbValue, FromDbValue, SqlxValueRef, ToDbValue}; @@ -48,13 +49,13 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// equality comparison, which protects against timing attacks. /// /// 2. An alternative is to compare 2 instances of the [`Password`] type -/// directly because this password struct implements the [`PartialEq`] trait. -/// You can also use the [`Password::as_str`] method to compare the strings -/// directly. This approach uses non-constant-time comparison, which is less -/// secure but may be acceptable in certain legitimate use cases where the -/// security tradeoff is understood, e.g., when you're creating a user -/// registration form with the "retype your password" field, where both -/// passwords come from the same source anyway. +/// directly because this password struct implements the [`PartialEq`] trait +/// which also uses constant-time comparison. Comparing 2 instances of the +/// [`Password`] type is less secure than using [`PasswordHash::verify`], but +/// may be acceptable in certain legitimate use cases where the security +/// tradeoff is understood, e.g., when you're creating a user registration +/// form with the "retype your password" field, in this case this approach +/// might save on hashing costs. /// /// # Examples /// @@ -73,7 +74,10 @@ pub struct Password(SecureString); impl PartialEq for Password { fn eq(&self, other: &Self) -> bool { - self.0 == other.0 + self.as_str() + .as_bytes() + .ct_eq(other.as_str().as_bytes()) + .into() } }