feat: Improve Security of Goose Extensions Via Environment Variable Denylist (#1803)

crates/goose/src/agents/extension.rs

@@ -3,6 +3,7 @@ use std::collections::HashMap;
 use mcp_client::client::Error as ClientError;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
+use tracing::warn;
 use utoipa::ToSchema;
 
 use crate::config;
@@ -19,6 +20,8 @@ pub enum ExtensionError {
     ContextLimit,
     #[error("Transport error: {0}")]
     Transport(#[from] mcp_client::transport::Error),
+    #[error("Environment variable `{0}` is not allowed to be overridden.")]
+    InvalidEnvVar(String),
 }
 
 pub type ExtensionResult<T> = Result<T, ExtensionError>;
@@ -32,15 +35,80 @@ pub struct Envs {
 }
 
 impl Envs {
+    /// List of sensitive env vars that should not be overridden
+    const DISALLOWED_KEYS: [&'static str; 31] = [
+        // 🔧 Binary path manipulation
+        "PATH",       // Controls executable lookup paths — critical for command hijacking
+        "PATHEXT",    // Windows: Determines recognized executable extensions (e.g., .exe, .bat)
+        "SystemRoot", // Windows: Can affect system DLL resolution (e.g., `kernel32.dll`)
+        "windir",     // Windows: Alternative to SystemRoot (used in legacy apps)
+        // 🧬 Dynamic linker hijacking (Linux/macOS)
+        "LD_LIBRARY_PATH",  // Alters shared library resolution
+        "LD_PRELOAD",       // Forces preloading of shared libraries — common attack vector
+        "LD_AUDIT",         // Loads a monitoring library that can intercept execution
+        "LD_DEBUG",         // Enables verbose linker logging (information disclosure risk)
+        "LD_BIND_NOW",      // Forces immediate symbol resolution, affecting ASLR
+        "LD_ASSUME_KERNEL", // Tricks linker into thinking it’s running on an older kernel
+        // 🍎 macOS dynamic linker variables
+        "DYLD_LIBRARY_PATH",     // Same as LD_LIBRARY_PATH but for macOS
+        "DYLD_INSERT_LIBRARIES", // macOS equivalent of LD_PRELOAD
+        "DYLD_FRAMEWORK_PATH",   // Overrides framework lookup paths
+        // 🐍 Python / Node / Ruby / Java / Golang hijacking
+        "PYTHONPATH",   // Overrides Python module resolution
+        "PYTHONHOME",   // Overrides Python root directory
+        "NODE_OPTIONS", // Injects options/scripts into every Node.js process
+        "RUBYOPT",      // Injects Ruby execution flags
+        "GEM_PATH",     // Alters where RubyGems looks for installed packages
+        "GEM_HOME",     // Changes RubyGems default install location
+        "CLASSPATH",    // Java: Controls where classes are loaded from — critical for RCE attacks
+        "GO111MODULE",  // Go: Forces use of module proxy or disables it
+        "GOROOT", // Go: Changes root installation directory (could lead to execution hijacking)
+        // 🖥️ Windows-specific process & DLL hijacking
+        "APPINIT_DLLS", // Forces Windows to load a DLL into every process
+        "SESSIONNAME",  // Affects Windows session configuration
+        "ComSpec",      // Determines default command interpreter (can replace `cmd.exe`)
+        "TEMP",
+        "TMP",          // Redirects temporary file storage (useful for injection attacks)
+        "LOCALAPPDATA", // Controls application data paths (can be abused for persistence)
+        "USERPROFILE",  // Windows user directory (can affect profile-based execution paths)
+        "HOMEDRIVE",
+        "HOMEPATH", // Changes where the user's home directory is located
+    ];
+
+    /// Constructs a new Envs, skipping disallowed env vars with a warning
     pub fn new(map: HashMap<String, String>) -> Self {
-        Self { map }
+        let mut validated = HashMap::new();
+
+        for (key, value) in map {
+            if Self::is_disallowed(&key) {
+                warn!("Skipping disallowed env var: {}", key);
+                continue;
+            }
+            validated.insert(key, value);
+        }
+
+        Self { map: validated }
     }
 
+    /// Returns a copy of the validated env vars
     pub fn get_env(&self) -> HashMap<String, String> {
-        self.map
+        self.map.clone()
+    }
+
+    /// Returns an error if any disallowed env var is present
+    pub fn validate(&self) -> Result<(), Box<ExtensionError>> {
+        for key in self.map.keys() {
+            if Self::is_disallowed(key) {
+                return Err(Box::new(ExtensionError::InvalidEnvVar(key.clone())));
+            }
+        }
+        Ok(())
+    }
+
+    fn is_disallowed(key: &str) -> bool {
+        Self::DISALLOWED_KEYS
             .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
+            .any(|disallowed| disallowed.eq_ignore_ascii_case(key))
     }
 }