diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 986d7ff..3c3b957 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-03 - Prevent Path Traversal in Snapshot Commands +**Vulnerability:** The snapshot command constructs file paths using `snap_dir.join(format!("{name}.json"))` where `name` is directly sourced from user input (CLI argument). If the user provides a path starting with `/` or `../`, `Path::join` resolves it absolutely or traverses up, allowing arbitrary file read/write across the system instead of isolating it within `snap_dir`. +**Learning:** In Rust, `Path::join` replaces the base path if the appended segment is absolute (e.g., `/tmp/malicious`). Unvalidated user inputs used in path construction can lead to severe path traversal vulnerabilities. +**Prevention:** Implement strict file name validation functions ensuring that user-provided names consist only of safe characters (e.g., alphanumeric, underscores, dashes) before constructing file paths. diff --git a/src/commands/snapshot.rs b/src/commands/snapshot.rs index 7be1d9e..00bad5d 100644 --- a/src/commands/snapshot.rs +++ b/src/commands/snapshot.rs @@ -1,6 +1,7 @@ use crate::cli::{Cli, SnapshotAction, SnapshotArgs}; use crate::error::AppError; use crate::output::CommandOutput; +use crate::utils::validate_file_name; use crate::{confirm, profile_path, read_profile, snapshot_dir, write_bytes_atomic}; use std::fs; @@ -12,6 +13,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result { + 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 +29,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result { + 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..a73fb90 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -3,6 +3,21 @@ use crate::error::AppError; use std::env; use std::path::{Path, PathBuf}; +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: '{}'", + c + ))); + } + } + Ok(()) +} + pub fn home_dir() -> PathBuf { if let Some(home) = std::env::var_os("HOME") { PathBuf::from(home) @@ -219,3 +234,37 @@ pub fn parse_indices(s: Option<&str>) -> Result, AppError> { } Ok(indices) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::AppError; + + #[test] + fn test_validate_file_name() { + assert!(validate_file_name("valid_name-123").is_ok()); + assert!(validate_file_name("SNAPSHOT_name").is_ok()); + + assert!(matches!(validate_file_name(""), Err(AppError::Invalid(_)))); + assert!(matches!( + validate_file_name("../passwd"), + Err(AppError::Invalid(_)) + )); + assert!(matches!( + validate_file_name("/etc/passwd"), + Err(AppError::Invalid(_)) + )); + assert!(matches!( + validate_file_name("foo.bar"), + Err(AppError::Invalid(_)) + )); + assert!(matches!( + validate_file_name("a/b"), + Err(AppError::Invalid(_)) + )); + assert!(matches!( + validate_file_name("invalid chars!"), + Err(AppError::Invalid(_)) + )); + } +}