From 7bbb9cc5cd5e17129bca50741921e44707f8cd59 Mon Sep 17 00:00:00 2001 From: Dorian Date: Wed, 18 Mar 2026 00:45:15 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20Phase=203=20=E2=80=94=20command=20inject?= =?UTF-8?q?ion,=20unwrap/expect=20panics,=20unsigned=20image=20acceptance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - VPN key gen: replaced sh -c with format string (command injection) with safe stdin piping to wg pubkey - Secrets manager: replaced .unwrap() on path.parent() with proper error - Tor proxy: replaced .expect("valid proxy") with continue on error - Image verifier: added require_signatures flag, strict mode rejects unsigned images and missing cosign binary Co-Authored-By: Claude Opus 4.6 (1M context) --- core/archipelago/src/api/rpc/tor.rs | 6 +++++- core/archipelago/src/vpn.rs | 31 ++++++++++++++-------------- core/security/src/image_verifier.rs | 28 ++++++++++++++++++++----- core/security/src/secrets_manager.rs | 4 +++- loop/plan.md | 10 ++++----- 5 files changed, 51 insertions(+), 28 deletions(-) diff --git a/core/archipelago/src/api/rpc/tor.rs b/core/archipelago/src/api/rpc/tor.rs index 10f02e44..a399290c 100644 --- a/core/archipelago/src/api/rpc/tor.rs +++ b/core/archipelago/src/api/rpc/tor.rs @@ -522,7 +522,11 @@ async fn notify_federation_peers_address_change( }); let url = format!("http://{}/rpc/v1", &peer.onion); let client = match reqwest::Client::builder() - .proxy(reqwest::Proxy::all(format!("socks5h://{}", proxy)).unwrap_or_else(|_| reqwest::Proxy::all("socks5h://127.0.0.1:9050").expect("valid proxy"))) + .proxy(match reqwest::Proxy::all(format!("socks5h://{}", proxy)) + .or_else(|_| reqwest::Proxy::all("socks5h://127.0.0.1:9050")) { + Ok(p) => p, + Err(_) => continue, + }) .timeout(std::time::Duration::from_secs(30)) .build() { diff --git a/core/archipelago/src/vpn.rs b/core/archipelago/src/vpn.rs index adab62ee..ea705eba 100644 --- a/core/archipelago/src/vpn.rs +++ b/core/archipelago/src/vpn.rs @@ -121,30 +121,29 @@ pub async fn generate_wireguard_keypair() -> Result<(String, String)> { .trim() .to_string(); - let _pubkey_output = tokio::process::Command::new("wg") + let mut child = tokio::process::Command::new("wg") .arg("pubkey") .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) .spawn() .context("Failed to spawn wg pubkey")?; - // Use echo + pipe approach instead - let pubkey_output = tokio::process::Command::new("sh") - .arg("-c") - .arg(format!("echo '{}' | wg pubkey", private_key)) - .output() - .await - .context("Failed to derive public key")?; - - if !pubkey_output.status.success() { - anyhow::bail!( - "wg pubkey failed: {}", - String::from_utf8_lossy(&pubkey_output.stderr) - ); + if let Some(mut stdin) = child.stdin.take() { + use tokio::io::AsyncWriteExt; + stdin.write_all(private_key.as_bytes()).await + .context("Failed to write private key to wg stdin")?; } - let public_key = String::from_utf8(pubkey_output.stdout) - .context("Invalid UTF-8 from wg pubkey")? + let output = child.wait_with_output().await + .context("wg pubkey process failed")?; + + if !output.status.success() { + anyhow::bail!("wg pubkey failed: {}", String::from_utf8_lossy(&output.stderr)); + } + + let public_key = String::from_utf8(output.stdout) + .context("wg pubkey output is not valid UTF-8")? .trim() .to_string(); diff --git a/core/security/src/image_verifier.rs b/core/security/src/image_verifier.rs index 310557de..1eb80d12 100644 --- a/core/security/src/image_verifier.rs +++ b/core/security/src/image_verifier.rs @@ -6,28 +6,46 @@ use std::process::Command; use tracing::{info, warn}; pub struct ImageVerifier { - cosign_public_key: Option, // Public key for verification + cosign_public_key: Option, + require_signatures: bool, } impl ImageVerifier { pub fn new(cosign_public_key: Option) -> Self { - Self { cosign_public_key } + Self { cosign_public_key, require_signatures: false } } - + + /// Create a verifier that requires all images to be signed. + pub fn new_strict(cosign_public_key: Option) -> Self { + Self { cosign_public_key, require_signatures: true } + } + /// Verify a container image signature pub async fn verify_image(&self, image: &str, signature: Option<&str>) -> Result { if signature.is_none() && self.cosign_public_key.is_none() { + if self.require_signatures { + return Err(anyhow::anyhow!( + "Image '{}' has no signature and no cosign key is configured. \ + All container images must be signed for production use.", + image + )); + } warn!("No signature provided for image: {}", image); return Ok(false); } - + // Check if cosign is available let cosign_available = Command::new("cosign") .arg("version") .output() .is_ok(); - + if !cosign_available { + if self.require_signatures { + return Err(anyhow::anyhow!( + "Cosign binary not found. Install cosign to verify container image signatures." + )); + } warn!("Cosign not available, skipping signature verification"); return Ok(false); } diff --git a/core/security/src/secrets_manager.rs b/core/security/src/secrets_manager.rs index ed5f7d54..84b7de21 100644 --- a/core/security/src/secrets_manager.rs +++ b/core/security/src/secrets_manager.rs @@ -109,7 +109,9 @@ impl SecretsManager { .join(app_id) .join(format!("{}.secret", secret_id)); - fs::create_dir_all(secret_path.parent().unwrap()).await?; + let parent = secret_path.parent() + .ok_or_else(|| anyhow::anyhow!("Invalid secret path: no parent directory for {:?}", secret_path))?; + fs::create_dir_all(parent).await?; let encrypted = self .encrypt(value.as_bytes()) diff --git a/loop/plan.md b/loop/plan.md index eead4417..1e3ef36a 100644 --- a/loop/plan.md +++ b/loop/plan.md @@ -190,7 +190,7 @@ > server (command injection) or crash your entire node at will (unwrap panic). These are the most > dangerous code-level bugs found. -- [ ] **Fix command injection in VPN key generation**: In `core/archipelago/src/vpn.rs`, find lines 132-137 where `sh -c` is used with `format!("echo '{}' | wg pubkey", private_key)`. This is a textbook command injection vulnerability. Replace the entire block with safe stdin piping: +- [x] **Fix command injection in VPN key generation**: In `core/archipelago/src/vpn.rs`, find lines 132-137 where `sh -c` is used with `format!("echo '{}' | wg pubkey", private_key)`. This is a textbook command injection vulnerability. Replace the entire block with safe stdin piping: ```rust let mut child = tokio::process::Command::new("wg") .arg("pubkey") @@ -223,7 +223,7 @@ Build: `cd ~/archy/core && cargo clippy --all-targets --all-features`. Test: If VPN setup is available in the UI, test generating a WireGuard key. -- [ ] **Fix unwrap crash in secrets manager**: In `core/security/src/secrets_manager.rs`, find line 112 with `secret_path.parent().unwrap()`. Replace with: +- [x] **Fix unwrap crash in secrets manager**: In `core/security/src/secrets_manager.rs`, find line 112 with `secret_path.parent().unwrap()`. Replace with: ```rust let parent = secret_path.parent() .ok_or_else(|| anyhow::anyhow!("Invalid secret path: no parent directory for {:?}", secret_path))?; @@ -232,7 +232,7 @@ Search for ALL `.unwrap()` calls in the file: `grep -n "unwrap()" core/security/src/secrets_manager.rs`. For each one in a non-test function, evaluate whether it can actually fail and replace with `?` or `.ok_or_else()` if so. Common safe unwraps (e.g., after a `.is_some()` check) can stay but should get a comment explaining why they're safe. Build and deploy. -- [ ] **Fix expect crash in Tor proxy fallback**: In `core/archipelago/src/api/rpc/tor.rs`, find line ~525 with `.expect("valid proxy")`. Replace the entire proxy chain with proper error handling: +- [x] **Fix expect crash in Tor proxy fallback**: In `core/archipelago/src/api/rpc/tor.rs`, find line ~525 with `.expect("valid proxy")`. Replace the entire proxy chain with proper error handling: ```rust let proxy_url = format!("socks5h://{}", proxy); let proxy = reqwest::Proxy::all(&proxy_url) @@ -242,7 +242,7 @@ Search for ALL `.expect(` calls in non-test code: `grep -rn "\.expect(" core/archipelago/src/ --include="*.rs" | grep -v "#\[cfg(test)\]" | grep -v "mod tests"`. List them and fix any that could realistically fail in production. Build: `cargo clippy --all-targets --all-features`. -- [ ] **Fix image verifier accepting unsigned images**: In `core/security/src/image_verifier.rs`, find lines 18-22 where the verifier returns `Ok(false)` for unsigned images. Change to: +- [x] **Fix image verifier accepting unsigned images**: In `core/security/src/image_verifier.rs`, find lines 18-22 where the verifier returns `Ok(false)` for unsigned images. Change to: ```rust if signature.is_none() && self.cosign_public_key.is_none() { return Err(anyhow::anyhow!( @@ -262,7 +262,7 @@ ``` Build and test. Note: this may cause existing unsigned images to fail verification. If the system doesn't use cosign yet, add a config flag `require_signatures: bool` that defaults to `false` for now but can be flipped to `true` when cosign is deployed. -- [ ] **Verify Phase 3 — No more crash vectors**: Run these checks: +- [x] **Verify Phase 3 — No more crash vectors**: Run these checks: 1. `grep -rn 'Command::new("sh")' core/ --include="*.rs"` — should return zero results. 2. `grep -rn "\.unwrap()" core/security/src/secrets_manager.rs | grep -v test` — should be minimal/commented. 3. `grep -rn "\.expect(" core/archipelago/src/api/ --include="*.rs" | grep -v test | grep -v "// SAFE:"` — review each remaining expect.