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 7050f248..849ed7a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -941,6 +941,7 @@ dependencies = [ "schemars", "sea-query", "sea-query-binder", + "secure-string", "serde", "serde_json", "serde_urlencoded", @@ -3667,6 +3668,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 = "3.7.0" diff --git a/Cargo.toml b/Cargo.toml index 7d3193af..48de91eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -111,6 +111,7 @@ mime_guess = { version = "2", default-features = false } mockall = "0.14" 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 4900c27b..6dfa8ec0 100644 --- a/cot/Cargo.toml +++ b/cot/Cargo.toml @@ -47,6 +47,7 @@ mime.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 redis = { workspace = true, features = ["aio", "tokio-comp"], optional = true } schemars = { workspace = true, optional = true, features = ["derive"] } diff --git a/cot/src/common_types.rs b/cot/src/common_types.rs index a120afe0..8c3a4383 100644 --- a/cot/src/common_types.rs +++ b/cot/src/common_types.rs @@ -16,6 +16,9 @@ use cot::db::impl_postgres::PostgresValueRef; use cot::db::impl_sqlite::SqliteValueRef; use cot::form::FormFieldValidationError; use email_address::EmailAddress; +#[cfg(not(miri))] +use secure_string::SecureString; +use subtle::ConstantTimeEq; use thiserror::Error; #[cfg(feature = "db")] @@ -49,12 +52,14 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// 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 /// @@ -62,17 +67,35 @@ const MAX_EMAIL_LENGTH: u32 = 254; /// use cot::common_types::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)] +#[cfg(not(miri))] +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.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. /// @@ -85,7 +108,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. @@ -100,7 +123,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. @@ -114,6 +137,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 } @@ -137,6 +181,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 URL wrapper. /// /// This structure ensures that the contained URL is correctly formatted and @@ -737,7 +788,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 218245e9..bab6795a 100644 --- a/cot/src/config.rs +++ b/cot/src/config.rs @@ -22,6 +22,8 @@ use chrono::{DateTime, FixedOffset, Utc}; use cot_core::error::impl_into_cot_error; 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; use thiserror::Error; @@ -2081,11 +2083,12 @@ impl Default for EmailConfig { /// /// # 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 /// @@ -2096,10 +2099,30 @@ impl Default for EmailConfig { /// assert_eq!(key.as_bytes(), &[1, 2, 3]); /// ``` #[repr(transparent)] -#[derive(Clone, Serialize, Deserialize)] +#[derive(Clone, Deserialize)] #[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. /// @@ -2112,6 +2135,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)) } @@ -2127,6 +2158,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 } @@ -2142,6 +2181,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 } @@ -2165,12 +2213,6 @@ impl From<&str> for SecretKey { } } -impl PartialEq for SecretKey { - fn eq(&self, other: &Self) -> bool { - self.0.ct_eq(&other.0).into() - } -} - impl Eq for SecretKey {} impl Debug for SecretKey { @@ -2180,6 +2222,12 @@ impl Debug for SecretKey { } } +impl PartialEq for SecretKey { + fn eq(&self, other: &Self) -> bool { + self.as_bytes().ct_eq(other.as_bytes()).into() + } +} + impl Default for SecretKey { fn default() -> Self { Self::new(&[]) @@ -2460,6 +2508,7 @@ impl From<&str> for EmailUrl { #[cfg(test)] mod tests { + use serde_json; use time::OffsetDateTime; use super::*; @@ -2729,6 +2778,14 @@ 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]"); + } #[test] #[cfg(feature = "redis")] fn cache_type_from_str_redis() { diff --git a/deny.toml b/deny.toml index 44e84bbb..d4bfe0cc 100644 --- a/deny.toml +++ b/deny.toml @@ -25,4 +25,5 @@ allow = [ "MIT", "Unicode-3.0", "Zlib", + "Unlicense", ]