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/Cargo.lock b/Cargo.lock index 53ea9b7d..ab58863c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -618,6 +618,7 @@ dependencies = [ "schemars", "sea-query", "sea-query-binder", + "secure-string", "serde", "serde_html_form", "serde_json", @@ -2573,6 +2574,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 837e5a49..113fce3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,6 +96,7 @@ mime_guess = { version = "2", default-features = false } mockall = "0.13" multer = "3" 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 f0bccae6..2bd6fe63 100644 --- a/cot/Cargo.toml +++ b/cot/Cargo.toml @@ -43,6 +43,7 @@ indexmap.workspace = true mime_guess.workspace = true multer.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..be60fb10 100644 --- a/cot/src/common_types.rs +++ b/cot/src/common_types.rs @@ -15,6 +15,9 @@ 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; +use subtle::ConstantTimeEq; #[cfg(feature = "db")] use crate::db::{ColumnType, DatabaseField, DbValue, FromDbValue, SqlxValueRef, ToDbValue}; @@ -45,12 +48,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 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 +/// 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 /// @@ -58,17 +63,35 @@ 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); - -impl Debug for Password { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("Password").field(&"**********").finish() +#[derive(Clone, Debug)] +#[cfg(not(miri))] +pub struct Password(SecureString); + +impl PartialEq for Password { + fn eq(&self, other: &Self) -> bool { + self.as_str() + .as_bytes() + .ct_eq(other.as_str().as_bytes()) + .into() } } +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. /// @@ -81,7 +104,7 @@ impl Password { /// ``` #[must_use] pub fn new>(password: T) -> Self { - Self(password.into()) + Self(SecureString::from(password.into())) } /// Returns the password as a string. @@ -96,7 +119,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. @@ -110,6 +133,27 @@ impl Password { /// assert_eq!(password.into_string(), "password"); /// ``` #[must_use] + pub fn into_string(self) -> String { + self.0.into_unsecure() + } +} + +#[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 } @@ -133,6 +177,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 @@ -422,7 +473,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] diff --git a/cot/src/config.rs b/cot/src/config.rs index 37024064..6a6bf726 100644 --- a/cot/src/config.rs +++ b/cot/src/config.rs @@ -19,6 +19,8 @@ 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; @@ -819,11 +821,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 uses +/// constant-time comparison 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,10 +837,30 @@ impl SessionMiddlewareConfigBuilder { /// assert_eq!(key.as_bytes(), &[1, 2, 3]); /// ``` #[repr(transparent)] -#[derive(Clone, Serialize, Deserialize)] +#[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 + S: serde::Serializer, + { + serializer.serialize_bytes(self.as_bytes()) + } +} + impl SecretKey { /// Create a new [`SecretKey`] from a byte array. /// @@ -850,6 +873,14 @@ 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)) } @@ -865,6 +896,14 @@ 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 } @@ -880,6 +919,15 @@ 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 } @@ -905,19 +953,12 @@ impl From<&str> for SecretKey { impl PartialEq for SecretKey { fn eq(&self, other: &Self) -> bool { - self.0.ct_eq(&other.0).into() + self.as_bytes().ct_eq(other.as_bytes()).into() } } 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(&[]) @@ -1004,6 +1045,8 @@ impl Debug for DatabaseUrl { #[cfg(test)] mod tests { + use serde_json; + use super::*; #[test] @@ -1078,4 +1121,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]"); + } } 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", ]