diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 986d7ff..80cbf8e 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-04-10 - Missing Path Validation on Snapshot and Idempotency Features +**Vulnerability:** User-provided string parameters (`name` in snapshot save/restore, `profile` in context, and `idempotency_key`) were used directly in `Path::join()` to build file paths without character validation, allowing arbitrary filesystem read/write (Path Traversal via `../`). +**Learning:** Functions that accept strings used for file paths must validate the contents of the string against a strict allowlist. +**Prevention:** A `validate_file_name` function was added checking for `c.is_ascii_alphanumeric() || c == '_' || c == '-'`. Any user-provided identifier used as a filename component should be passed through this validation prior to use. 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/main.rs b/src/main.rs index 35d61bb..73c7401 100644 --- a/src/main.rs +++ b/src/main.rs @@ -19,6 +19,7 @@ mod wizard; use crate::cli::{Cli, Command, PolicyMode}; use crate::config::*; use crate::error::AppError; +use crate::utils::validate_file_name; use crate::utils::{env_bool, env_non_empty}; use clap::Parser; use is_terminal::IsTerminal; @@ -741,13 +742,12 @@ fn command_fingerprint(cli: &Cli) -> String { format!("{:016x}", hasher.finish()) } -fn idempotency_store_path(cli: &Cli) -> PathBuf { - crate::wallet_service::data_dir(&service_config(cli)) +fn idempotency_store_path(cli: &Cli) -> Result { + let profile = cli.profile.as_deref().unwrap_or("default"); + validate_file_name(profile)?; + Ok(crate::wallet_service::data_dir(&service_config(cli)) .join("idempotency") - .join(format!( - "{}.json", - cli.profile.as_deref().unwrap_or("default") - )) + .join(format!("{}.json", profile))) } fn load_idempotency_store(path: &Path) -> Result { @@ -791,7 +791,7 @@ fn try_replay_idempotent_result( return Ok(None); } - let path = idempotency_store_path(cli); + let path = idempotency_store_path(cli)?; let store = load_idempotency_store(&path)?; let Some(entry) = store.entries.get(key) else { return Ok(None); @@ -824,7 +824,8 @@ fn record_idempotent_result( Some(k) => k.to_string(), None => return Ok(now_unix_ms()), }; - let path = idempotency_store_path(cli); + validate_file_name(&key)?; + let path = idempotency_store_path(cli)?; let mut store = load_idempotency_store(&path)?; let fingerprint = command_fingerprint(cli); diff --git a/src/utils.rs b/src/utils.rs index 8e3996f..6a32d73 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -3,6 +3,20 @@ 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() + || !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return Err(AppError::Invalid(format!( + "invalid file name '{}': must contain only alphanumeric characters, underscores, and dashes", + name + ))); + } + Ok(()) +} + pub fn home_dir() -> PathBuf { if let Some(home) = std::env::var_os("HOME") { PathBuf::from(home)