From 27f87acd114d622671de3e565c5d8d11d9bf15df Mon Sep 17 00:00:00 2001 From: "aikido-autofix[bot]" <119856028+aikido-autofix[bot]@users.noreply.github.com> Date: Sat, 5 Sep 2026 17:42:16 +0000 Subject: [PATCH 1/2] fix(security): Restrict GPG program resolution to global Git configuration only --- src-tauri/src/commands/history.rs | 121 +++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/commands/history.rs b/src-tauri/src/commands/history.rs index 8db6969..3284efb 100644 --- a/src-tauri/src/commands/history.rs +++ b/src-tauri/src/commands/history.rs @@ -62,6 +62,7 @@ pub async fn verify_commits( trait VerificationRunner { fn git_config_get(&self, repo_path: &str, key: &str) -> Result, String>; + fn git_config_get_global(&self, repo_path: &str, key: &str) -> Result, String>; fn verify_signatures( &self, repo_path: &str, @@ -106,6 +107,33 @@ impl VerificationRunner for ProcessVerificationRunner { }) } + fn git_config_get_global(&self, repo_path: &str, key: &str) -> Result, String> { + let mut command = crate::configured_git_command(); + command.current_dir(repo_path); + // Use --global flag to only read from global config, not repository-local config. + // This prevents malicious repositories from specifying arbitrary executables. + let output = command + .args(["config", "--global", "--get", key]) + .output() + .map_err(|error| error.to_string())?; + + if output.status.success() { + let value = String::from_utf8_lossy(&output.stdout).trim().to_string(); + return Ok((!value.is_empty()).then_some(value)); + } + + if output.status.code() == Some(1) { + return Ok(None); + } + + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + Err(if stderr.is_empty() { + format!("Failed to get git config {key}") + } else { + stderr + }) + } + fn verify_signatures( &self, repo_path: &str, @@ -349,8 +377,16 @@ fn effective_gpg_program( repo_path: &str, runner: &impl VerificationRunner, ) -> Result { - if let Ok(Some(program)) = runner.git_config_get(repo_path, "gpg.program") { - return Ok(program); + // Only read gpg.program from global config to prevent repository-local + // config from specifying arbitrary executables. This mitigates a code execution + // vulnerability where a malicious repository could set gpg.program to an + // attacker-controlled executable. + if let Ok(Some(program)) = runner.git_config_get_global(repo_path, "gpg.program") { + if let Some(validated) = validate_gpg_program_path(&program) { + return Ok(validated); + } + // If validation fails, fall through to system defaults rather than failing, + // to maintain functionality when global config contains invalid values } #[cfg(windows)] @@ -363,6 +399,37 @@ fn effective_gpg_program( Ok("gpg".to_string()) } +/// Validates that a GPG program path is safe to execute. +/// Returns Some(validated_path) if the program is safe, None otherwise. +/// +/// Safety criteria: +/// - Simple command names (no path separators) are allowed and resolved via PATH +/// - Absolute paths must exist and point to a file +/// - Relative paths are rejected to prevent repository-controlled executables +fn validate_gpg_program_path(program: &str) -> Option { + let trimmed = program.trim().trim_matches('"'); + if trimmed.is_empty() { + return None; + } + + let path = Path::new(trimmed); + + // Check if this looks like a path (contains path separators) + let is_path_like = path.is_absolute() || trimmed.contains('/') || trimmed.contains('\\'); + + if is_path_like { + // For path-like values, only accept absolute paths that exist + if path.is_absolute() && path.is_file() { + return Some(trimmed.to_string()); + } + // Reject relative paths - they could point to repository-controlled files + return None; + } + + // Simple command names (no path separators) are safe - they'll be resolved via PATH + Some(trimmed.to_string()) +} + fn signature_key_type(commit_text: &str) -> Option { if commit_text.contains("-----BEGIN SSH SIGNATURE-----") { return Some("ssh".to_string()); @@ -437,6 +504,11 @@ mod tests { }) } + fn git_config_get_global(&self, _repo_path: &str, key: &str) -> Result, String> { + // For tests, treat global config the same as regular config + self.git_config_get(_repo_path, key) + } + fn verify_signatures( &self, _repo_path: &str, @@ -599,6 +671,51 @@ mod tests { assert_eq!(error, "fatal: bad revision"); } + + #[test] + fn validate_gpg_program_accepts_simple_command_names() { + assert_eq!( + validate_gpg_program_path("gpg"), + Some("gpg".to_string()) + ); + assert_eq!( + validate_gpg_program_path("gpg2"), + Some("gpg2".to_string()) + ); + assert_eq!( + validate_gpg_program_path(" gpg "), + Some("gpg".to_string()) + ); + } + + #[test] + fn validate_gpg_program_rejects_relative_paths() { + assert_eq!(validate_gpg_program_path("./gpg"), None); + assert_eq!(validate_gpg_program_path("../gpg"), None); + assert_eq!(validate_gpg_program_path("bin/gpg"), None); + assert_eq!(validate_gpg_program_path(".\\gpg"), None); + assert_eq!(validate_gpg_program_path("..\\gpg"), None); + } + + #[test] + fn validate_gpg_program_rejects_nonexistent_absolute_paths() { + assert_eq!( + validate_gpg_program_path("/nonexistent/path/to/gpg"), + None + ); + #[cfg(windows)] + assert_eq!( + validate_gpg_program_path("C:\\nonexistent\\path\\to\\gpg.exe"), + None + ); + } + + #[test] + fn validate_gpg_program_rejects_empty_values() { + assert_eq!(validate_gpg_program_path(""), None); + assert_eq!(validate_gpg_program_path(" "), None); + assert_eq!(validate_gpg_program_path("\"\""), None); + } } #[tauri::command] From 25f49f4e2b20a7803b73b514d0757ebfd94f0e36 Mon Sep 17 00:00:00 2001 From: cst8t <1810150+cst8t@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:07:47 +0100 Subject: [PATCH 2/2] fix(security): make GPG resolution tests portable Keep repository-local GPG configuration separate from the global test value so the security boundary is exercised consistently on Linux and Windows. --- src-tauri/src/commands/history.rs | 69 ++++++++++--------------------- 1 file changed, 22 insertions(+), 47 deletions(-) diff --git a/src-tauri/src/commands/history.rs b/src-tauri/src/commands/history.rs index 3284efb..56c4e99 100644 --- a/src-tauri/src/commands/history.rs +++ b/src-tauri/src/commands/history.rs @@ -110,8 +110,6 @@ impl VerificationRunner for ProcessVerificationRunner { fn git_config_get_global(&self, repo_path: &str, key: &str) -> Result, String> { let mut command = crate::configured_git_command(); command.current_dir(repo_path); - // Use --global flag to only read from global config, not repository-local config. - // This prevents malicious repositories from specifying arbitrary executables. let output = command .args(["config", "--global", "--get", key]) .output() @@ -377,16 +375,10 @@ fn effective_gpg_program( repo_path: &str, runner: &impl VerificationRunner, ) -> Result { - // Only read gpg.program from global config to prevent repository-local - // config from specifying arbitrary executables. This mitigates a code execution - // vulnerability where a malicious repository could set gpg.program to an - // attacker-controlled executable. if let Ok(Some(program)) = runner.git_config_get_global(repo_path, "gpg.program") { if let Some(validated) = validate_gpg_program_path(&program) { return Ok(validated); } - // If validation fails, fall through to system defaults rather than failing, - // to maintain functionality when global config contains invalid values } #[cfg(windows)] @@ -399,13 +391,7 @@ fn effective_gpg_program( Ok("gpg".to_string()) } -/// Validates that a GPG program path is safe to execute. -/// Returns Some(validated_path) if the program is safe, None otherwise. -/// -/// Safety criteria: -/// - Simple command names (no path separators) are allowed and resolved via PATH -/// - Absolute paths must exist and point to a file -/// - Relative paths are rejected to prevent repository-controlled executables +/// Accepts command names and existing absolute files, but not relative paths. fn validate_gpg_program_path(program: &str) -> Option { let trimmed = program.trim().trim_matches('"'); if trimmed.is_empty() { @@ -413,20 +399,16 @@ fn validate_gpg_program_path(program: &str) -> Option { } let path = Path::new(trimmed); - - // Check if this looks like a path (contains path separators) + let is_path_like = path.is_absolute() || trimmed.contains('/') || trimmed.contains('\\'); - + if is_path_like { - // For path-like values, only accept absolute paths that exist if path.is_absolute() && path.is_file() { return Some(trimmed.to_string()); } - // Reject relative paths - they could point to repository-controlled files return None; } - - // Simple command names (no path separators) are safe - they'll be resolved via PATH + Some(trimmed.to_string()) } @@ -454,7 +436,7 @@ mod tests { verification_succeeds: bool, verification_stderr: Option, key_types: HashMap, - gpg_program: Option, + global_gpg_program: Option, } impl FakeRunner { @@ -469,7 +451,7 @@ mod tests { verification_succeeds: true, verification_stderr: None, key_types: HashMap::new(), - gpg_program: None, + global_gpg_program: None, } } @@ -490,23 +472,25 @@ mod tests { self } - fn with_gpg_program(mut self, program: &str) -> Self { - self.gpg_program = Some(program.to_string()); + fn with_global_gpg_program(mut self, program: &str) -> Self { + self.global_gpg_program = Some(program.to_string()); self } } impl VerificationRunner for FakeRunner { fn git_config_get(&self, _repo_path: &str, key: &str) -> Result, String> { - Ok(match key { - "gpg.program" => self.gpg_program.clone(), - _ => None, - }) + Ok((key == "gpg.program").then(|| "repository-controlled-gpg".to_string())) } - fn git_config_get_global(&self, _repo_path: &str, key: &str) -> Result, String> { - // For tests, treat global config the same as regular config - self.git_config_get(_repo_path, key) + fn git_config_get_global( + &self, + _repo_path: &str, + key: &str, + ) -> Result, String> { + Ok((key == "gpg.program") + .then(|| self.global_gpg_program.clone()) + .flatten()) } fn verify_signatures( @@ -596,14 +580,14 @@ mod tests { "a\x1fG\x1fAlice\x1fABC123\x1fABC123", ]) .with_key_type("a", "gpg") - .with_gpg_program("/usr/local/bin/gpg"); + .with_global_gpg_program("gpg"); let results = verify_commit_signatures("/repo", &["a".to_string()], true, &runner) .expect("verification should complete"); assert_eq!(results[0].status, SignatureStatus::Verified); assert_eq!( runner.fetched_keys.borrow().as_slice(), - &["/usr/local/bin/gpg ABC123".to_string()] + &["gpg ABC123".to_string()] ); assert_eq!( runner.verified_hashes.borrow().as_slice(), @@ -674,14 +658,8 @@ mod tests { #[test] fn validate_gpg_program_accepts_simple_command_names() { - assert_eq!( - validate_gpg_program_path("gpg"), - Some("gpg".to_string()) - ); - assert_eq!( - validate_gpg_program_path("gpg2"), - Some("gpg2".to_string()) - ); + assert_eq!(validate_gpg_program_path("gpg"), Some("gpg".to_string())); + assert_eq!(validate_gpg_program_path("gpg2"), Some("gpg2".to_string())); assert_eq!( validate_gpg_program_path(" gpg "), Some("gpg".to_string()) @@ -699,10 +677,7 @@ mod tests { #[test] fn validate_gpg_program_rejects_nonexistent_absolute_paths() { - assert_eq!( - validate_gpg_program_path("/nonexistent/path/to/gpg"), - None - ); + assert_eq!(validate_gpg_program_path("/nonexistent/path/to/gpg"), None); #[cfg(windows)] assert_eq!( validate_gpg_program_path("C:\\nonexistent\\path\\to\\gpg.exe"),