🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Snapshot Command

.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-17 - Path Traversal in Snapshot Commands
+**Vulnerability:** The snapshot command allowed user-supplied snapshot names (via `args.name`) to be directly formatted into a path and joined with the snapshot directory (`snap_dir.join(format!("{name}.json"))`). This lack of validation creates a path traversal vulnerability where a user could provide a name like `../../../etc/passwd` (or similar) to read from or write to arbitrary locations on the file system, bypassing intended directory restrictions.
+**Learning:** Using user input directly in file paths is extremely dangerous, even if the application appends an extension like `.json`, as an attacker can still control the destination directory.
+**Prevention:** Always validate user-provided file names using strict allowlists (e.g., alphanumeric, underscores, dashes) via utilities like `crate::utils::validate_file_name` before using them in file path construction.

src/commands/snapshot.rs

@@ -12,6 +12,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppErr
 
     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<CommandOutput, AppErr
             })
         }
         SnapshotAction::Restore { name } => {
+            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()));
             }

src/utils.rs

@@ -91,6 +91,20 @@ pub(crate) fn best_match<'a>(needle: &str, candidates: &'a [&'a str]) -> Option<
     }
 }
 
+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: '{}' (only alphanumeric, underscores, and dashes are allowed)",
+            name
+        )));
+    }
+    Ok(())
+}
+
 pub fn maybe_write_text(path: Option<&str>, text: &str) -> Result<(), crate::error::AppError> {
     if let Some(path) = path {
         crate::paths::write_secure_file(path, text.as_bytes())

src/utils.rs.orig

@@ -0,0 +1,221 @@
+use crate::config::{NetworkArg, Profile, SchemeArg};
+use crate::error::AppError;
+use std::env;
+use std::path::{Path, PathBuf};
+
+pub fn home_dir() -> PathBuf {
+    if let Some(home) = std::env::var_os("HOME") {
+        PathBuf::from(home)
+    } else {
+        PathBuf::from(".")
+    }
+}
+
+pub fn env_non_empty(name: &str) -> Option<String> {
+    let value = env::var(name).ok()?;
+    let trimmed = value.trim();
+    if trimmed.is_empty() {
+        None
+    } else {
+        Some(trimmed.to_string())
+    }
+}
+
+pub fn env_bool(name: &str) -> Option<bool> {
+    let value = env_non_empty(name)?;
+    let normalized = value.to_ascii_lowercase();
+    match normalized.as_str() {
+        "1" | "true" | "yes" | "on" => Some(true),
+        "0" | "false" | "no" | "off" => Some(false),
+        _ => None,
+    }
+}
+
+pub fn parse_network(s: &str) -> Result<NetworkArg, AppError> {
+    match s.to_lowercase().as_str() {
+        "bitcoin" | "mainnet" => Ok(NetworkArg::Bitcoin),
+        "signet" => Ok(NetworkArg::Signet),
+        "testnet" => Ok(NetworkArg::Testnet),
+        "regtest" => Ok(NetworkArg::Regtest),
+        _ => Err(AppError::Invalid(format!("unknown network: {s}"))),
+    }
+}
+
+pub fn parse_scheme(s: &str) -> Result<SchemeArg, AppError> {
+    match s.to_lowercase().as_str() {
+        "unified" => Ok(SchemeArg::Unified),
+        "dual" => Ok(SchemeArg::Dual),
+        _ => Err(AppError::Invalid(format!("unknown scheme: {s}"))),
+    }
+}
+
+pub(crate) fn parse_bool_value(value: &str, context: &str) -> Result<bool, String> {
+    let normalized = value.trim().to_ascii_lowercase();
+    match normalized.as_str() {
+        "1" | "true" | "yes" | "on" => Ok(true),
+        "0" | "false" | "no" | "off" => Ok(false),
+        _ => Err(format!(
+            "{context} must be one of: true,false,yes,no,on,off,1,0"
+        )),
+    }
+}
+
+pub(crate) fn unknown_with_hint(kind: &str, unknown: &str, candidates: &[&str]) -> String {
+    if let Some(suggestion) = best_match(unknown, candidates) {
+        return format!("unknown {kind}: {unknown} (did you mean {suggestion}?)");
+    }
+    format!("unknown {kind}: {unknown}")
+}
+
+pub(crate) fn best_match<'a>(needle: &str, candidates: &'a [&'a str]) -> Option<&'a str> {
+    let mut best: Option<(&str, usize)> = None;
+    for &candidate in candidates {
+        let score = levenshtein(needle, candidate);
+        match best {
+            Some((_, best_score)) if score >= best_score => {}
+            _ => best = Some((candidate, score)),
+        }
+    }
+
+    let (candidate, score) = best?;
+    let threshold = match needle.len() {
+        0..=4 => 1,
+        5..=9 => 2,
+        _ => 3,
+    };
+
+    if score <= threshold {
+        Some(candidate)
+    } else {
+        None
+    }
+}
+
+pub fn maybe_write_text(path: Option<&str>, text: &str) -> Result<(), crate::error::AppError> {
+    if let Some(path) = path {
+        crate::paths::write_secure_file(path, text.as_bytes())
+            .map_err(|e| crate::error::AppError::Io(format!("failed to write to {path}: {e}")))
+    } else {
+        Ok(())
+    }
+}
+
+pub fn run_bitcoin_cli(
+    profile: &Profile,
+    args: &[String],
+) -> Result<String, crate::error::AppError> {
+    let mut cmd = std::process::Command::new(&profile.bitcoin_cli);
+    for arg in &profile.bitcoin_cli_args {
+        cmd.arg(arg);
+    }
+    cmd.args(args);
+    let output = cmd
+        .output()
+        .map_err(|e| crate::error::AppError::Internal(format!("bitcoin-cli failed: {e}")))?;
+    if !output.status.success() {
+        let err = String::from_utf8_lossy(&output.stderr);
+        return Err(crate::error::AppError::Internal(format!(
+            "bitcoin-cli error: {err}"
+        )));
+    }
+    Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
+}
+
+pub(crate) fn levenshtein(a: &str, b: &str) -> usize {
+    if a == b {
+        return 0;
+    }
+    if a.is_empty() {
+        return b.chars().count();
+    }
+    if b.is_empty() {
+        return a.chars().count();
+    }
+
+    let b_len = b.chars().count();
+    let mut prev: Vec<usize> = (0..=b_len).collect();
+    let mut curr = vec![0; b_len + 1];
+
+    for (i, ca) in a.chars().enumerate() {
+        curr[0] = i + 1;
+        for (j, cb) in b.chars().enumerate() {
+            let cost = usize::from(ca != cb);
+            curr[j + 1] = (curr[j] + 1).min(prev[j + 1] + 1).min(prev[j] + cost);
+        }
+        std::mem::swap(&mut prev, &mut curr);
+    }
+
+    prev[b_len]
+}
+pub fn resolve_psbt_source(
+    psbt: Option<&str>,
+    psbt_file: Option<&Path>,
+    psbt_stdin: bool,
+) -> Result<String, AppError> {
+    let count = (psbt.is_some() as u8) + (psbt_file.is_some() as u8) + (psbt_stdin as u8);
+    if count > 1 {
+        return Err(AppError::Invalid(
+            "accepts only one of --psbt, --psbt-file, --psbt-stdin".to_string(),
+        ));
+    }
+    if let Some(psbt) = psbt {
+        return Ok(psbt.to_string());
+    }
+    if let Some(path) = psbt_file {
+        return std::fs::read_to_string(path).map_err(|e| {
+            AppError::Io(format!("failed to read psbt file {}: {e}", path.display()))
+        });
+    }
+    if psbt_stdin {
+        use std::io::Read;
+        let mut buffer = String::new();
+        std::io::stdin()
+            .read_to_string(&mut buffer)
+            .map_err(|e| AppError::Io(format!("failed to read psbt from stdin: {e}")))?;
+        let trimmed = buffer.trim();
+        if trimmed.is_empty() {
+            return Err(AppError::Invalid(
+                "stdin did not contain a PSBT string".to_string(),
+            ));
+        }
+        return Ok(trimmed.to_string());
+    }
+    Err(AppError::Invalid(
+        "requires one of --psbt, --psbt-file, --psbt-stdin".to_string(),
+    ))
+}
+
+pub fn parse_indices(s: Option<&str>) -> Result<Vec<usize>, AppError> {
+    let s = match s {
+        Some(s) => s,
+        None => return Ok(Vec::new()),
+    };
+    let mut indices = Vec::new();
+    for part in s.split(',') {
+        let part = part.trim();
+        if part.is_empty() {
+            continue;
+        }
+        if part.contains('-') {
+            let bounds: Vec<&str> = part.split('-').collect();
+            if bounds.len() != 2 {
+                return Err(AppError::Invalid(format!("invalid index range: {part}")));
+            }
+            let start: usize = bounds[0]
+                .parse()
+                .map_err(|_| AppError::Invalid(format!("invalid start index: {}", bounds[0])))?;
+            let end: usize = bounds[1]
+                .parse()
+                .map_err(|_| AppError::Invalid(format!("invalid end index: {}", bounds[1])))?;
+            for i in start..=end {
+                indices.push(i);
+            }
+        } else {
+            let index: usize = part
+                .parse()
+                .map_err(|_| AppError::Invalid(format!("invalid index: {part}")))?;
+            indices.push(index);
+        }
+    }
+    Ok(indices)
+}