🛡️ Sentinel: [HIGH] Fix path traversal and secure permissions in snapshots

.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-05-28 - Path Traversal Vulnerability in Profile and Snapshot Creation
+**Vulnerability:** User input for snapshot names (e.g. `../../etc/passwd`) was used directly in `snap_dir.join(format!("{name}.json"))` and could allow writing arbitrary files or traversing paths.
+**Learning:** `Path::join` combined with user input string allows relative paths like `../` to navigate out of the intended base directory.
+**Prevention:** Validate input strings to ensure they are alphanumeric and do not contain directory traversal characters like `/`, `\`, `.` or `..`. Replace insecure standard `fs::create_dir_all` with the internal secure utility `crate::paths::create_secure_dir_all`.

src/commands/snapshot.rs

@@ -4,14 +4,27 @@ use crate::output::CommandOutput;
 use crate::{confirm, profile_path, read_profile, snapshot_dir, write_bytes_atomic};
 use std::fs;
 
+fn is_valid_snapshot_name(name: &str) -> bool {
+    !name.is_empty()
+        && name
+            .chars()
+            .all(|c| c.is_alphanumeric() || c == '_' || c == '-')
+}
+
 pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppError> {
     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 } => {
+            if !is_valid_snapshot_name(name) {
+                return Err(AppError::Invalid(
+                    "invalid snapshot name. Only alphanumeric, '_', and '-' are allowed."
+                        .to_string(),
+                ));
+            }
             let source = read_profile(&profile_path)?;
             let destination = snap_dir.join(format!("{name}.json"));
             if destination.exists() && !(*overwrite || cli.yes) {
@@ -27,6 +40,12 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppErr
             })
         }
         SnapshotAction::Restore { name } => {
+            if !is_valid_snapshot_name(name) {
+                return Err(AppError::Invalid(
+                    "invalid snapshot name. Only alphanumeric, '_', and '-' are allowed."
+                        .to_string(),
+                ));
+            }
             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()));
             }