🛡️ Sentinel: [CRITICAL] Fix arbitrary command execution via profile.bitcoin_cli

.jules/sentinel.md

@@ -0,0 +1,5 @@
+## 2024-03-26 - Centralize and secure bitcoin-cli execution
+
+**Vulnerability:** `run_bitcoin_cli` is defined in two places (`src/utils.rs` and `src/wallet_service.rs`) and executes the `bitcoin_cli` command defined in a user-provided profile without strict validation, potentially allowing arbitrary command execution if the profile is tampered with or maliciously constructed.
+**Learning:** External command execution based on configurable profile values must be strictly validated against an allowlist (e.g., only "bitcoin-cli" or "bitcoin-cli.exe") to prevent arbitrary command injection. Duplicated security-critical functions make auditing harder.
+**Prevention:** Centralize external process execution and enforce strict validation on executable names derived from configurations.

src/utils.rs

@@ -104,6 +104,20 @@ pub fn run_bitcoin_cli(
     profile: &Profile,
     args: &[String],
 ) -> Result<String, crate::error::AppError> {
+    // SECURITY 🛡️: Validate binary name to prevent arbitrary command execution via configuration.
+    // The profile is untrusted input. We must ensure the command is exactly the expected binary.
+    let bin_name = std::path::Path::new(&profile.bitcoin_cli)
+        .file_name()
+        .and_then(|n| n.to_str())
+        .unwrap_or("");
+
+    if bin_name != "bitcoin-cli" && bin_name != "bitcoin-cli.exe" {
+        return Err(crate::error::AppError::Config(format!(
+            "security violation: execution of arbitrary binaries is blocked. Expected 'bitcoin-cli', got '{}'",
+            profile.bitcoin_cli
+        )));
+    }
+
     let mut cmd = std::process::Command::new(&profile.bitcoin_cli);
     for arg in &profile.bitcoin_cli_args {
         cmd.arg(arg);

src/wallet_service.rs

@@ -190,26 +190,5 @@ pub fn persist_wallet_session(session: &mut WalletSession) -> Result<(), AppErro
     write_profile(&session.profile_path, &session.profile)
 }
 
-#[allow(dead_code)]
-pub fn run_bitcoin_cli(profile: &Profile, args: &[String]) -> Result<String, AppError> {
-    let output = ShellCommand::new(&profile.bitcoin_cli)
-        .args(&profile.bitcoin_cli_args)
-        .args(args)
-        .output()
-        .map_err(|e| AppError::Config(format!("failed to launch {}: {e}", profile.bitcoin_cli)))?;
-
-    if !output.status.success() {
-        let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
-        let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string();
-        let details = if !stderr.is_empty() { stderr } else { stdout };
-        return Err(AppError::Network(format!(
-            "bitcoin-cli command failed: {} {}",
-            profile.bitcoin_cli, details
-        )));
-    }
-
-    Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
-}
-
 #[cfg(test)]
 mod tests {}