diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 986d7ff..1bc5a60 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,7 @@ **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-05-18 - Path Traversal via PathBuf::join with Unvalidated Input +**Vulnerability:** Constructing file paths dynamically using `PathBuf::join(format!("{name}.json"))` with unvalidated user input (`name` from CLI arguments) allows an attacker to perform path traversal using `../` (e.g., `--name ../../../etc/passwd`). Furthermore, standard `fs::create_dir_all` might fall back to insecure default permissions depending on the caller. +**Learning:** The `join` method on `Path` or `PathBuf` does not validate or sanitize components. If an attacker controls the input and provides absolute paths or traversal segments (`../`), it resolves outside the intended directory. +**Prevention:** Always validate user-provided file names before appending them to paths. Use strict allowlists (`[a-zA-Z0-9_-]+`) for custom names. Remove redundant and standard directory creation functions if secure wrappers (e.g., `create_secure_dir_all`) are already implemented and called upstream. diff --git a/src/commands/snapshot.rs b/src/commands/snapshot.rs index 7be1d9e..7c4f462 100644 --- a/src/commands/snapshot.rs +++ b/src/commands/snapshot.rs @@ -7,11 +7,12 @@ use std::fs; pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result { let profile_path = profile_path(cli)?; let snap_dir = snapshot_dir(cli)?; - fs::create_dir_all(&snap_dir) + crate::paths::create_secure_dir_all(&snap_dir) .map_err(|e| AppError::Config(format!("failed to create snapshot dir: {e}")))?; match &args.action { SnapshotAction::Save { name, overwrite } => { + 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/utils.rs b/src/utils.rs index 8e3996f..b139078 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -185,6 +185,21 @@ pub fn resolve_psbt_source( )) } +pub fn validate_file_name(name: &str) -> Result<(), AppError> { + if name.is_empty() { + return Err(AppError::Invalid("file name cannot be empty".to_string())); + } + for c in name.chars() { + if !c.is_ascii_alphanumeric() && c != '-' && c != '_' { + return Err(AppError::Invalid(format!( + "invalid character '{}' in file name (only alphanumeric, '-', and '_' are allowed)", + c + ))); + } + } + Ok(()) +} + pub fn parse_indices(s: Option<&str>) -> Result, AppError> { let s = match s { Some(s) => s,