diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 986d7ff..eda3c5b 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,8 @@ **Vulnerability:** The `maybe_write_text` utility function was using `std::fs::write`, which resulted in sensitive data (like PSBT files and offers) being saved with insecure default file permissions, making them readable by other users on a shared system. **Learning:** Even generic utility functions used for saving user-requested command outputs must use secure file permissions (`0o600`) if the data they handle (like PSBTs and offers) is sensitive. **Prevention:** Always use `crate::paths::write_secure_file` instead of `std::fs::write` for all file writing operations that might contain sensitive material in this codebase. + +## 2024-04-16 - Prevent Path Traversal in Profile and Snapshot Processing +**Vulnerability:** The application was using unvalidated user input (`config.profile` and `name` in snapshot commands) to construct file paths for reading, writing, and creating profile and snapshot files (e.g. `snapshots/{name}.json`), resulting in a critical Path Traversal vulnerability. +**Learning:** File paths concatenated using standard path joining methods (e.g. `Path::join` and `format!`) inherently trust the supplied inputs. Without explicit validation of the filename characters, relative path components like `../` will resolve and potentially bypass intended directory sandbox constraints, exposing critical state, leading to data exposure or arbitrary file overwrites. +**Prevention:** Implement and enforce a strict path validation mechanism (e.g., `validate_file_name`) that asserts the input exclusively contains safe characters (like alphanumeric, underscores, and dashes) and rejects attempts using directories separators or empty inputs before passing the input to any file system operations. diff --git a/src/commands/snapshot.rs b/src/commands/snapshot.rs index 7be1d9e..d4b1143 100644 --- a/src/commands/snapshot.rs +++ b/src/commands/snapshot.rs @@ -12,6 +12,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result { + crate::utils::validate_file_name(name)?; let source = read_profile(&profile_path)?; let destination = snap_dir.join(format!("{name}.json")); if destination.exists() && !(*overwrite || cli.yes) { @@ -27,6 +28,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result { + crate::utils::validate_file_name(name)?; if !confirm(&format!("Are you sure you want to restore snapshot '{name}'? This will overwrite your current profile."), cli) { return Err(AppError::Internal("aborted by user".to_string())); } diff --git a/src/paths.rs b/src/paths.rs index e9be32c..8fa55e4 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -56,6 +56,7 @@ pub fn home_dir() -> PathBuf { } pub fn profile_path(config: &crate::config::ServiceConfig<'_>) -> Result { + crate::utils::validate_file_name(config.profile)?; let root = data_dir(config); let profiles = root.join("profiles"); if !profiles.exists() { @@ -70,6 +71,7 @@ pub fn profile_lock_path(config: &crate::config::ServiceConfig<'_>) -> Result) -> Result { + crate::utils::validate_file_name(config.profile)?; let root = data_dir(config); let directory = root.join("snapshots").join(config.profile); create_secure_dir_all(&directory) diff --git a/src/utils.rs b/src/utils.rs index 8e3996f..cdff525 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -185,6 +185,22 @@ pub fn resolve_psbt_source( )) } +pub fn validate_file_name(name: &str) -> Result<(), AppError> { + if name.is_empty() { + return Err(AppError::Invalid("filename cannot be empty".to_string())); + } + if !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return Err(AppError::Invalid( + "filename can only contain alphanumeric characters, underscores, and dashes" + .to_string(), + )); + } + Ok(()) +} + pub fn parse_indices(s: Option<&str>) -> Result, AppError> { let s = match s { Some(s) => s,