diff --git a/Cargo.lock b/Cargo.lock index f65225de1bd947..17acf0e6ed997b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -494,6 +494,7 @@ dependencies = [ "heapless", "html_to_markdown", "http_client", + "idna", "image", "indoc", "itertools 0.14.0", @@ -549,6 +550,7 @@ dependencies = [ "tree-sitter-md", "ui", "ui_input", + "unicode-script", "unicode-segmentation", "unindent", "url", @@ -15995,6 +15997,7 @@ dependencies = [ "libc", "log", "nix 0.29.0", + "seccompiler", "serde", "serde_json", "smol", @@ -16312,6 +16315,15 @@ dependencies = [ "zeroize", ] +[[package]] +name = "seccompiler" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4ae55de56877481d112a559bbc12667635fdaf5e005712fd4e2b2fa50ffc884" +dependencies = [ + "libc", +] + [[package]] name = "secrecy" version = "0.10.3" @@ -16724,6 +16736,7 @@ dependencies = [ "agent_skills", "anyhow", "audio", + "client", "cloud_api_types", "codestral", "collections", diff --git a/Cargo.toml b/Cargo.toml index 916cb147fa5554..c657ff1aefb58a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -744,6 +744,7 @@ rustls-platform-verifier = "0.5.0" # WARNING: If you change this, you must also publish a new version of zed-scap to crates.io scap = { git = "https://github.com/zed-industries/scap", rev = "4afea48c3b002197176fb19cd0f9b180dd36eaac", default-features = false, package = "zed-scap", version = "0.0.8-zed" } schemars = { version = "1.0", features = ["indexmap2"] } +seccompiler = "0.5" semver = { version = "1.0", features = ["serde"] } serde = { version = "1.0.221", features = ["derive", "rc"] } serde_json = { version = "1.0.144", features = ["preserve_order", "raw_value"] } diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index bb5378fd4684c4..c2b2fe35d4d4ff 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -224,6 +224,11 @@ pub struct SandboxFallbackAuthorizationDetails { /// whether to run the command without a sandbox. #[serde(default)] pub reason: String, + /// Slug of the sandboxing docs section that best explains how to fix this + /// failure (see [`crate::LinuxWslSandboxError::docs_section`]), rendered as a + /// "Learn more" link. `None` when the cause is unknown. + #[serde(default)] + pub docs_section: Option, } pub fn meta_with_sandbox_fallback_authorization( diff --git a/crates/acp_thread/src/terminal.rs b/crates/acp_thread/src/terminal.rs index 3bc33e2a957edc..cce475268e68aa 100644 --- a/crates/acp_thread/src/terminal.rs +++ b/crates/acp_thread/src/terminal.rs @@ -129,6 +129,31 @@ impl LinuxWslSandboxError { LinuxWslSandboxError::Other(message) => message.clone(), } } + + /// The slug of the sandboxing docs section that best explains how to resolve + /// this failure, for deep-linking from the UI. Pair with + /// `client::zed_urls::sandboxing_docs`. + pub fn docs_section(&self) -> &'static str { + match self { + // Both "no bwrap" and "only a setuid-root bwrap" are resolved by + // installing a non-setuid Bubblewrap. + LinuxWslSandboxError::BwrapNotFound | LinuxWslSandboxError::SetuidRejected => { + "installing-bubblewrap" + } + // A failed probe on Linux is almost always disabled unprivileged + // user namespaces, which the Ubuntu-specific section covers. + LinuxWslSandboxError::SandboxProbeFailed => "installing-bubblewrap-ubuntu", + // Catch-all (includes WSL/Windows messages): point at the platform + // overview for the current OS. + LinuxWslSandboxError::Other(_) => { + if cfg!(target_os = "windows") { + "windows" + } else { + "linux" + } + } + } + } } impl SandboxWrap { @@ -151,6 +176,15 @@ impl SandboxWrap { /// grant as a [`sandbox::HostFilesystemLocation`] (pinning the inode / canonical /// path) rather than passing a re-resolvable path. A location that can't be /// captured (e.g. it doesn't exist) is dropped from the grant — fail-closed. + /// + /// This function has **no filesystem side effects**: it never creates paths. + /// It is used both by the side-effect-free [`Self::can_create_sandbox`] probe + /// and by real sandbox construction, and must behave identically. On Linux a + /// writable grant that doesn't exist yet simply can't be captured (bwrap + /// can't bind a missing path), so it's dropped here — the sanctioned way to + /// get a grant to a new directory is the `create_directory` tool, which + /// creates it (pinning the inode) before the grant is recorded. On macOS a + /// missing leaf still canonicalizes, so such grants are captured directly. fn to_policy(&self) -> sandbox::SandboxPolicy { let protected_paths = self .protected_paths @@ -164,12 +198,11 @@ impl SandboxWrap { .writable_paths .iter() .chain(self.extra_write_paths.iter()) - .filter_map(|path| { - // Create not-yet-existing writable grants (e.g. an approved - // scratch dir) so they can be captured and bound; best-effort. - let _ = std::fs::create_dir_all(path); - sandbox::HostFilesystemLocation::new(path).ok() - }) + // Capture only — never create anything here (see the doc comment): + // materializing an approved-but-missing grant is deferred to + // `Sandbox::new` so it can never happen during the `can_create` + // probe, before the user has approved the grant. + .filter_map(|path| sandbox::HostFilesystemLocation::new(path).ok()) .collect(); sandbox::SandboxFsPolicy::Restricted { writable_paths, diff --git a/crates/agent/src/sandboxing.rs b/crates/agent/src/sandboxing.rs index e8d6589601eef4..90599fd6551c71 100644 --- a/crates/agent/src/sandboxing.rs +++ b/crates/agent/src/sandboxing.rs @@ -5,10 +5,9 @@ //! caller see the same answer (and so the `target_os` gate lives in one //! place instead of scattered across the agent crate). //! -//! The current policy is: enabled iff the user has the `sandboxing` feature -//! flag turned on, the project is local, the platform has an integration, and -//! the user has not persistently allowed unsandboxed execution (the -//! `allow_unsandboxed` sandbox setting). Setting `allow_unsandboxed` +//! The current policy is: enabled iff the project is local, the platform has an +//! integration, and the user has not persistently allowed unsandboxed execution +//! (the `allow_unsandboxed` sandbox setting). Setting `allow_unsandboxed` //! persistently turns sandboxing off for the model-facing surface entirely: //! the plain (non-sandboxed) `terminal` tool is exposed and the system prompt //! omits the sandbox section, since every command would run without a wrap @@ -19,14 +18,12 @@ //! //! macOS (Seatbelt), Linux (Bubblewrap), and Windows (Bubblewrap via WSL) //! have real sandbox integrations; on platforms without one the per-command -//! wrap is a no-op, so commands run with the agent's ambient permissions even -//! when the flag is on. +//! wrap is a no-op, so commands run with the agent's ambient permissions. //! //! Naming note: this module is about agent terminal sandboxing specifically. //! Other agent operations (e.g. file edits) are gated separately. use agent_settings::{AgentSettings, SandboxPermissions}; -use feature_flags::{FeatureFlagAppExt as _, SandboxingFeatureFlag}; use gpui::App; use http_proxy::HostPattern; use project::Project; @@ -176,12 +173,6 @@ pub fn settings_sandbox_policy(persistent: &SandboxPermissions) -> SandboxPolicy SandboxPolicy { fs, network } } -/// Whether agent-run terminal commands should be wrapped in an OS-level -/// sandbox for this process. See module docs for the policy. -pub(crate) fn sandboxing_enabled(cx: &App) -> bool { - cx.has_flag::() -} - /// Whether the sandboxed terminal can be exposed for this project. /// /// The persistent `allow_unsandboxed` setting turns sandboxing off for the @@ -193,20 +184,19 @@ pub(crate) fn sandboxing_enabled(cx: &App) -> bool { /// prompt in place, since the model is still operating in the sandbox model and /// only escaping individual commands (tracked in `ThreadSandboxGrants`). pub(crate) fn sandboxing_enabled_for_project(project: &Project, cx: &App) -> bool { - sandboxing_available_for_project(project, cx) + sandboxing_available_for_project(project) && !AgentSettings::get_global(cx) .sandbox_permissions .allow_unsandboxed } -/// Whether sandboxing is *applicable* for this project at all — the feature is -/// enabled, the project is local, and the platform has a sandbox integration — -/// independent of the persistent `allow_unsandboxed` setting. Used by the UI to -/// distinguish "sandboxing isn't relevant here" (don't show the indicator) from -/// "sandboxing is available but turned off in settings" (show it, struck out). -pub(crate) fn sandboxing_available_for_project(project: &Project, cx: &App) -> bool { - sandboxing_enabled(cx) - && project.is_local() +/// Whether sandboxing is *applicable* for this project at all — the project is +/// local and the platform has a sandbox integration — independent of the +/// persistent `allow_unsandboxed` setting. Used by the UI to distinguish +/// "sandboxing isn't relevant here" (don't show the indicator) from "sandboxing +/// is available but turned off in settings" (show it, struck out). +pub(crate) fn sandboxing_available_for_project(project: &Project) -> bool { + project.is_local() && cfg!(any( target_os = "macos", target_os = "linux", diff --git a/crates/agent/src/templates/system_prompt.hbs b/crates/agent/src/templates/system_prompt.hbs index 160c94328d168a..233a0e45215f6c 100644 --- a/crates/agent/src/templates/system_prompt.hbs +++ b/crates/agent/src/templates/system_prompt.hbs @@ -198,7 +198,7 @@ You can request elevated permissions on individual `terminal` calls: - `allow_hosts: ["github.com", "*.npmjs.org"]` — allow outbound HTTP/HTTPS to specific hosts (exact hostnames or leading-`*.` subdomain wildcards; no IP literals). Prefer this whenever you know which hosts the command needs. - `allow_all_hosts: true` — lift the network restriction entirely: outbound access to any host over any protocol, so SSH, FTP, and raw sockets work too (unlike `allow_hosts`, which is HTTP/HTTPS-only). Use only when the specific hosts can't be enumerated up front. {{/if}} -- `fs_write_paths: ["/abs/or/worktree-relative/path", ...]` — allow writes to specific paths (each directory grants its whole subtree). Prefer this whenever you know which paths the command needs to write. Git metadata paths cannot be requested and will never be made writable while sandboxed. +- `fs_write_paths: ["/abs/or/worktree-relative/path", ...]` — allow writes to specific paths (each directory grants its whole subtree). Prefer this whenever you know which paths the command needs to write. Each path must be an existing directory. To write into a directory that doesn't exist yet, first create it with the `create_directory` tool (which creates it and grants write access to exactly that directory) rather than requesting write access to a broad existing parent. Git metadata paths cannot be requested and will never be made writable while sandboxed. - `allow_fs_write_all: true` — allow unrestricted filesystem writes except protected Git metadata. Only use this when the specific paths can't be enumerated up front. - `unsandboxed: true` — run the command with no sandbox at all. Use only when none of the above suffice, including when a command must write Git metadata. diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 446dee9f4636b8..73415f55af4db8 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -7390,6 +7390,194 @@ async fn test_fetch_tool_unsandboxed_lifts_restrictions(cx: &mut TestAppContext) ); } +/// A granted host that redirects to a loopback target must not have that +/// redirect followed: loopback hosts can't be granted individually, so the hop +/// is refused just like a direct loopback fetch. This is the redirect variant of +/// the SSRF protection — the approved domain can't be used to bounce the request +/// onto the local machine. +#[gpui::test] +async fn test_fetch_tool_refuses_redirect_to_loopback(cx: &mut TestAppContext) { + init_test(cx); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + FetchTool::NAME.into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Allow), + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + settings + .sandbox_permissions + .network_hosts + .push("example.com".into()); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let http_client = gpui::http_client::FakeHttpClient::create(|req| async move { + let uri = req.uri().to_string(); + assert!( + uri.contains("example.com"), + "the loopback redirect target must never be requested, but saw {uri}" + ); + Ok(gpui::http_client::Response::builder() + .status(302) + .header("location", "http://localhost:3000/internal") + .body("".into()) + .unwrap()) + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::FetchTool::new(http_client)); + let (event_stream, _rx) = crate::ToolCallEventStream::test(); + + let input: crate::FetchToolInput = + serde_json::from_value(json!({"url": "https://example.com/start"})).unwrap(); + + let task = cx.update(|cx| tool.run(ToolInput::resolved(input), event_stream, cx)); + let result = task.await; + assert!( + result.is_err(), + "expected a redirect to a loopback host to be refused" + ); + assert!( + result.unwrap_err().contains("unsandboxed"), + "error should point at unsandboxed access as the way to reach loopback hosts" + ); +} + +/// A granted host that redirects to a *different*, ungranted host triggers a +/// fresh per-host authorization prompt for the redirect target — the redirect is +/// not silently followed to a host the user never approved. +#[gpui::test] +async fn test_fetch_tool_reauthorizes_redirect_to_new_host(cx: &mut TestAppContext) { + init_test(cx); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + FetchTool::NAME.into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Allow), + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + settings + .sandbox_permissions + .network_hosts + .push("example.com".into()); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let http_client = gpui::http_client::FakeHttpClient::create(|req| async move { + let uri = req.uri().to_string(); + assert!( + uri.contains("example.com"), + "the ungranted redirect target must not be requested before authorization, \ + but saw {uri}" + ); + Ok(gpui::http_client::Response::builder() + .status(302) + .header("location", "https://redirect-target.example/landing") + .body("".into()) + .unwrap()) + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::FetchTool::new(http_client)); + let (event_stream, mut rx) = crate::ToolCallEventStream::test(); + + let input: crate::FetchToolInput = + serde_json::from_value(json!({"url": "https://example.com/start"})).unwrap(); + + let _task = cx.update(|cx| tool.run(ToolInput::resolved(input), event_stream, cx)); + + cx.run_until_parked(); + + let authorization = rx.expect_authorization().await; + let details = + acp_thread::sandbox_authorization_details_from_meta(&authorization.tool_call.meta) + .expect("a redirect to an ungranted host should request a sandbox network grant"); + assert_eq!( + details.network_hosts, + vec!["redirect-target.example".to_string()] + ); + assert!(!details.network_all_hosts); +} + +/// Redirects between paths on an already-granted host are followed without any +/// additional prompt, so ordinary redirects (http→https upgrades, trailing-slash +/// canonicalization, etc.) keep working after the per-hop authorization change. +#[gpui::test] +async fn test_fetch_tool_follows_same_host_redirect(cx: &mut TestAppContext) { + init_test(cx); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + FetchTool::NAME.into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Allow), + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + settings + .sandbox_permissions + .network_hosts + .push("example.com".into()); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + let http_client = gpui::http_client::FakeHttpClient::create(|req| async move { + let uri = req.uri().to_string(); + if uri.ends_with("/start") { + Ok(gpui::http_client::Response::builder() + .status(302) + .header("location", "https://example.com/final") + .body("".into()) + .unwrap()) + } else if uri.ends_with("/final") { + Ok(gpui::http_client::Response::builder() + .status(200) + .header("content-type", "text/plain") + .body("final content".into()) + .unwrap()) + } else { + panic!("unexpected request to {uri}"); + } + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::FetchTool::new(http_client)); + let (event_stream, mut rx) = crate::ToolCallEventStream::test(); + + let input: crate::FetchToolInput = + serde_json::from_value(json!({"url": "https://example.com/start"})).unwrap(); + + let task = cx.update(|cx| tool.run(ToolInput::resolved(input), event_stream, cx)); + let result = task.await; + assert_eq!( + result.expect("same-host redirect should succeed"), + "final content" + ); + + let event = rx.try_recv(); + assert!( + !matches!(event, Ok(Ok(ThreadEvent::ToolCallAuthorization(_)))), + "expected no authorization prompt for a redirect to an already-granted host" + ); +} + /// Approving one pending tool call with "Always for " auto-resolves /// sibling pending authorizations for the same tool in the same turn. #[gpui::test] diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 2c8c5e3dea36f8..fe5c9f2004cb70 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -1846,12 +1846,12 @@ impl Thread { sandboxing_enabled_for_project(self.project.read(cx), cx) } - /// Whether sandboxing is *applicable* for this thread's project (feature on, - /// local project, supported platform), regardless of whether it's been - /// turned off in settings. The UI shows the sandbox indicator whenever this - /// is true, drawing it struck-out when sandboxing is disabled. + /// Whether sandboxing is *applicable* for this thread's project (local + /// project, supported platform), regardless of whether it's been turned off + /// in settings. The UI shows the sandbox indicator whenever this is true, + /// drawing it struck-out when sandboxing is disabled. pub fn sandboxing_available(&self, cx: &App) -> bool { - sandboxing_available_for_project(self.project.read(cx), cx) + sandboxing_available_for_project(self.project.read(cx)) } /// The directory subtrees the sandbox always grants write access to for this @@ -5926,10 +5926,15 @@ impl ToolCallEventStream { &self, command: Option, reason: String, + docs_section: Option, retries: usize, cx: &mut App, ) -> Task> { - let details = acp_thread::SandboxFallbackAuthorizationDetails { command, reason }; + let details = acp_thread::SandboxFallbackAuthorizationDetails { + command, + reason, + docs_section, + }; let retry_label = if retries == 0 { "Retry".to_string() } else { @@ -7686,6 +7691,7 @@ mod tests { event_stream.authorize_sandbox_fallback( Some("cargo build".to_string()), "bwrap not found on PATH".to_string(), + Some("installing-bubblewrap".to_string()), 0, cx, ) @@ -7697,6 +7703,10 @@ mod tests { .expect("fallback authorization should include details"); assert_eq!(details.command.as_deref(), Some("cargo build")); assert_eq!(details.reason, "bwrap not found on PATH"); + assert_eq!( + details.docs_section.as_deref(), + Some("installing-bubblewrap") + ); let acp_thread::PermissionOptions::Flat(options) = &authorization.options else { panic!("expected flat fallback permission options"); @@ -7737,6 +7747,7 @@ mod tests { event_stream.authorize_sandbox_fallback( None, "probe failed".to_string(), + None, retries, cx, ) @@ -7781,6 +7792,7 @@ mod tests { event_stream.authorize_sandbox_fallback( Some("cargo build".to_string()), "user namespaces are disabled".to_string(), + None, 0, cx, ) @@ -7810,7 +7822,13 @@ mod tests { let (event_stream, mut receiver) = ToolCallEventStream::test(); let authorize = cx.update(|cx| { - event_stream.authorize_sandbox_fallback(None, "bwrap probe failed".to_string(), 0, cx) + event_stream.authorize_sandbox_fallback( + None, + "bwrap probe failed".to_string(), + None, + 0, + cx, + ) }); let authorization = receiver.expect_authorization().await; authorization diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index 308e7b9145805f..22918c43154857 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -5,7 +5,7 @@ use super::tool_permissions::{ use agent_client_protocol::schema::v1 as acp; use agent_settings::AgentSettings; use futures::FutureExt as _; -use gpui::{App, Entity, SharedString, Task}; +use gpui::{App, AppContext as _, AsyncApp, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -17,12 +17,13 @@ use crate::{ AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, authorize_with_sensitive_settings, decide_permission_for_path, }; -use std::path::Path; +use std::path::{Path, PathBuf}; -/// Creates a new directory at the specified path within the project. Returns confirmation that the directory was created. +/// Creates a new directory at the specified path, and all necessary parent directories. Returns confirmation that the directory was created. /// -/// This tool creates a directory and all necessary parent directories. It should be used whenever you need to create new directories within the project. -/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. +/// Use this whenever you need to create new directories. Paths inside the project are created directly. +/// +/// This tool can also create a directory **outside** the project. When agent terminal commands are sandboxed, doing so grants those commands write access to exactly that new directory — so, rather than requesting write access to a broad existing parent (e.g. your home directory) just to create something inside it, create the specific directory here first and then write into it. The only other supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct CreateDirectoryToolInput { /// The path of the new directory. @@ -40,6 +41,13 @@ pub struct CreateDirectoryToolInput { /// To create a global agent skill directory, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill`. /// pub path: String, + + /// Justification for creating a directory **outside** the project, shown to + /// the user (attributed to you) in the approval prompt that grants sandboxed + /// terminal commands write access to it. Required only for out-of-project + /// paths; ignored for paths inside the project or the global skills dir. + #[serde(default)] + pub reason: Option, } pub struct CreateDirectoryTool { @@ -83,6 +91,28 @@ impl AgentTool for CreateDirectoryTool { let project = self.project.clone(); cx.spawn(async move |cx| { let input = input.recv().await.map_err(|e| e.to_string())?; + + let fs = project.read_with(cx, |project, _cx| project.fs().clone()); + + // Resolve where this directory lives. The global agent-skills dir is a + // special case allowed outside the project; anything else outside the + // project is handled as a narrow sandbox write grant below. + let global_skill_directory = + resolve_creatable_global_skill_path(Path::new(&input.path), fs.as_ref()).await; + let in_project = project.read_with(cx, |project, cx| { + project.find_project_path(&input.path, cx).is_some() + }); + + // A path outside the project (and not the global skills dir) can only + // be created as a narrow sandbox write grant: create the directory and + // grant sandboxed terminal commands write access to exactly it. The + // sandbox approval prompt — which shows the real, canonicalized target + // — fully replaces the normal permission and symlink-escape prompts + // here. + if global_skill_directory.is_none() && !in_project { + return create_out_of_project_directory(&project, &input, &event_stream, cx).await; + } + let decision = cx.update(|cx| { decide_permission_for_path(Self::NAME, &input.path, AgentSettings::get_global(cx)) }); @@ -93,7 +123,6 @@ impl AgentTool for CreateDirectoryTool { let destination_path: Arc = input.path.as_str().into(); - let fs = project.read_with(cx, |project, _cx| project.fs().clone()); let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; let symlink_escape_target = project.read_with(cx, |project, cx| { @@ -149,9 +178,7 @@ impl AgentTool for CreateDirectoryTool { authorize.await.map_err(|e| e.to_string())?; } - if let Some(global_skill_directory) = - resolve_creatable_global_skill_path(Path::new(&input.path), fs.as_ref()).await - { + if let Some(global_skill_directory) = global_skill_directory { futures::select! { result = fs.create_dir(&global_skill_directory).fuse() => { result.map_err(|e| format!("Creating directory {destination_path}: {e}"))?; @@ -185,6 +212,100 @@ impl AgentTool for CreateDirectoryTool { } } +/// Create a directory that lives **outside** the project by granting sandboxed +/// terminal commands write access to exactly it. +/// +/// The directory is created (Linux: eagerly, pinning the inode; macOS: after +/// approval) and the user is shown the real, canonicalized target in the sandbox +/// approval prompt — which is what defends against a concurrent symlink swap: the +/// grant is always against the inode/path the user actually saw. On denial, only +/// the directories we created are removed. +async fn create_out_of_project_directory( + project: &Entity, + input: &CreateDirectoryToolInput, + event_stream: &ToolCallEventStream, + cx: &mut AsyncApp, +) -> Result { + // Narrowing a grant to a brand-new directory only makes sense when the + // project's terminal commands are sandboxed, and only on platforms that can + // grant a not-yet-existing directory. Otherwise keep the historical + // "outside the project" rejection. + let sandboxing = project.read_with(cx, |project, cx| { + crate::sandboxing::sandboxing_enabled_for_project(project, cx) + }); + let platform_supported = cfg!(any(target_os = "linux", target_os = "macos")); + if !sandboxing || !platform_supported { + return Err("Path to create was outside the project".to_string()); + } + + let Some(reason) = input + .reason + .as_deref() + .map(str::trim) + .filter(|reason| !reason.is_empty()) + else { + return Err( + "Creating a directory outside the project grants sandboxed terminal commands write \ + access to it, so a `reason` is required: briefly justify why the directory is needed, \ + then try again." + .to_string(), + ); + }; + let reason = reason.to_string(); + + let absolute = resolve_absolute_path(project, &input.path, cx) + .ok_or_else(|| format!("Couldn't resolve `{}` to an absolute path.", input.path))?; + + let prepared = cx + .background_spawn(async move { sandbox::GrantableWriteDir::prepare(&absolute) }) + .await + .map_err(|error| format!("Creating directory {}: {error}", input.path))?; + + let canonical = prepared.canonical_path().to_path_buf(); + let request = crate::sandboxing::SandboxRequest { + write_paths: vec![canonical.clone()], + ..Default::default() + }; + + let approve = cx.update(|cx| event_stream.authorize_sandbox(request, reason, cx)); + match approve.await { + Ok(()) => { + let display = canonical.display().to_string(); + cx.background_spawn(async move { prepared.finalize() }) + .await + .map_err(|error| format!("Creating directory {display}: {error}"))?; + Ok(format!("Created directory {display}")) + } + Err(error) => { + // Roll back exactly what we created; leave the user no litter. + cx.background_spawn(async move { prepared.discard() }).await; + Err(format!("Create directory cancelled: {error}")) + } + } +} + +/// Resolve a model-provided path to an absolute, lexically-normalized path. +/// Relative paths are joined onto the first worktree root. +fn resolve_absolute_path( + project: &Entity, + raw: &str, + cx: &mut AsyncApp, +) -> Option { + let path = Path::new(raw); + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + let base = project.read_with(cx, |project, cx| { + project + .worktrees(cx) + .next() + .map(|worktree| worktree.read(cx).abs_path().to_path_buf()) + })?; + base.join(path) + }; + util::paths::normalize_lexically(&absolute).ok() +} + #[cfg(test)] mod tests { use super::*; @@ -231,7 +352,10 @@ mod tests { let (event_stream, mut event_rx) = ToolCallEventStream::test(); let task = cx.update(|cx| { tool.run( - ToolInput::resolved(CreateDirectoryToolInput { path: input_path }), + ToolInput::resolved(CreateDirectoryToolInput { + path: input_path, + reason: None, + }), event_stream, cx, ) @@ -286,6 +410,7 @@ mod tests { tool.run( ToolInput::resolved(CreateDirectoryToolInput { path: outside_path.to_string_lossy().into_owned(), + reason: None, }), event_stream, cx, @@ -342,6 +467,7 @@ mod tests { tool.run( ToolInput::resolved(CreateDirectoryToolInput { path: "project/link_to_external".into(), + reason: None, }), event_stream, cx, @@ -404,6 +530,7 @@ mod tests { tool.run( ToolInput::resolved(CreateDirectoryToolInput { path: "project/link_to_external".into(), + reason: None, }), event_stream, cx, @@ -463,6 +590,7 @@ mod tests { tool.run( ToolInput::resolved(CreateDirectoryToolInput { path: "project/link_to_external".into(), + reason: None, }), event_stream, cx, @@ -545,6 +673,7 @@ mod tests { tool.run( ToolInput::resolved(CreateDirectoryToolInput { path: "project/link_to_external".into(), + reason: None, }), event_stream, cx, @@ -561,4 +690,107 @@ mod tests { "Deny policy should not emit symlink authorization prompt", ); } + + /// Out-of-project creation goes through the sandbox write-grant prompt and, + /// on approval, creates the *specific* new directory (not its broad parent). + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[gpui::test] + async fn test_create_directory_out_of_project_creates_and_grants(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({ "project": { "src": {} } })) + .await; + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + // The sandbox create path operates on the *real* filesystem, so use a + // real directory outside the (fake) project. + let scratch = tempfile::tempdir().unwrap(); + let target = scratch.path().join("new_grant_dir"); + assert!(!target.exists()); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let path_input = target.to_string_lossy().into_owned(); + let task = cx.update(|cx| { + tool.run( + ToolInput::resolved(CreateDirectoryToolInput { + path: path_input, + reason: Some("scratch space for the build".into()), + }), + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let details = acp_thread::sandbox_authorization_details_from_meta(&auth.tool_call.meta) + .expect("out-of-project create should request a sandbox write grant"); + // The grant is for exactly the new directory, not its parent. + assert_eq!( + details.write_paths, + vec![scratch.path().canonicalize().unwrap().join("new_grant_dir")] + ); + + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new(acp_thread::SandboxPermission::AllowThread.as_id()), + acp::PermissionOptionKind::AllowAlways, + )) + .unwrap(); + + let result = task.await; + assert!(result.is_ok(), "expected success, got {result:?}"); + assert!( + target.is_dir(), + "the new directory should have been created" + ); + } + + /// Denying the grant removes the directory we eagerly created, leaving no + /// trace on the filesystem. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[gpui::test] + async fn test_create_directory_out_of_project_denied_cleans_up(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({ "project": { "src": {} } })) + .await; + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let scratch = tempfile::tempdir().unwrap(); + let target = scratch.path().join("denied_dir"); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let path_input = target.to_string_lossy().into_owned(); + let task = cx.update(|cx| { + tool.run( + ToolInput::resolved(CreateDirectoryToolInput { + path: path_input, + reason: Some("scratch space".into()), + }), + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new(acp_thread::SandboxPermission::Deny.as_id()), + acp::PermissionOptionKind::RejectOnce, + )) + .unwrap(); + + let result = task.await; + assert!(result.is_err(), "denied create should fail"); + assert!( + !target.exists(), + "denied create should leave no directory behind" + ); + } } diff --git a/crates/agent/src/tools/fetch_tool.rs b/crates/agent/src/tools/fetch_tool.rs index 96c0fd2aba175b..cfe7792c9f144e 100644 --- a/crates/agent/src/tools/fetch_tool.rs +++ b/crates/agent/src/tools/fetch_tool.rs @@ -23,12 +23,38 @@ enum ContentType { Json, } +/// The maximum number of HTTP redirects the fetch tool will follow. Each hop is +/// re-authorized against the shared network grants before being followed. +const MAX_REDIRECTS: usize = 20; + +/// The outcome of a single (non-redirect-following) HTTP request. +enum FetchStep { + /// The server responded with a redirect to this absolute URL. Its host must + /// be authorized before the redirect is followed. + Redirect(String), + /// A terminal response was received and converted to Markdown. + Complete(String), +} + +/// Prepends `https://` when the URL has no explicit HTTP(S) scheme, matching the +/// behavior the fetch tool has always had for user/model-supplied URLs. +fn normalize_url(url: &str) -> Cow<'_, str> { + if !url.starts_with("https://") && !url.starts_with("http://") { + Cow::Owned(format!("https://{url}")) + } else { + Cow::Borrowed(url) + } +} + /// Fetches a URL and returns the content as Markdown. /// /// This tool is not run inside the terminal OS sandbox, but it still refuses to /// reach any host that hasn't been granted network access. It shares the same /// per-host grants as the `terminal` tool: approving a host for one authorizes /// it for the other, whether the grant is for this thread or saved permanently. +/// HTTP redirects are followed one hop at a time, and each hop's host must be +/// granted the same way, so a granted host can't redirect the request to a host +/// that hasn't been approved. /// When unsandboxed access has been granted, these restrictions are lifted /// entirely, matching the terminal, which is also how loopback and IP-literal /// hosts (which can't be granted individually) become reachable. @@ -47,14 +73,35 @@ impl FetchTool { Self { http_client } } - async fn build_message(http_client: Arc, url: &str) -> Result { - let url = if !url.starts_with("https://") && !url.starts_with("http://") { - Cow::Owned(format!("https://{url}")) - } else { - Cow::Borrowed(url) - }; + /// Performs a single HTTP GET *without* following redirects, so the tool can + /// re-authorize each hop against the shared network grants before following + /// it. Returns the redirect target when the server responds with a 3xx, or + /// the final content converted to Markdown otherwise. + async fn fetch_step(http_client: Arc, url: &str) -> Result { + let normalized = normalize_url(url); + + let mut response = http_client + .get(&normalized, AsyncBody::default(), false) + .await?; - let mut response = http_client.get(&url, AsyncBody::default(), true).await?; + let status = response.status(); + if status.is_redirection() { + let location = response + .headers() + .get("location") + .context("redirect response is missing a Location header")? + .to_str() + .context("redirect response has an invalid Location header")?; + let target = url::Url::parse(&normalized) + .with_context(|| format!("could not parse URL {normalized:?}"))? + .join(location) + .with_context(|| format!("invalid redirect target {location:?}"))?; + anyhow::ensure!( + matches!(target.scheme(), "http" | "https"), + "refusing to follow redirect to non-HTTP(S) URL {target}" + ); + return Ok(FetchStep::Redirect(target.to_string())); + } let mut body = Vec::new(); response @@ -63,12 +110,9 @@ impl FetchTool { .await .context("error reading response body")?; - if response.status().is_client_error() { + if status.is_client_error() { let text = String::from_utf8_lossy(body.as_slice()); - bail!( - "status error {}, response: {text:?}", - response.status().as_u16() - ); + bail!("status error {}, response: {text:?}", status.as_u16()); } let Some(content_type) = response.headers().get("content-type") else { @@ -86,7 +130,7 @@ impl FetchTool { ContentType::Html }; - match content_type { + let text = match content_type { ContentType::Html => { let mut handlers: Vec = vec![ Rc::new(RefCell::new(markdown::WebpageChromeRemover)), @@ -96,7 +140,7 @@ impl FetchTool { Rc::new(RefCell::new(markdown::TableHandler::new())), Rc::new(RefCell::new(markdown::StyledTextHandler)), ]; - if url.contains("wikipedia.org") { + if normalized.contains("wikipedia.org") { use html_to_markdown::structure::wikipedia; handlers.push(Rc::new(RefCell::new(wikipedia::WikipediaChromeRemover))); @@ -108,30 +152,25 @@ impl FetchTool { handlers.push(Rc::new(RefCell::new(markdown::CodeHandler))); } - convert_html_to_markdown(&body[..], &mut handlers) + convert_html_to_markdown(&body[..], &mut handlers)? } - ContentType::Plaintext => Ok(std::str::from_utf8(&body)?.to_owned()), + ContentType::Plaintext => std::str::from_utf8(&body)?.to_owned(), ContentType::Json => { let json: serde_json::Value = serde_json::from_slice(&body)?; - Ok(format!( - "```json\n{}\n```", - serde_json::to_string_pretty(&json)? - )) + format!("```json\n{}\n```", serde_json::to_string_pretty(&json)?) } - } + }; + + Ok(FetchStep::Complete(text)) } } /// Extracts the host from a fetch URL as a [`http_proxy::HostPattern`] so it can /// be matched against the shared network grants. Mirrors the scheme handling in -/// [`FetchTool::build_message`] (defaulting to `https://` when none is given). +/// [`normalize_url`] (defaulting to `https://` when none is given). fn host_pattern_for_url(url: &str) -> Result { - let normalized = if !url.starts_with("https://") && !url.starts_with("http://") { - Cow::Owned(format!("https://{url}")) - } else { - Cow::Borrowed(url) - }; + let normalized = normalize_url(url); let parsed = url::Url::parse(&normalized).with_context(|| format!("could not parse URL {url:?}"))?; let host = parsed @@ -211,36 +250,61 @@ impl AgentTool for FetchTool { // already runs without isolation, so we drop fetch's restrictions // too — including reaching hosts that can't be granted individually // (loopback and IP literals). + // + // Crucially, this authorization is applied to every redirect hop as + // well as the initial URL, so a granted host can't 30x-redirect the + // fetch to a host the user never approved. We disable the HTTP + // client's own redirect following and re-run the grant for each hop + // before requesting it. let unsandboxed = cx.update(|cx| event_stream.unsandboxed_access_granted(cx)); - if !unsandboxed { - let host = host_pattern_for_url(&input.url).map_err(|e| e.to_string())?; - let authorize_host = cx.update(|cx| { - let request = SandboxRequest { - network: NetworkRequest::Hosts(vec![host]), - ..Default::default() + + let mut current_url = input.url.clone(); + let mut redirects = 0; + let text = loop { + if !unsandboxed { + let host = host_pattern_for_url(¤t_url).map_err(|e| e.to_string())?; + let authorize_host = cx.update(|cx| { + let request = SandboxRequest { + network: NetworkRequest::Hosts(vec![host]), + ..Default::default() + }; + event_stream.authorize_sandbox(request, String::new(), cx) + }); + futures::select! { + result = authorize_host.fuse() => result.map_err(|e| e.to_string())?, + _ = event_stream.cancelled_by_user().fuse() => { + return Err("Fetch cancelled by user".to_string()); + } }; - event_stream.authorize_sandbox(request, String::new(), cx) + } + + let fetch_task = cx.background_spawn({ + let http_client = http_client.clone(); + let url = current_url.clone(); + async move { Self::fetch_step(http_client, &url).await } }); - futures::select! { - result = authorize_host.fuse() => result.map_err(|e| e.to_string())?, + + let step = futures::select! { + result = fetch_task.fuse() => result.map_err(|e| e.to_string())?, _ = event_stream.cancelled_by_user().fuse() => { return Err("Fetch cancelled by user".to_string()); } }; - } - - let fetch_task = cx.background_spawn({ - let http_client = http_client.clone(); - let url = input.url.clone(); - async move { Self::build_message(http_client, &url).await } - }); - let text = futures::select! { - result = fetch_task.fuse() => result.map_err(|e| e.to_string())?, - _ = event_stream.cancelled_by_user().fuse() => { - return Err("Fetch cancelled by user".to_string()); + match step { + FetchStep::Complete(text) => break text, + FetchStep::Redirect(target) => { + redirects += 1; + if redirects > MAX_REDIRECTS { + return Err(format!( + "exceeded the maximum of {MAX_REDIRECTS} redirects" + )); + } + current_url = target; + } } }; + if text.trim().is_empty() { return Err("no textual content found".to_string()); } diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index ef394c5e8ba14d..3e7954e5ba67d4 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -558,8 +558,10 @@ async fn run_terminal_tool( if !path.is_dir() { return Err(format!( "Cannot request sandbox write access to `{}`: on Linux, write access can only \ - be granted to directories that already exist. To create or modify files, \ - request write access to the existing directory that contains them, not the \ + be granted to directories that already exist. To create a new directory to write \ + into, use the `create_directory` tool (which creates it and grants write access to \ + exactly that directory) rather than requesting its parent. To modify existing \ + files, request write access to the existing directory that contains them, not the \ file path itself.", path.display() )); @@ -683,6 +685,7 @@ async fn run_terminal_tool( event_stream.authorize_sandbox_fallback( Some(input.command.clone()), error.user_facing_message(), + Some(error.docs_section().to_string()), retries, cx, ) @@ -788,6 +791,7 @@ async fn run_terminal_tool( event_stream.authorize_sandbox_fallback( Some(input.command.clone()), sandbox_error.user_facing_message(), + Some(sandbox_error.docs_section().to_string()), retries, cx, ) diff --git a/crates/agent_settings/src/agent_settings.rs b/crates/agent_settings/src/agent_settings.rs index 5d884e8a6ce47e..e1cce89d3b0223 100644 --- a/crates/agent_settings/src/agent_settings.rs +++ b/crates/agent_settings/src/agent_settings.rs @@ -414,7 +414,7 @@ impl Default for AgentProfileId { /// combines them with the in-memory per-thread grants. `write_paths` are /// stored as minimal, lexically-normalized subtrees (see /// [`compile_sandbox_permissions`]). -#[derive(Clone, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct SandboxPermissions { /// Allow sandboxed commands to reach any host over the network. pub allow_all_hosts: bool, @@ -432,6 +432,24 @@ pub struct SandboxPermissions { /// tool/prompt in place — see `agent::sandboxing`. pub allow_unsandboxed: bool, pub write_paths: Vec, + /// Whether sandbox escalation prompts warn about domains or write paths + /// that contain potentially confusable Unicode characters (homoglyphs, + /// invisible characters, or bidirectional overrides). Enabled by default. + pub warn_confusable_unicode: bool, +} + +impl Default for SandboxPermissions { + fn default() -> Self { + Self { + allow_all_hosts: false, + network_hosts: Vec::new(), + allow_fs_write_all: false, + allow_unsandboxed: false, + write_paths: Vec::new(), + // The confusable-Unicode warning is a safety net, so it defaults on. + warn_confusable_unicode: true, + } + } } #[derive(Clone, Debug, Default)] @@ -821,6 +839,7 @@ fn compile_sandbox_permissions( allow_fs_write_all: content.allow_fs_write_all.unwrap_or(false), allow_unsandboxed: content.allow_unsandboxed.unwrap_or(false), write_paths, + warn_confusable_unicode: content.warn_confusable_unicode.unwrap_or(true), } } @@ -1098,6 +1117,22 @@ mod tests { fn test_sandbox_permissions_empty() { let permissions = compile_sandbox_permissions(None); assert_eq!(permissions, SandboxPermissions::default()); + // The confusable-Unicode warning is a safety net, so it's on by default. + assert!(permissions.warn_confusable_unicode); + } + + #[test] + fn test_sandbox_permissions_warn_confusable_unicode_can_be_disabled() { + let content: settings::SandboxPermissionsContent = + serde_json::from_value(json!({ "warn_confusable_unicode": false })).unwrap(); + let permissions = compile_sandbox_permissions(Some(content)); + assert!(!permissions.warn_confusable_unicode); + + // Omitting the key keeps the warning enabled. + let content: settings::SandboxPermissionsContent = + serde_json::from_value(json!({})).unwrap(); + let permissions = compile_sandbox_permissions(Some(content)); + assert!(permissions.warn_confusable_unicode); } #[test] diff --git a/crates/agent_ui/Cargo.toml b/crates/agent_ui/Cargo.toml index 822b91c3cd19ce..3ccf5297380648 100644 --- a/crates/agent_ui/Cargo.toml +++ b/crates/agent_ui/Cargo.toml @@ -63,6 +63,7 @@ gpui.workspace = true gpui_tokio.workspace = true html_to_markdown.workspace = true http_client.workspace = true +idna.workspace = true indoc.workspace = true itertools.workspace = true jsonschema.workspace = true @@ -108,6 +109,7 @@ theme_settings.workspace = true time.workspace = true ui.workspace = true ui_input.workspace = true +unicode-script.workspace = true unicode-segmentation.workspace = true url.workspace = true util.workspace = true diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 1640a90297f091..91ae944e700e1b 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -35,6 +35,7 @@ pub mod thread_worktree_archive; pub mod threads_archive_view; mod ui; +mod unicode_confusables; use std::rc::Rc; use std::sync::Arc; diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index c1fe29024872e3..5a2ae6fd725502 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -25,6 +25,7 @@ use sandbox::{SandboxFsPolicy, SandboxNetPolicy, SandboxPolicy}; use crate::completion_provider::{AvailableSkill, PromptLocalCommand}; use crate::message_editor::SharedSessionCapabilities; use crate::ui::{SandboxGroup, SandboxRow, SandboxSection, SandboxStatusTooltip}; +use crate::unicode_confusables; use db::kvp::KeyValueStore; use gpui::List; @@ -38,8 +39,8 @@ use language_model::{ use notifications::status_toast::StatusToast; use settings::{update_settings_file, update_settings_file_with_completion}; use ui::{ - ButtonLike, CalloutBorderPosition, SpinnerLabel, SpinnerVariant, SplitButton, SplitButtonStyle, - Tab, + ButtonLike, CalloutBorderPosition, Checkbox, SpinnerLabel, SpinnerVariant, SplitButton, + SplitButtonStyle, Tab, ToggleState, }; use workspace::{OpenOptions, SERIALIZATION_THROTTLE_TIME}; @@ -588,6 +589,10 @@ pub struct ThreadView { pub expanded_tool_call_raw_inputs: HashSet, collapsed_sandbox_authorization_details: HashSet, collapsed_sandbox_network_details: HashSet, + /// Sandbox escalation prompts whose "surprising Unicode" warning the user + /// has explicitly acknowledged. Until a prompt's tool call is in this set, + /// its allow buttons stay disabled. See [`Self::sandbox_confusable_findings`]. + acknowledged_confusable_warnings: HashSet, pub subagent_scroll_handles: RefCell>, pub edits_expanded: bool, pub plan_expanded: bool, @@ -995,6 +1000,7 @@ impl ThreadView { expanded_tool_call_raw_inputs: HashSet::default(), collapsed_sandbox_authorization_details: HashSet::default(), collapsed_sandbox_network_details: HashSet::default(), + acknowledged_confusable_warnings: HashSet::default(), subagent_scroll_handles: RefCell::new(HashMap::default()), edits_expanded: false, plan_expanded: false, @@ -2485,13 +2491,40 @@ impl ThreadView { } pub fn allow_always(&mut self, _: &AllowAlways, window: &mut Window, cx: &mut Context) { + if self.pending_allow_blocked_by_confusables(cx) { + return; + } self.authorize_pending_tool_call(acp::PermissionOptionKind::AllowAlways, window, cx); } pub fn allow_once(&mut self, _: &AllowOnce, window: &mut Window, cx: &mut Context) { + if self.pending_allow_blocked_by_confusables(cx) { + return; + } self.authorize_pending_with_granularity(true, window, cx); } + /// Whether the currently pending permission prompt is blocked by an + /// unacknowledged surprising-Unicode warning, so the keyboard allow + /// shortcuts must be ignored (mirroring the disabled allow buttons). + fn pending_allow_blocked_by_confusables(&self, cx: &Context) -> bool { + let session_id = self.thread.read(cx).session_id().clone(); + let Some((_, tool_call_id, _)) = self + .conversation + .read(cx) + .pending_tool_call(&session_id, cx) + else { + return false; + }; + self.thread.read(cx).entries().iter().any(|entry| { + matches!( + entry, + AgentThreadEntry::ToolCall(call) + if call.id == tool_call_id && self.sandbox_confusables_block_allow(call, cx) + ) + }) + } + pub fn reject_once(&mut self, _: &RejectOnce, window: &mut Window, cx: &mut Context) { self.authorize_pending_with_granularity(false, window, cx); } @@ -7894,6 +7927,7 @@ impl ThreadView { }) .when_some(confirmation_options, |this, options| { let is_first = self.is_first_tool_call(active_session_id, &tool_call.id, cx); + let allow_disabled = self.sandbox_confusables_block_allow(tool_call, cx); this.child(self.render_permission_buttons( self.thread.read(cx).session_id().clone(), is_first, @@ -7901,6 +7935,7 @@ impl ThreadView { entry_ix, tool_call.id.clone(), focus_handle, + allow_disabled, cx, )) }) @@ -7914,34 +7949,44 @@ impl ThreadView { reason: &SandboxNotAppliedReason, cx: &Context, ) -> AnyElement { - let (title, detail): (SharedString, SharedString) = match reason { - SandboxNotAppliedReason::ErrorLinuxWsl(error) => ( - "Couldn't create a sandbox".into(), - error.user_facing_message().into(), - ), - SandboxNotAppliedReason::DisabledForThisThread => { - // The grant only exists because an earlier command failed to - // create a sandbox; surface that same explanation here. - let detail = self - .find_thread_sandbox_error(cx) - .map(|error| { - SharedString::from(format!( - "Allowed for this thread after the sandbox failed: {}", - error.user_facing_message() - )) - }) - .unwrap_or_else(|| { - "Unsandboxed execution is allowed for the rest of this thread.".into() - }); - ("Ran without sandbox".into(), detail) - } - }; + // (title, detail line, docs section slug) + let (title, detail, docs_section): (SharedString, SharedString, Option<&'static str>) = + match reason { + SandboxNotAppliedReason::ErrorLinuxWsl(error) => ( + "Couldn't create a sandbox".into(), + error.user_facing_message().into(), + Some(error.docs_section()), + ), + SandboxNotAppliedReason::DisabledForThisThread => { + // The grant only exists because an earlier command failed to + // create a sandbox; surface that same explanation here. + let thread_error = self.find_thread_sandbox_error(cx); + let detail = thread_error + .as_ref() + .map(|error| { + SharedString::from(format!( + "Allowed for this thread after the sandbox failed: {}", + error.user_facing_message() + )) + }) + .unwrap_or_else(|| { + "Unsandboxed execution is allowed for the rest of this thread.".into() + }); + let docs_section = thread_error.as_ref().map(|error| error.docs_section()); + ("Ran without sandbox".into(), detail, docs_section) + } + }; Callout::new() .severity(Severity::Warning) .icon(IconName::Warning) .title(title) .description(detail) + .actions_slot(self.render_sandbox_docs_link( + "sandbox-not-applied-docs-link", + docs_section, + cx, + )) .into_any_element() } @@ -8140,6 +8185,7 @@ impl ThreadView { entry_ix, &tool_call.id, details, + window, cx, )) }, @@ -8248,6 +8294,7 @@ impl ThreadView { entry_ix, tool_call.id.clone(), focus_handle, + self.sandbox_confusables_block_allow(tool_call, cx), cx, )) .into_any() @@ -8554,11 +8601,42 @@ impl ThreadView { .children(tool_output_display) } + /// A small "Learn more" link to the sandboxing docs, deep-linked to + /// `section` when provided. Shared by the sandbox warning and the two + /// sandbox approval prompts so the user can always reach an explanation of + /// what they're being asked about. + fn render_sandbox_docs_link( + &self, + id: &'static str, + section: Option<&str>, + cx: &Context, + ) -> AnyElement { + let url = zed_urls::sandboxing_docs(section, cx); + let tooltip = format!("Opens {url}"); + // Wrap in a row so the button shrinks to its content width instead of + // stretching to fill the enclosing column. + h_flex() + .child( + Button::new(id, "Learn more") + .label_size(LabelSize::Small) + .color(Color::Muted) + .end_icon( + Icon::new(IconName::ArrowUpRight) + .color(Color::Muted) + .size(IconSize::XSmall), + ) + .tooltip(Tooltip::text(tooltip)) + .on_click(move |_, _, cx| cx.open_url(&url)), + ) + .into_any_element() + } + fn render_sandbox_authorization_details( &self, entry_ix: usize, tool_call_id: &acp::ToolCallId, details: &SandboxAuthorizationDetails, + window: &Window, cx: &Context, ) -> AnyElement { let has_network = details.network_all_hosts || !details.network_hosts.is_empty(); @@ -8567,6 +8645,12 @@ impl ThreadView { return Empty.into_any_element(); } + let confusable_findings = if Self::confusable_warning_enabled(cx) { + Self::sandbox_confusable_findings(details) + } else { + Vec::new() + }; + let network_section = has_network.then(|| { let summary = if details.network_all_hosts { "any host".to_string() @@ -8794,10 +8878,186 @@ impl ThreadView { v_flex() .border_t_1() .border_color(self.tool_card_border_color(cx)) + .when(!confusable_findings.is_empty(), |this| { + this.child(self.render_sandbox_confusable_warning( + tool_call_id, + &confusable_findings, + window, + cx, + )) + }) .children(network_section) .children(write_section) .children(unsandboxed_section) .children(reason_section) + .child( + h_flex() + .px_1() + .py_0p5() + .child(self.render_sandbox_docs_link( + "sandbox-authorization-docs-link", + None, + cx, + )), + ) + .into_any_element() + } + + /// Scan the hosts and paths in a sandbox escalation request for surprising + /// Unicode characters (homoglyphs, invisible characters, bidi overrides). + /// Returns, for each offending value, the display string shown to the user + /// and the distinct suspicious characters it contains. Hosts are decoded from + /// Punycode first, so the display string is the Unicode form the user should + /// scrutinize. Empty when nothing is surprising. + fn sandbox_confusable_findings( + details: &SandboxAuthorizationDetails, + ) -> Vec<(String, Vec)> { + let mut findings = Vec::new(); + for host in &details.network_hosts { + let (decoded, suspicious) = unicode_confusables::scan_host(host); + if !suspicious.is_empty() { + findings.push((decoded, suspicious)); + } + } + for path in &details.write_paths { + let display = path.display().to_string(); + let suspicious = unicode_confusables::scan(&display); + if !suspicious.is_empty() { + findings.push((display, suspicious)); + } + } + findings + } + + /// Whether the surprising-Unicode warning is enabled in settings (on by + /// default). When off, prompts neither show the banner nor gate their allow + /// buttons on it. + fn confusable_warning_enabled(cx: &App) -> bool { + AgentSettings::get_global(cx) + .sandbox_permissions + .warn_confusable_unicode + } + + /// Whether this tool call's sandbox escalation shows surprising Unicode that + /// the user hasn't acknowledged yet. While true, the prompt's allow buttons + /// stay disabled so the user can't grant access to a lookalike target + /// without first ticking the acknowledgement checkbox. + fn sandbox_confusables_block_allow(&self, tool_call: &ToolCall, cx: &App) -> bool { + if !Self::confusable_warning_enabled(cx) { + return false; + } + let Some(details) = tool_call.sandbox_authorization_details.as_ref() else { + return false; + }; + if self + .acknowledged_confusable_warnings + .contains(&tool_call.id) + { + return false; + } + !Self::sandbox_confusable_findings(details).is_empty() + } + + /// Red banner warning that a requested domain or path contains surprising + /// Unicode characters, with a checkbox the user must tick to unlock the + /// allow buttons. See [`Self::sandbox_confusables_block_allow`]. + fn render_sandbox_confusable_warning( + &self, + tool_call_id: &acp::ToolCallId, + findings: &[(String, Vec)], + window: &Window, + cx: &Context, + ) -> AnyElement { + let acknowledged = self.acknowledged_confusable_warnings.contains(tool_call_id); + let line_height = window.line_height(); + + v_flex() + .w_full() + .p_2() + .gap_2() + .border_t_1() + .border_color(cx.theme().status().error_border) + .bg(cx.theme().status().error_background.opacity(0.15)) + .child( + h_flex() + .w_full() + .gap_1p5() + .items_start() + .child( + h_flex() + .h(line_height) + .flex_none() + .justify_center() + .child( + Icon::new(IconName::Warning) + .size(IconSize::Small) + .color(Color::Error), + ), + ) + .child( + v_flex().min_w_0().flex_1().gap_1().children(findings.iter().map( + |(value, suspicious)| { + v_flex() + .min_w_0() + .gap_0p5() + .child( + Label::new(format!( + "“{value}” contains potentially surprising Unicode characters" + )) + .size(LabelSize::Small) + .color(Color::Error), + ) + .child(v_flex().min_w_0().pl_2().children( + suspicious.iter().map(|character| { + Label::new(format!("• {}", character.description())) + .size(LabelSize::XSmall) + .color(Color::Muted) + .buffer_font(cx) + }), + )) + }, + )), + ) + .child( + IconButton::new("configure-confusable-warning", IconName::Settings) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .tooltip(Tooltip::text("Configure unicode confusables warning")) + .on_click(|_, window, cx| { + window.dispatch_action( + Box::new(zed_actions::OpenSettingsAt { + path: zed_actions::AGENT_SANDBOX_SETTINGS_PATH.to_string(), + target: None, + }), + cx, + ); + }), + ), + ) + .child( + Checkbox::new( + SharedString::from(format!("confusable-ack-{}", tool_call_id.0)), + if acknowledged { + ToggleState::Selected + } else { + ToggleState::Unselected + }, + ) + .label("I understand and wish to proceed") + .label_size(LabelSize::Small) + .on_click(cx.listener({ + let tool_call_id = tool_call_id.clone(); + move |this, state: &ToggleState, _window, cx| { + if *state == ToggleState::Selected { + this.acknowledged_confusable_warnings + .insert(tool_call_id.clone()); + } else { + this.acknowledged_confusable_warnings.remove(&tool_call_id); + } + cx.notify(); + } + })), + ) .into_any_element() } @@ -8833,7 +9093,12 @@ impl ThreadView { .size(LabelSize::Small) .color(Color::Muted), ) - .child(Label::new(details.reason.clone()).size(LabelSize::Small)), + .child(Label::new(details.reason.clone()).size(LabelSize::Small)) + .child(self.render_sandbox_docs_link( + "sandbox-fallback-docs-link", + details.docs_section.as_deref(), + cx, + )), ) .into_any_element() } @@ -8901,6 +9166,9 @@ impl ThreadView { entry_ix: usize, tool_call_id: acp::ToolCallId, focus_handle: &FocusHandle, + // When true, the "allow" choices are disabled (e.g. an unacknowledged + // surprising-Unicode warning is showing). "Deny"/"Retry" stay enabled. + allow_disabled: bool, cx: &Context, ) -> Div { match options { @@ -8911,6 +9179,7 @@ impl ThreadView { entry_ix, tool_call_id, focus_handle, + allow_disabled, cx, ), PermissionOptions::Dropdown(choices) => self.render_permission_buttons_with_dropdown( @@ -8921,6 +9190,7 @@ impl ThreadView { session_id, tool_call_id, focus_handle, + allow_disabled, cx, ), PermissionOptions::DropdownWithPatterns { @@ -8935,6 +9205,7 @@ impl ThreadView { session_id, tool_call_id, focus_handle, + allow_disabled, cx, ), } @@ -8949,6 +9220,7 @@ impl ThreadView { session_id: acp::SessionId, tool_call_id: acp::ToolCallId, focus_handle: &FocusHandle, + allow_disabled: bool, cx: &Context, ) -> Div { let selection = self.permission_selections.get(&tool_call_id); @@ -9003,13 +9275,14 @@ impl ThreadView { .gap_0p5() .child( Button::new(("allow-btn", entry_ix), "Allow") + .disabled(allow_disabled) .start_icon( Icon::new(IconName::Check) .size(IconSize::XSmall) .color(Color::Success), ) .label_size(LabelSize::Small) - .when(is_first, |this| { + .when(is_first && !allow_disabled, |this| { this.key_binding( KeyBinding::for_action_in( &AllowOnce as &dyn Action, @@ -9335,6 +9608,7 @@ impl ThreadView { entry_ix: usize, tool_call_id: acp::ToolCallId, focus_handle: &FocusHandle, + allow_disabled: bool, cx: &Context, ) -> Div { let mut seen_kinds: ArrayVec = ArrayVec::new(); @@ -9398,13 +9672,22 @@ impl ThreadView { } }; - let this = this.start_icon(icon); + // An "allow" choice is disabled while a surprising-Unicode + // warning is unacknowledged; "deny"/"retry" stay enabled. + let is_allow = matches!( + option.kind, + acp::PermissionOptionKind::AllowOnce + | acp::PermissionOptionKind::AllowAlways + ) && !is_retry; + let disabled = allow_disabled && is_allow; + + let this = this.start_icon(icon).disabled(disabled); let Some(action) = action else { return this; }; - if !is_first || seen_kinds.contains(&option.kind) { + if !is_first || disabled || seen_kinds.contains(&option.kind) { return this; } diff --git a/crates/agent_ui/src/unicode_confusables.rs b/crates/agent_ui/src/unicode_confusables.rs new file mode 100644 index 00000000000000..3b1ea11eb21b2c --- /dev/null +++ b/crates/agent_ui/src/unicode_confusables.rs @@ -0,0 +1,244 @@ +//! Detection of "surprising" Unicode characters in the domains and paths shown +//! in sandbox privilege-escalation prompts. +//! +//! Homoglyph/confusable attacks (a Cyrillic `а` standing in for a Latin `a`), +//! invisible characters (zero-width spaces), and bidirectional overrides can +//! make a requested domain or path look like something it is not, tricking the +//! user into granting access to the wrong target. Domains reach the prompt in +//! Punycode (`xn--…`) ASCII form, so a lookalike host is decoded back to +//! Unicode before scanning; paths are scanned as they are displayed. + +use unicode_script::UnicodeScript as _; + +/// Why a character in a domain or path is considered surprising. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SuspiciousKind { + /// A bidirectional control that can visually reorder surrounding text (for + /// example U+202E RIGHT-TO-LEFT OVERRIDE) — the classic "Trojan Source" + /// trick. + BidiControl, + /// A zero-width, invisible, or non-ASCII whitespace formatting character. + Invisible, + /// A visible non-ASCII character that can be confused with ASCII (a + /// homoglyph) or that mixes an unexpected script into otherwise-ASCII text. + Confusable, +} + +/// A single surprising character discovered while scanning a domain or path. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct SuspiciousChar { + pub character: char, + pub kind: SuspiciousKind, +} + +impl SuspiciousChar { + /// A human-readable, one-line description for the approval banner, such as + /// `‘а’ (U+0430 Cyrillic)` or `U+202E right-to-left override`. + pub fn description(&self) -> String { + let codepoint = format!("U+{:04X}", self.character as u32); + match self.kind { + SuspiciousKind::Confusable => { + format!( + "‘{}’ ({codepoint} {})", + self.character, + self.character.script().full_name() + ) + } + // Bidi controls and invisible characters have no meaningful glyph to + // show (and printing them could itself reorder the banner text), so + // we render only the codepoint and a name. + SuspiciousKind::BidiControl | SuspiciousKind::Invisible => { + match well_known_name(self.character) { + Some(name) => format!("{codepoint} {name}"), + None => codepoint, + } + } + } + } +} + +/// Scan a raw string for surprising Unicode characters, returning each distinct +/// offending character once, in order of first appearance. +pub fn scan(text: &str) -> Vec { + let mut result: Vec = Vec::new(); + for character in text.chars() { + if character.is_ascii() { + continue; + } + if result.iter().any(|found| found.character == character) { + continue; + } + result.push(SuspiciousChar { + character, + kind: classify(character), + }); + } + result +} + +/// Scan a host for surprising characters, first decoding any IDN/Punycode +/// (`xn--…`) labels back to Unicode so a lookalike domain that reaches us as +/// ASCII is still caught. Returns the decoded (Unicode) host — which is what the +/// banner shows the user — alongside the findings. When nothing is surprising +/// the returned host equals the input. +pub fn scan_host(host: &str) -> (String, Vec) { + // `domain_to_unicode` never fails destructively: on error it still returns a + // best-effort decoding, which is exactly what we want to scan and show. + let (decoded, _result) = idna::domain_to_unicode(host); + let findings = scan(&decoded); + (decoded, findings) +} + +fn classify(character: char) -> SuspiciousKind { + if is_bidi_control(character) { + SuspiciousKind::BidiControl + } else if is_invisible(character) { + SuspiciousKind::Invisible + } else { + SuspiciousKind::Confusable + } +} + +fn is_bidi_control(character: char) -> bool { + matches!(character, + '\u{061C}' // ARABIC LETTER MARK + | '\u{200E}' // LEFT-TO-RIGHT MARK + | '\u{200F}' // RIGHT-TO-LEFT MARK + | '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO + | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI + ) +} + +fn is_invisible(character: char) -> bool { + matches!(character, + '\u{00AD}' // SOFT HYPHEN + | '\u{180E}' // MONGOLIAN VOWEL SEPARATOR + | '\u{200B}' // ZERO WIDTH SPACE + | '\u{200C}' // ZERO WIDTH NON-JOINER + | '\u{200D}' // ZERO WIDTH JOINER + | '\u{2060}' // WORD JOINER + | '\u{2061}'..='\u{2064}' // invisible math operators + | '\u{FEFF}' // ZERO WIDTH NO-BREAK SPACE (BOM) + ) || is_non_ascii_space(character) + // Any remaining control/format character (categories Cc/Cf) is + // invisible for our purposes. + || character.is_control() +} + +fn is_non_ascii_space(character: char) -> bool { + matches!( + character, + '\u{00A0}' // NO-BREAK SPACE + | '\u{1680}' // OGHAM SPACE MARK + | '\u{2000}' + ..='\u{200A}' // EN QUAD … HAIR SPACE + | '\u{202F}' // NARROW NO-BREAK SPACE + | '\u{205F}' // MEDIUM MATHEMATICAL SPACE + | '\u{3000}' // IDEOGRAPHIC SPACE + ) +} + +/// Friendly names for the invisible/bidi characters most likely to show up in an +/// attack, so the banner reads better than a bare codepoint. +fn well_known_name(character: char) -> Option<&'static str> { + Some(match character { + '\u{00A0}' => "no-break space", + '\u{00AD}' => "soft hyphen", + '\u{061C}' => "arabic letter mark", + '\u{180E}' => "mongolian vowel separator", + '\u{200B}' => "zero-width space", + '\u{200C}' => "zero-width non-joiner", + '\u{200D}' => "zero-width joiner", + '\u{200E}' => "left-to-right mark", + '\u{200F}' => "right-to-left mark", + '\u{202A}' => "left-to-right embedding", + '\u{202B}' => "right-to-left embedding", + '\u{202C}' => "pop directional formatting", + '\u{202D}' => "left-to-right override", + '\u{202E}' => "right-to-left override", + '\u{2060}' => "word joiner", + '\u{2066}' => "left-to-right isolate", + '\u{2067}' => "right-to-left isolate", + '\u{2068}' => "first strong isolate", + '\u{2069}' => "pop directional isolate", + '\u{3000}' => "ideographic space", + '\u{FEFF}' => "zero-width no-break space", + _ => return None, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn plain_ascii_is_never_flagged() { + assert!(scan("github.com").is_empty()); + assert!(scan("/home/user/project/src/main.rs").is_empty()); + assert!(scan("*.npmjs.org").is_empty()); + } + + #[test] + fn detects_cyrillic_homoglyph() { + // "gіthub.com" with a Cyrillic "і" (U+0456). + let findings = scan("g\u{0456}thub.com"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].character, '\u{0456}'); + assert_eq!(findings[0].kind, SuspiciousKind::Confusable); + assert!(findings[0].description().contains("U+0456")); + assert!(findings[0].description().contains("Cyrillic")); + } + + #[test] + fn detects_bidi_override() { + let findings = scan("safe\u{202E}txt.exe"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].kind, SuspiciousKind::BidiControl); + assert_eq!(findings[0].description(), "U+202E right-to-left override"); + } + + #[test] + fn detects_zero_width_space() { + let findings = scan("git\u{200B}hub.com"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].kind, SuspiciousKind::Invisible); + assert_eq!(findings[0].description(), "U+200B zero-width space"); + } + + #[test] + fn deduplicates_repeated_characters() { + // Two Cyrillic "а" (U+0430) should be reported once. + let findings = scan("\u{0430}bc\u{0430}"); + assert_eq!(findings.len(), 1); + } + + #[test] + fn scan_host_decodes_punycode_lookalike() { + // "аpple.com" (leading Cyrillic а, U+0430) encodes to this Punycode. + let (decoded, findings) = scan_host("xn--pple-43d.com"); + assert_eq!(decoded, "\u{0430}pple.com"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].character, '\u{0430}'); + assert_eq!(findings[0].kind, SuspiciousKind::Confusable); + } + + #[test] + fn scan_host_leaves_plain_domains_alone() { + let (decoded, findings) = scan_host("github.com"); + assert_eq!(decoded, "github.com"); + assert!(findings.is_empty()); + } + + #[test] + fn scan_host_handles_wildcard_subdomain_patterns() { + // Host patterns can carry a leading `*.` wildcard; decoding must not + // choke on it, and a lookalike label behind it is still caught. + let (_decoded, findings) = scan_host("*.xn--pple-43d.com"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].character, '\u{0430}'); + + let (decoded, findings) = scan_host("*.github.com"); + assert_eq!(decoded, "*.github.com"); + assert!(findings.is_empty()); + } +} diff --git a/crates/client/src/zed_urls.rs b/crates/client/src/zed_urls.rs index 6eb69b83273fbf..d9cd62552265d4 100644 --- a/crates/client/src/zed_urls.rs +++ b/crates/client/src/zed_urls.rs @@ -69,6 +69,23 @@ pub fn skills_docs(cx: &App) -> String { format!("{docs_url}/ai/skills", docs_url = docs_url(cx)) } +/// Returns the URL to Zed's Agent sandboxing documentation. +/// +/// Pass `section` to deep-link to a specific section anchor on the page (for +/// example, `Some("installing-bubblewrap")`); pass `None` to link to the top of +/// the page. +/// +/// Unlike the account/app links above, this targets `zed.dev/docs` (via +/// [`release_channel::docs_url`]) rather than the configured `server_url`: the +/// docs are a static site hosted on `zed.dev`, so pointing at a local dev +/// `server_url` would 404. +pub fn sandboxing_docs(section: Option<&str>, cx: &App) -> String { + let base = release_channel::docs_url("ai/sandboxing", cx); + match section { + Some(section) => format!("{base}#{section}"), + None => base, + } +} pub fn llm_provider_docs(cx: &App) -> String { format!("{docs_url}/ai/llm-providers", docs_url = docs_url(cx)) } diff --git a/crates/feature_flags/src/flags.rs b/crates/feature_flags/src/flags.rs index 31096cc38bcd3b..f9db079a4917e4 100644 --- a/crates/feature_flags/src/flags.rs +++ b/crates/feature_flags/src/flags.rs @@ -118,14 +118,3 @@ impl FeatureFlag for AutoWatchFeatureFlag { type Value = PresenceFlag; } register_feature_flag!(AutoWatchFeatureFlag); - -/// Wraps agent-run terminal commands in an OS-level sandbox where supported -/// (currently macOS Seatbelt only). When off, terminal commands run with the -/// agent's full ambient permissions, as they always have. -pub struct SandboxingFeatureFlag; - -impl FeatureFlag for SandboxingFeatureFlag { - const NAME: &'static str = "sandboxing"; - type Value = PresenceFlag; -} -register_feature_flag!(SandboxingFeatureFlag); diff --git a/crates/sandbox/Cargo.toml b/crates/sandbox/Cargo.toml index c9211dfa99f57d..58951c163177c0 100644 --- a/crates/sandbox/Cargo.toml +++ b/crates/sandbox/Cargo.toml @@ -58,6 +58,10 @@ libc.workspace = true # Safe wrappers for the SCM_RIGHTS fd-passing and `fstat` the bind validator # needs, so that code doesn't hand-roll `msghdr`/`CMSG_*`/`mem::zeroed` unsafe. nix = { workspace = true, features = ["fs", "socket", "uio"] } +# Builds the in-sandbox seccomp-BPF filter that blocks the untrusted command from +# creating `AF_UNIX` (and other non-IP) sockets, `io_uring`, `ptrace`, etc. — the +# syscall-level half of preventing session-IPC-socket sandbox escapes. +seccompiler.workspace = true [target.'cfg(target_os = "linux")'.dev-dependencies] tempfile.workspace = true diff --git a/crates/sandbox/README.md b/crates/sandbox/README.md index ab23ecad891a83..d2b24249737343 100644 --- a/crates/sandbox/README.md +++ b/crates/sandbox/README.md @@ -228,6 +228,47 @@ If the attacker managed to change a path to point to a different inode to when the FD was captured, the check will fail, and we don't run the untrusted command. +#### Blocking IPC-socket escapes (seccomp) + +A read-only bind mount does **not** stop a process from `connect()`-ing to a +Unix-domain socket: the kernel deliberately exempts sockets (and FIFOs, and +device nodes) from the read-only-filesystem write check, because connecting +modifies no filesystem data. So even with `--ro-bind / /`, a sandboxed command +could reach a session IPC socket in `$XDG_RUNTIME_DIR` (a Wayland compositor, +the D-Bus session bus, ...) or a system socket like the Docker daemon, and use +it to run a process *outside* the sandbox — defeating both the filesystem and +network restrictions, regardless of the read/write grant. `--unshare-net` does +not help: it isolates abstract sockets and TCP/IP, but these are pathname +sockets on the bound filesystem. + +The fix is a seccomp-BPF filter (built with `seccompiler`) installed on the +untrusted command just before it runs. Rather than trying to hide every socket, +it stops the command from *obtaining* one it could escape through: + +- `socket()` is allowed only for `AF_INET`/`AF_INET6`/`AF_NETLINK`; every other + family — notably `AF_UNIX` (session IPC) and `AF_VSOCK` (the VM host) — is + denied with `EPERM`. +- `socketpair()` is allowed only for `AF_UNIX` (a process-local pair that can't + reach anything outside the sandbox). +- `io_uring_*` is denied, so its ring operations can't create/connect a socket + without going through the filtered syscalls; `ptrace`/`process_vm_*` are denied. +- `connect`/`recvmsg`/`sendmsg`/`bind`/`listen`/`accept` stay allowed. With no + way to create a forbidden socket — and, by fd hygiene, none inherited — there + is nothing dangerous for them to act on, and blocking `connect` would break + legitimate loopback/proxy use. `seccompiler`'s architecture check kills + foreign-arch syscalls, closing the 32-bit (`socketcall`) bypass. + +The filter must apply to the command but **not** to the launcher/bridge process, +which keeps using `AF_UNIX` to reach the host proxy for every request. So it is +installed inline right before `exec` in the direct case, and via the child's +`pre_exec` in the restricted-network bridge case. Because the filter lives in the +in-sandbox launcher, the launcher is now **always** run (even when there are no +writable binds to validate and no bridge), so the filter is always installed. + +This is Linux/WSL-specific. On macOS, Seatbelt gates Unix-socket `connect` as a +separate `network-outbound` capability that is denied by default, so the same +escape is already closed there without a seccomp filter. + ### Windows > [!NOTE] The Windows implementation depends heavily on the details of the Linux @@ -241,6 +282,27 @@ To work around this, we launch `zed --wsl-sandbox-helper` in WSL, which is a shim that captures the FDs and sets up the socket. We download this to `~/.local/libexec/zed`, so that it does not conflict with the Windows `zed.exe` binary that WSL will inject into the Linux `$PATH` (yes the `.exe` is stripped). + +### MacOS + +MacOS uses seatbelt, which enforces a rules file. This generally makes +enforcement more straightforward. Unlike Linux, paths are resolved and checked +at *syscall time*, meaning the symlink swap attack will not succeed. + +However, care has to be taken with various parts of the rules file, specifically +when it comes to `mach-lookup`. This controls access to, among other things, +Launch Services, which allows unsandboxed code execution. + +The exact policy is defined in `src/macos_seatbelt.rs`, and is inspired by a +mixture of Codex and Chromium's rules. + +Some of the denied services are somewhat questionable (i.e. +`com.apple.FontObjectsServer`) - there are legitimate uses for an application to +use this, but on the other hand, fonts can contain executable code, and have +historically been exploited to achieve RCE. Given that, in the Zed agent, it is +easy to opt-out of the sandbox, denying seems like a good choice. But we may +want to revisit this. + ## Code design ### `HostFilesystemLocation` diff --git a/crates/sandbox/src/bwrap_test_helper.rs b/crates/sandbox/src/bwrap_test_helper.rs index f6aa0cb6f29377..68ef38659b2ed1 100644 --- a/crates/sandbox/src/bwrap_test_helper.rs +++ b/crates/sandbox/src/bwrap_test_helper.rs @@ -46,6 +46,12 @@ mod imp { /// successful round-trip, non-zero otherwise. Run *inside* the sandbox. const SUBCOMMAND_ECHO_CHECK: &str = "__echo_check"; + /// Internal subcommand: connect to the unix-domain socket at the given path + /// and round-trip a byte through it. Exits 0 on a successful round-trip, + /// non-zero on any failure (including `socket(AF_UNIX)` being blocked once + /// the seccomp guard lands). Run *inside* the sandbox. + const SUBCOMMAND_UNIX_CONNECT_CHECK: &str = "__unix_connect_check"; + /// Default port for echo targets given as a bare hostname (e.g. `echo1`). const DEFAULT_ECHO_PORT: &str = "7000"; @@ -57,6 +63,9 @@ mod imp { let args: Vec = std::env::args().collect(); let result = match args.get(1).map(String::as_str) { Some(SUBCOMMAND_ECHO_CHECK) => run_echo_check(args.get(2).map(String::as_str)), + Some(SUBCOMMAND_UNIX_CONNECT_CHECK) => { + run_unix_connect_check(args.get(2).map(String::as_str)) + } _ => run_checks(), }; @@ -93,8 +102,8 @@ mod imp { /// One declarative check: a sandbox policy, an operation, and the expected /// result. Deserialized from the JSON the Nix test produces. /// - /// Exactly one operation field (`read`, `write`, `network`, or `canCreate`) - /// must be set. Policy fields default to the most-confined policy + /// Exactly one operation field (`read`, `write`, `network`, `socketPath`, or + /// `canCreate`) must be set. Policy fields default to the most-confined policy /// (restricted filesystem with no writable paths, blocked network). #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] @@ -127,6 +136,9 @@ mod imp { /// the sandbox. #[serde(default)] network: Option, + /// Connect to this unix-domain socket path from inside the sandbox. + #[serde(default)] + socket_path: Option, /// Assert that `Sandbox::can_create` for this policy matches the value: /// `true` => the sandbox can be created, `false` => it cannot. #[serde(default)] @@ -239,6 +251,8 @@ mod imp { format!("write {path}") } else if let Some(host) = &check.network { format!("network {host}") + } else if let Some(path) = &check.socket_path { + format!("socket_connect {path}") } else if let Some(expected) = check.can_create { format!("can_create == {expected}") } else { @@ -277,6 +291,8 @@ mod imp { run_write(check, path)? } else if let Some(host) = &check.network { run_network(check, host, echo_port)? + } else if let Some(path) = &check.socket_path { + run_socket_connect(check, path)? } else { bail!("check {label:?} has no operation"); }; @@ -354,6 +370,21 @@ mod imp { run_command(&mut sandbox, &exe, &[SUBCOMMAND_ECHO_CHECK, &target]) } + /// Attempt to connect to the unix-domain socket at `path` from inside the + /// sandbox via the `__unix_connect_check` subcommand, returning whether the + /// round-trip succeeded. A read-only bind mount of `/` leaves the socket + /// reachable, so a sandboxed command can currently `connect()` to a session + /// IPC socket owned by a process *outside* the sandbox — the escape a + /// `socket(AF_UNIX)` seccomp filter is meant to block. When that guard lands, + /// `socket(AF_UNIX)` returns `EPERM`, the subcommand fails, and this returns + /// `false`. + fn run_socket_connect(check: &Check, path: &str) -> Result { + let exe = current_exe_str()?; + let policy = policy_of(check)?; + let mut sandbox = Sandbox::new(policy).map_err(sandbox_err)?; + run_command(&mut sandbox, &exe, &[SUBCOMMAND_UNIX_CONNECT_CHECK, path]) + } + fn error_matches(error: &SandboxError, expected: &str) -> bool { matches!( (error, expected), @@ -429,6 +460,35 @@ mod imp { } } + /// Inner command: connect to the unix-domain socket at `path` and round-trip + /// a byte through it. + /// + /// Any failure — `socket(AF_UNIX)` being denied (how the seccomp guard will + /// manifest, as `EPERM`), `connect()` failing, or a bad round-trip — exits + /// non-zero, so the caller reads it as "not connected". A clean round-trip + /// (exit 0) means the socket outside the sandbox was reachable. + fn run_unix_connect_check(path: Option<&str>) -> Result<()> { + use std::os::unix::net::UnixStream; + + let path = path.context("unix connect check requires a socket path argument")?; + let mut stream = UnixStream::connect(path) + .with_context(|| format!("failed to connect to unix socket {path}"))?; + stream.set_read_timeout(Some(Duration::from_secs(10)))?; + stream + .write_all(b"ping\n") + .context("failed to write to unix socket")?; + let mut buffer = [0u8; 32]; + let read = stream + .read(&mut buffer) + .context("failed to read from unix socket")?; + let echoed = String::from_utf8_lossy(&buffer[..read]); + if echoed.contains("ping") { + Ok(()) + } else { + bail!("unix socket returned unexpected data: {echoed:?}"); + } + } + /// Read an HTTP status line (up to the first CRLF), then drain the rest of /// the header block (up to the blank line) so the stream is positioned at /// the tunneled body. diff --git a/crates/sandbox/src/linux_bubblewrap.rs b/crates/sandbox/src/linux_bubblewrap.rs index 21a4602523bffe..373f3520786071 100644 --- a/crates/sandbox/src/linux_bubblewrap.rs +++ b/crates/sandbox/src/linux_bubblewrap.rs @@ -610,15 +610,20 @@ pub fn wrap_invocation( // Create the requested writable directories up front, with the agent's // ambient permissions, so each can be bind-mounted at its exact path (see - // `build_bwrap_args`). Without this a not-yet-existing writable path could - // not be bound, and the command could not create it either (its parent is - // read-only inside the sandbox). Best-effort: a directory we can't create is - // left unbound rather than widening the sandbox to an existing ancestor. + // `build_bwrap_args`): `bwrap` can't bind a nonexistent source, and the + // command can't create it either (its parent is read-only inside the + // sandbox). If a path still doesn't exist afterwards we can't grant the + // write access the agent asked for, and running anyway would give the + // command silently less access than it believes it has — so fail closed with + // a clear error instead. (An existing *file* makes `create_dir_all` error + // but is fine: it exists and the `--bind` below handles it.) if !permissions.allow_fs_write { for directory in writable_dirs { - if let Err(error) = std::fs::create_dir_all(directory) { - log::warn!( - "[sandbox] could not create writable directory {}: {error}", + if let Err(error) = std::fs::create_dir_all(directory) + && !directory.exists() + { + bail!( + "failed to provide writable sandbox path {}: {error}", directory.display() ); } @@ -652,41 +657,41 @@ pub fn wrap_invocation( NetworkAccess::None | NetworkAccess::All => None, }; - // The launcher is only needed when there is something for it to do: validate - // writable binds, and/or run the restricted-network bridge. Otherwise the - // command runs directly under bwrap. - if validation_socket.is_some() || bridge.is_some() { - bwrap_args.push(bridge_program.to_string()); - bwrap_args.push(LAUNCHER_FLAG.to_string()); - // Field 1: validation socket (in-sandbox path) or sentinel. - bwrap_args.push(match validation_socket { - Some(socket) => socket.sandbox_socket_path.to_string_lossy().into_owned(), - None => LAUNCHER_NONE.to_string(), - }); - // Fields 2-3: bridge socket (in-sandbox path) + port, or sentinels. - match &bridge { - Some((socket, port)) => { - bwrap_args.push(socket.to_string_lossy().into_owned()); - bwrap_args.push(port.to_string()); - } - None => { - bwrap_args.push(LAUNCHER_NONE.to_string()); - bwrap_args.push(LAUNCHER_NONE.to_string()); - } + // Always route through the in-sandbox launcher, even when there are no + // writable binds to validate and no restricted-network bridge: the launcher + // is where the seccomp filter is installed on the untrusted command (see + // `exec_command` / `run_bridge`). Absent fields are passed as the `-` + // sentinel, and `run_launcher` then just installs the filter and `exec`s. + bwrap_args.push(bridge_program.to_string()); + bwrap_args.push(LAUNCHER_FLAG.to_string()); + // Field 1: validation socket (in-sandbox path) or sentinel. + bwrap_args.push(match validation_socket { + Some(socket) => socket.sandbox_socket_path.to_string_lossy().into_owned(), + None => LAUNCHER_NONE.to_string(), + }); + // Fields 2-3: bridge socket (in-sandbox path) + port, or sentinels. + match &bridge { + Some((socket, port)) => { + bwrap_args.push(socket.to_string_lossy().into_owned()); + bwrap_args.push(port.to_string()); } - // Field 4: the writable bind-destination paths to validate (count, then - // the paths), in the same order the host sends their fds. - let validation_paths: &[&Path] = if validation_socket.is_some() { - writable_dirs - } else { - &[] - }; - bwrap_args.push(validation_paths.len().to_string()); - for path in validation_paths { - bwrap_args.push(path.to_string_lossy().into_owned()); + None => { + bwrap_args.push(LAUNCHER_NONE.to_string()); + bwrap_args.push(LAUNCHER_NONE.to_string()); } - bwrap_args.push("--".to_string()); } + // Field 4: the writable bind-destination paths to validate (count, then + // the paths), in the same order the host sends their fds. + let validation_paths: &[&Path] = if validation_socket.is_some() { + writable_dirs + } else { + &[] + }; + bwrap_args.push(validation_paths.len().to_string()); + for path in validation_paths { + bwrap_args.push(path.to_string_lossy().into_owned()); + } + bwrap_args.push("--".to_string()); bwrap_args.push(program.to_string()); bwrap_args.extend(args.iter().cloned()); @@ -859,9 +864,126 @@ fn validate_binds(socket_path: &Path, paths: &[PathBuf]) -> Result<()> { Ok(()) } +/// Build the seccomp-BPF program installed on the untrusted command before it +/// runs. This is the syscall-level half of preventing session-IPC-socket +/// sandbox escapes: a read-only bind mount does not stop `connect()` to a unix +/// socket, so instead we stop the command from ever *obtaining* a non-IP socket. +/// +/// The program (default action: allow): +/// - `socket()` is denied (`EPERM`) unless the family is `AF_INET`/`AF_INET6`/ +/// `AF_NETLINK` — so no `AF_UNIX` (session IPC) or `AF_VSOCK` (VM host) sockets. +/// - `socketpair()` is allowed only for `AF_UNIX` (a process-local pair that +/// cannot reach anything outside the sandbox). +/// - `io_uring_*` is denied, so its ring ops can't create/connect sockets +/// without going through the filtered syscalls. +/// - `ptrace`/`process_vm_*` are denied. +/// +/// `connect`/`recvmsg`/`sendmsg`/`bind`/`listen`/`accept` stay allowed: with no +/// way to create a forbidden socket (and — by fd hygiene — no forbidden fd +/// inherited), there is nothing dangerous for them to act on, and blocking +/// `connect` would break legitimate loopback/proxy use. Foreign-architecture +/// syscalls are killed by seccompiler's arch check, closing the 32-bit-ABI +/// (`socketcall`) bypass. +/// +/// Returns `None` on an architecture seccompiler can't target (we ship on +/// x86_64/aarch64, so that is not a configuration we run in practice). +fn build_command_seccomp_program() -> Result> { + use seccompiler::{ + BpfProgram, SeccompAction, SeccompCmpArgLen, SeccompCmpOp, SeccompCondition, SeccompFilter, + SeccompRule, TargetArch, + }; + use std::collections::BTreeMap; + + let Ok(target_arch) = TargetArch::try_from(std::env::consts::ARCH) else { + return Ok(None); + }; + + // `socket(domain, ...)`: deny unless `domain` (arg0, an `int` — compare the + // low 32 bits) is an allowed IP/netlink family. The rule matches when the + // family is none of the allowed ones, and a matched rule takes the deny + // action; an allowed family matches no rule and falls through to `Allow`. + let socket_deny = SeccompRule::new(vec![ + SeccompCondition::new( + 0, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Ne, + libc::AF_INET as u64, + )?, + SeccompCondition::new( + 0, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Ne, + libc::AF_INET6 as u64, + )?, + SeccompCondition::new( + 0, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Ne, + libc::AF_NETLINK as u64, + )?, + ])?; + // `socketpair(domain, ...)`: allow only `AF_UNIX`. + let socketpair_deny = SeccompRule::new(vec![SeccompCondition::new( + 0, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Ne, + libc::AF_UNIX as u64, + )?])?; + + let mut rules: BTreeMap> = BTreeMap::new(); + rules.insert(libc::SYS_socket, vec![socket_deny]); + rules.insert(libc::SYS_socketpair, vec![socketpair_deny]); + // Unconditional denials (an empty rule chain always takes the match action). + for syscall in [ + libc::SYS_io_uring_setup, + libc::SYS_io_uring_enter, + libc::SYS_io_uring_register, + libc::SYS_ptrace, + libc::SYS_process_vm_readv, + libc::SYS_process_vm_writev, + ] { + rules.insert(syscall, Vec::new()); + } + + let filter = SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Errno(libc::EPERM as u32), + target_arch, + ) + .context("building sandbox command seccomp filter")?; + let program = + BpfProgram::try_from(filter).context("compiling sandbox command seccomp filter")?; + Ok(Some(program)) +} + +/// Install [`build_command_seccomp_program`] on the calling thread, which is +/// about to become (or `exec` into) the untrusted command; the filter survives +/// `exec`. `apply_filter` also sets `PR_SET_NO_NEW_PRIVS`. On an unsupported +/// architecture there is no filter to install — log and proceed rather than +/// break the sandbox on a platform we don't ship. +fn install_command_seccomp_filter() -> Result<()> { + match build_command_seccomp_program()? { + Some(program) => seccompiler::apply_filter(&program) + .context("installing sandbox command seccomp filter")?, + None => log::warn!( + "[sandbox] seccomp is unavailable on {}; the unix-socket syscall guard \ + was not installed", + std::env::consts::ARCH + ), + } + Ok(()) +} + /// Replace this process with the sandboxed command. Only returns (after logging) /// if `exec` itself fails. fn exec_command(program: &OsStr, args: &[OsString]) -> ! { + // Lock down socket/io_uring/ptrace syscalls right before handing control to + // the untrusted command; the filter survives `exec`. + if let Err(error) = install_command_seccomp_filter() { + eprintln!("zed: failed to install sandbox seccomp filter: {error:#}"); + std::process::exit(SANDBOX_SETUP_FAILED_EXIT_CODE); + } let error = Command::new(program).args(args).exec(); eprintln!("zed: failed to exec sandboxed command: {error}"); std::process::exit(SANDBOX_SETUP_FAILED_EXIT_CODE); @@ -889,7 +1011,32 @@ fn run_bridge(socket_path: PathBuf, port: u16, program: &OsStr, program_args: &[ std::process::exit(SANDBOX_SETUP_FAILED_EXIT_CODE); } - let mut child = match Command::new(program).args(program_args).spawn() { + // The command runs under the syscall filter, installed in the child via + // `pre_exec` — *this* bridge process must NOT be filtered, since it keeps + // using `AF_UNIX` to reach the host proxy for every request the command + // makes. Build the program before the fork; the child only applies it. + let seccomp_program = match build_command_seccomp_program() { + Ok(program) => program, + Err(error) => { + eprintln!("zed: failed to build sandbox seccomp filter: {error:#}"); + std::process::exit(SANDBOX_SETUP_FAILED_EXIT_CODE); + } + }; + let mut command = Command::new(program); + command.args(program_args); + // SAFETY: the closure runs in the forked child after `fork` and before + // `exec`. It only calls `seccompiler::apply_filter` (a `prctl` on a program + // built before the fork) — async-signal-safe and allocation-free. + unsafe { + command.pre_exec(move || { + if let Some(program) = &seccomp_program { + seccompiler::apply_filter(program) + .map_err(|error| std::io::Error::other(format!("seccomp: {error}")))?; + } + Ok(()) + }); + } + let mut child = match command.spawn() { Ok(child) => child, Err(error) => { eprintln!("zed: failed to spawn sandboxed command: {error}"); @@ -1134,29 +1281,42 @@ fn run_wsl_helper(invocation: WslHelperInvocation) -> ! { }; let mut args = invocation.base_args.clone(); + // Always re-exec ourselves as the in-sandbox launcher, even when there is + // nothing to validate: the launcher is where the seccomp filter is installed + // on the untrusted command (see `exec_command`). When there are writable + // binds, also bind the validation socket so the launcher can verify them. + // WSL has no restricted-network bridge, so both bridge fields are the absent + // sentinel. if let Some(sender) = &validation { - // Bind the validation socket in (after the base args' tmpfs and writable - // binds so it isn't shadowed), then re-exec ourselves inside the sandbox - // as the validator before the real command. WSL has no restricted-network - // bridge, so both bridge fields are the absent sentinel. + // Bind the validation socket in, after the base args' tmpfs and writable + // binds so it isn't shadowed. args.push(OsString::from("--bind")); args.push(sender.host_socket_path().as_os_str().to_os_string()); args.push(sender.sandbox_socket_path().as_os_str().to_os_string()); - args.push(OsString::from("--")); - args.push(current_exe.into_os_string()); - args.push(OsString::from(LAUNCHER_FLAG)); - args.push(sender.sandbox_socket_path().as_os_str().to_os_string()); - args.push(OsString::from(LAUNCHER_NONE)); - args.push(OsString::from(LAUNCHER_NONE)); - args.push(OsString::from(invocation.writable_paths.len().to_string())); - for path in &invocation.writable_paths { - args.push(path.clone().into_os_string()); - } - args.push(OsString::from("--")); + } + args.push(OsString::from("--")); + args.push(current_exe.into_os_string()); + args.push(OsString::from(LAUNCHER_FLAG)); + // Field 1: validation socket (in-sandbox path) or sentinel. + match &validation { + Some(sender) => args.push(sender.sandbox_socket_path().as_os_str().to_os_string()), + None => args.push(OsString::from(LAUNCHER_NONE)), + } + // Fields 2-3: bridge socket + port (WSL has no bridge). + args.push(OsString::from(LAUNCHER_NONE)); + args.push(OsString::from(LAUNCHER_NONE)); + // Field 4: writable bind-destination paths to validate (count, then paths); + // empty when there is nothing to validate. + let validation_paths: &[PathBuf] = if validation.is_some() { + &invocation.writable_paths } else { - // Nothing to validate — run the command directly under bwrap. - args.push(OsString::from("--")); + &[] + }; + args.push(OsString::from(validation_paths.len().to_string())); + for path in validation_paths { + args.push(path.clone().into_os_string()); } + args.push(OsString::from("--")); args.push(invocation.program.clone()); args.extend(invocation.args.iter().cloned()); @@ -1664,4 +1824,100 @@ mod tests { "unexpected error: {error:#}" ); } + + // The filter compiles to a non-empty BPF program on architectures + // seccompiler can target (the ones we ship). On others it's `None`, which is + // acceptable — no filter is installed there. + #[test] + fn test_command_seccomp_program_builds() { + let program = build_command_seccomp_program().expect("build seccomp program"); + if let Some(program) = program { + assert!(!program.is_empty(), "seccomp program must not be empty"); + } + } + + // Actually enforce the filter: in a child process, apply it and confirm that + // `socket(AF_UNIX)` is denied while `socket(AF_INET)` and + // `socketpair(AF_UNIX)` still work — the exact guarantee that closes the + // unix-socket sandbox escape. + #[test] + fn test_command_seccomp_filter_blocks_unix_but_allows_ip() { + // Build in the parent (this allocates); the child only applies the + // prebuilt program and makes raw syscalls. + let Some(program) = build_command_seccomp_program().expect("build seccomp program") else { + return; // unsupported arch: nothing to enforce + }; + + // SAFETY: after `fork`, the child calls only async-signal-safe + // libc/`prctl` (via `apply_filter` on a program built before the fork) + // and `_exit`; it never returns to Rust or allocates. + let pid = unsafe { libc::fork() }; + assert!(pid >= 0, "fork failed"); + if pid == 0 { + if seccompiler::apply_filter(&program).is_err() { + unsafe { libc::_exit(10) }; + } + // AF_UNIX socket creation must be denied. + if unsafe { libc::socket(libc::AF_UNIX, libc::SOCK_STREAM, 0) } >= 0 { + unsafe { libc::_exit(11) }; + } + // AF_INET socket creation must still work. + let inet = unsafe { libc::socket(libc::AF_INET, libc::SOCK_STREAM, 0) }; + if inet < 0 { + unsafe { libc::_exit(12) }; + } + // AF_UNIX socketpair (process-local) must still work. + let mut fds = [0i32; 2]; + if unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) } + != 0 + { + unsafe { libc::_exit(13) }; + } + unsafe { libc::_exit(0) }; + } + + let mut status = 0i32; + let waited = unsafe { libc::waitpid(pid, &mut status, 0) }; + assert_eq!(waited, pid, "waitpid failed"); + assert!( + libc::WIFEXITED(status), + "child did not exit normally: {status:#x}" + ); + let code = libc::WEXITSTATUS(status); + assert_eq!( + code, 0, + "child reported a seccomp mismatch (exit {code}): 10=apply failed, \ + 11=AF_UNIX allowed, 12=AF_INET blocked, 13=socketpair blocked" + ); + } + + // A requested writable path that can't be created (here, under an existing + // file, so `create_dir_all` errors and the path never exists) must fail the + // whole invocation — not run the command with silently less write access + // than the agent asked for. This check runs before `resolve_bwrap`, so the + // test needs no real `bwrap`. + #[test] + fn test_wrap_invocation_fails_when_writable_path_cannot_be_provided() { + let file = tempfile::NamedTempFile::new().unwrap(); + let unbindable = file.path().join("subdir"); + let result = wrap_invocation( + "/proc/self/exe", + SandboxPermissions { + network: NetworkAccess::None, + allow_fs_write: false, + }, + &[unbindable.as_path()], + &[], + None, + "/bin/true", + &[], + None, + None, + ); + let error = result.expect_err("must fail closed when a writable path can't be provided"); + assert!( + error.to_string().contains("writable sandbox path"), + "unexpected error: {error:#}" + ); + } } diff --git a/crates/sandbox/src/macos_seatbelt.rs b/crates/sandbox/src/macos_seatbelt.rs index 17a2db9928ed5c..a62803a84f59ec 100644 --- a/crates/sandbox/src/macos_seatbelt.rs +++ b/crates/sandbox/src/macos_seatbelt.rs @@ -243,8 +243,44 @@ fn generate_seatbelt_config( ; Allow sysctl reads (needed for many system calls) (allow sysctl-read) -; Allow mach lookups (needed for IPC) -(allow mach-lookup) +; Mach service lookups. This is an ALLOWLIST, not a blanket `(allow mach-lookup)`. +; An unrestricted mach-lookup lets a sandboxed command reach LaunchServices / +; launchd and have a process spawned *outside* the sandbox (e.g. `open -a +; Terminal`, or opening a crafted `.app`) — the launched process does not inherit +; this profile, so that is a full sandbox escape. So we allow only the services +; ordinary dev tooling needs and deliberately EXCLUDE the LaunchServices/launchd +; endpoints (closing that escape), the pasteboard (silent clipboard theft), and +; audio (mic/privacy). Curated from Codex's and Chromium's Seatbelt policies; add +; an entry here (with a comment) if a legitimate toolchain needs another service. +; A non-existent name is simply never matched, so erring toward including +; plausible infrastructure services is safe. +(allow mach-lookup + ; identity: user & group resolution (getpwuid, id, whoami, perm checks) + (global-name "com.apple.system.opendirectoryd.libinfo") + (global-name "com.apple.system.opendirectoryd.membership") + (global-name "com.apple.system.DirectoryService.libinfo_v1") + ; per-user temp/cache dir resolution ($TMPDIR, /var/folders/...) + (global-name "com.apple.bsd.dirhelper") + ; CFPreferences (pervasive in Apple frameworks linked by dev tools). + ; Chromium denies cfprefsd.daemon to force in-process prefs; we follow Codex + ; and allow it since dev commands legitimately use many prefs domains — it's + ; a prefs read/write, not an escape. + (global-name "com.apple.cfprefsd.daemon") + (global-name "com.apple.cfprefsd.agent") + (local-name "com.apple.cfprefsd.agent") + ; logging / diagnostics (os_log, ASL, Darwin notifications) + (global-name "com.apple.logd") + (global-name "com.apple.logd.events") + (global-name "com.apple.system.logger") + (global-name "com.apple.diagnosticd") + (global-name "com.apple.system.notification_center") + ; Apple telemetry (data goes to Apple only; harmless, avoids init latency) + (global-name "com.apple.analyticsd") + (global-name "com.apple.analyticsd.messagetracer") + ; power assertions (caffeinate / prevent idle sleep during long builds) + (global-name "com.apple.PowerManagement.control") + ; developer-tools automation-mode flag (our workload is dev tooling) + (global-name "com.apple.dt.automationmode.reader")) ; Allow pseudo-terminal operations (allow pseudo-tty) @@ -345,6 +381,33 @@ fn generate_seatbelt_config( } } + // When outbound network is permitted at all, tools that do their own DNS + // resolution, TLS trust evaluation, and network-configuration lookups need a + // few more Mach services. Kept out of the base allowlist so a no-network + // command can't reach them. Still an allowlist (mirrors Codex's Seatbelt + // network policy) that excludes LaunchServices/launchd. + if !matches!(permissions.network, NetworkAccess::None) { + config.push_str( + r#" +; Extra Mach services for DNS / TLS-trust / network configuration, needed only +; when outbound network is permitted. Still an allowlist that excludes +; LaunchServices/launchd. If hostname resolution fails, add +; `com.apple.mDNSResponder`; if offline code-signature verification of loaded +; dylibs/plugins fails, move the trust services into the base block above. +(allow mach-lookup + ; network / DNS configuration + (global-name "com.apple.SystemConfiguration.configd") + (global-name "com.apple.SystemConfiguration.DNSConfiguration") + (global-name "com.apple.networkd") + ; TLS certificate trust / keychain / revocation + (global-name "com.apple.SecurityServer") + (global-name "com.apple.trustd") + (global-name "com.apple.trustd.agent") + (global-name "com.apple.ocspd")) +"#, + ); + } + if !allowed_unix_socket_paths.is_empty() { config.push_str( r#" @@ -396,6 +459,19 @@ mod tests { use super::*; use std::path::PathBuf; + /// Strip SBPL comment lines (`;`-prefixed) from a generated Seatbelt profile + /// so assertions match on the actual rules rather than on documentation. + /// Several comments legitimately mention rule syntax (for example the + /// blanket `(allow mach-lookup)` form they explain we avoid), which would + /// otherwise cause a naive substring check to spuriously match. + fn seatbelt_rules_only(config: &str) -> String { + config + .lines() + .filter(|line| !line.trim_start().starts_with(';')) + .collect::>() + .join("\n") + } + #[test] fn test_generate_seatbelt_config_contains_read_and_project_write_permissions_by_default() { let dir = PathBuf::from("/Users/test/projects/myproject"); @@ -443,6 +519,57 @@ mod tests { assert!(config.contains("^/dev/ttys[0-9]+")); } + #[test] + fn test_generate_seatbelt_config_scopes_mach_lookup_and_excludes_escape_services() { + let dir = PathBuf::from("/Users/test/projects/myproject"); + let config = + generate_seatbelt_config(&[dir.as_path()], &[], &[], SandboxPermissions::default()) + .unwrap(); + // Assert on the rules only: the mach-lookup comment intentionally spells + // out the blanket `(allow mach-lookup)` form it avoids, which a raw + // `config.contains` would match. + let rules = seatbelt_rules_only(&config); + + // A scoped allowlist, never the blanket form — a blanket `(allow + // mach-lookup)` would let a command reach LaunchServices/launchd and + // escape the sandbox via `open`. + assert!(rules.contains("(allow mach-lookup")); + assert!(!rules.contains("(allow mach-lookup)")); + assert!(rules.contains("com.apple.cfprefsd.daemon")); + + // The escape/abuse endpoints must fall through to `(deny default)`. + assert!(!rules.contains("launchservicesd")); + assert!(!rules.contains("com.apple.lsd")); + assert!(!rules.contains("com.apple.pasteboard")); + + // Network-only services must not be granted without network. + assert!(!rules.contains("com.apple.SecurityServer")); + assert!(!rules.contains("com.apple.SystemConfiguration.configd")); + } + + #[test] + fn test_generate_seatbelt_config_adds_network_mach_services_when_network_allowed() { + let dir = PathBuf::from("/Users/test/projects/myproject"); + let config = generate_seatbelt_config( + &[dir.as_path()], + &[], + &[], + SandboxPermissions { + network: NetworkAccess::All, + allow_fs_write: false, + }, + ) + .unwrap(); + + // DNS / TLS / network-config services appear only when network is allowed. + assert!(config.contains("com.apple.SystemConfiguration.configd")); + assert!(config.contains("com.apple.SecurityServer")); + assert!(config.contains("com.apple.trustd")); + assert!(config.contains("com.apple.trustd.agent")); + // ...but never the escape endpoints. + assert!(!config.contains("launchservicesd")); + } + #[test] fn test_generate_seatbelt_config_allows_unix_socket_paths_without_network() { let dir = PathBuf::from("/Users/test/projects/myproject"); diff --git a/crates/sandbox/src/sandbox.rs b/crates/sandbox/src/sandbox.rs index 067a4c50c1a5fd..466ced18c05eb2 100644 --- a/crates/sandbox/src/sandbox.rs +++ b/crates/sandbox/src/sandbox.rs @@ -1487,6 +1487,184 @@ mod tests { } } +/// A directory that is about to be granted as a sandbox write path but may not +/// exist yet. Preparing one resolves the platform difference in how a +/// not-yet-existing write grant is materialized, while keeping the same security +/// property: the caller shows [`Self::canonical_path`] to the user and records +/// *that* as the grant, so approval is always against the real, symlink-resolved +/// target. +/// +/// - **Linux/WSL**: bubblewrap can only bind an existing inode, so the missing +/// directory (and any missing parents) is created **eagerly**, per component, +/// and the leaf's inode is pinned to read back its canonical path. If the +/// grant is denied, [`Self::discard`] removes exactly the directories that were +/// created, deepest-first, following no symlinks (`rmdir` only removes empty +/// dirs and never traverses a swapped-in symlink). +/// - **macOS**: Seatbelt resolves paths at syscall time and can grant a missing +/// path, so nothing is created here; the directory is materialized only after +/// approval via [`Self::finalize`]. +/// +/// The eventual bind is still protected by the usual capture-and-revalidate path +/// (`HostFilesystemLocation`), which re-pins the inode when the command runs. +pub struct GrantableWriteDir { + canonical_path: PathBuf, + /// Directories created eagerly to pin the inode, shallowest-first. Empty on + /// platforms that defer creation to [`Self::finalize`]. + eagerly_created: Vec, +} + +impl GrantableWriteDir { + /// Prepare `path` for use as a sandbox write grant. `path` must be absolute. + pub fn prepare(path: &Path) -> std::io::Result { + #[cfg(target_os = "linux")] + { + let mut eagerly_created = Vec::new(); + if let Err(error) = create_missing_dirs(path, &mut eagerly_created) { + // Roll back any partial creation so a failure leaves no litter. + for dir in eagerly_created.iter().rev() { + let _ = std::fs::remove_dir(dir); + } + return Err(error); + } + let canonical_path = pinned_canonical_path(path)?; + Ok(Self { + canonical_path, + eagerly_created, + }) + } + #[cfg(target_os = "macos")] + { + Ok(Self { + canonical_path: canonicalize_allowing_missing_leaf(path), + eagerly_created: Vec::new(), + }) + } + #[cfg(not(any(target_os = "linux", target_os = "macos")))] + { + let _ = path; + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "granting a not-yet-existing write directory is not supported on this platform", + )) + } + } + + /// The canonical, symlink-resolved path to show the user and record as the + /// grant. + pub fn canonical_path(&self) -> &Path { + &self.canonical_path + } + + /// Materialize the directory once the grant is approved. A no-op on platforms + /// that already created it eagerly. + pub fn finalize(&self) -> std::io::Result<()> { + #[cfg(target_os = "macos")] + { + std::fs::create_dir_all(&self.canonical_path)?; + } + Ok(()) + } + + /// Remove exactly the directories we created (deepest-first) when the grant + /// is denied. Best-effort; `rmdir` leaves non-empty dirs and swapped-in + /// symlinks untouched. + pub fn discard(self) { + for dir in self.eagerly_created.iter().rev() { + let _ = std::fs::remove_dir(dir); + } + } +} + +/// Create each missing component of `path` with `create_dir` (never +/// `create_dir_all`), recording exactly the directories created so they can be +/// removed again if the grant is denied. Components that already exist are left +/// alone. +#[cfg(target_os = "linux")] +fn create_missing_dirs(path: &Path, created: &mut Vec) -> std::io::Result<()> { + let mut cur = PathBuf::new(); + for component in path.components() { + cur.push(component); + match std::fs::create_dir(&cur) { + Ok(()) => created.push(cur.clone()), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => return Err(error), + } + } + Ok(()) +} + +/// Open an `O_PATH` handle to `path` and read back the canonical path of the +/// inode it pins, so the value shown to the user reflects the real target even +/// when a component is a symlink. +#[cfg(target_os = "linux")] +fn pinned_canonical_path(path: &Path) -> std::io::Result { + use std::os::fd::AsRawFd as _; + use std::os::unix::fs::OpenOptionsExt as _; + let handle = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_PATH | libc::O_CLOEXEC) + .open(path)?; + std::fs::read_link(format!("/proc/self/fd/{}", handle.as_raw_fd())) +} + +#[cfg(all(test, target_os = "linux"))] +mod grantable_write_dir_tests { + use super::GrantableWriteDir; + use std::fs; + + #[test] + fn creates_missing_dirs_and_discard_removes_only_those() { + let root = tempfile::tempdir().unwrap(); + let existing = root.path().join("existing"); + fs::create_dir(&existing).unwrap(); + let target = existing.join("a").join("b").join("c"); + + let prepared = GrantableWriteDir::prepare(&target).unwrap(); + assert!(target.is_dir()); + assert_eq!(prepared.canonical_path(), target.canonicalize().unwrap()); + + prepared.discard(); + // Everything we created is gone... + assert!(!existing.join("a").exists()); + // ...but the pre-existing ancestor is untouched. + assert!(existing.is_dir()); + } + + #[test] + fn existing_dir_is_left_alone_and_not_removed_on_discard() { + let root = tempfile::tempdir().unwrap(); + let target = root.path().join("already"); + fs::create_dir(&target).unwrap(); + + let prepared = GrantableWriteDir::prepare(&target).unwrap(); + assert!(target.is_dir()); + // We created nothing, so discard removes nothing. + prepared.discard(); + assert!(target.is_dir()); + } + + #[test] + fn canonical_path_resolves_a_symlinked_parent() { + let root = tempfile::tempdir().unwrap(); + let real = root.path().join("real"); + fs::create_dir(&real).unwrap(); + let link = root.path().join("link"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + // Granting `link/child` where `link` legitimately points at `real` must + // succeed and show the user the *resolved* `real/child`, not fail. + let prepared = GrantableWriteDir::prepare(&link.join("child")).unwrap(); + assert_eq!( + prepared.canonical_path(), + real.canonicalize().unwrap().join("child") + ); + assert!(real.join("child").is_dir()); + + prepared.discard(); + assert!(!real.join("child").exists()); + } +} + /// Canonicalize `path`, resolving symlinks, even when its final component /// doesn't exist yet. /// diff --git a/crates/settings_content/src/agent.rs b/crates/settings_content/src/agent.rs index 0d27b3f9a84a96..460714c4622d6f 100644 --- a/crates/settings_content/src/agent.rs +++ b/crates/settings_content/src/agent.rs @@ -812,6 +812,14 @@ pub struct SandboxPermissionsContent { /// to without prompting. Paths written by Zed are absolute. /// Default: [] pub write_paths: Option>, + + /// Whether to warn when a sandbox escalation prompt requests a domain or + /// write path that contains potentially confusable Unicode characters + /// (homoglyphs, invisible characters, or bidirectional overrides). When + /// enabled, such prompts show a warning that must be acknowledged before + /// the request can be allowed. + /// Default: true + pub warn_confusable_unicode: Option, } #[with_fallible_options] diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index b3a145c659887c..ecc19ef8ac5c4a 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -21,6 +21,7 @@ agent_settings.workspace = true agent_skills.workspace = true anyhow.workspace = true audio.workspace = true +client.workspace = true cloud_api_types.workspace = true component.workspace = true codestral.workspace = true diff --git a/crates/settings_ui/src/pages/sandbox_settings.rs b/crates/settings_ui/src/pages/sandbox_settings.rs index da69fdd5761d16..533054f015f5f6 100644 --- a/crates/settings_ui/src/pages/sandbox_settings.rs +++ b/crates/settings_ui/src/pages/sandbox_settings.rs @@ -73,6 +73,25 @@ pub(crate) fn render_sandbox_settings_page( ) .tab_index(0), ) + .child({ + let docs_url = + client::zed_urls::sandboxing_docs(Some("persistent-sandbox-permissions"), cx); + let tooltip = format!("Opens {docs_url}"); + // Wrap in a row so the button shrinks to its content width instead + // of stretching across the settings page. + h_flex().child( + Button::new("sandbox-docs-link", "Learn more about sandboxing") + .label_size(LabelSize::Small) + .color(Color::Muted) + .end_icon( + Icon::new(IconName::ArrowUpRight) + .color(Color::Muted) + .size(IconSize::XSmall), + ) + .tooltip(Tooltip::text(tooltip)) + .on_click(move |_, _, cx| cx.open_url(&docs_url)), + ) + }) .when(sandbox_enabled, |this| this .when_some(validation_error, |this, error| { this.child( @@ -145,6 +164,27 @@ pub(crate) fn render_sandbox_settings_page( empty_border, )), ) + .child(Divider::horizontal()) + .child( + v_flex() + .gap_4() + .child(SettingsSectionHeader::new("Escalation Prompts").no_padding(true)) + .child( + SwitchField::new( + "sandbox-warn-confusable-unicode", + Some("Warn About Confusable Unicode"), + Some( + "Warn when an approval prompt requests a domain or write path that contains potentially confusable Unicode characters, such as homoglyphs (i.e. two symbols that look similar, such as a Cyrillic `а`)" + .into(), + ), + permissions.warn_confusable_unicode, + move |state, _window, cx| { + set_warn_confusable_unicode(*state == ToggleState::Selected, cx); + }, + ) + .tab_index(0), + ), + ) ) .into_any_element() } @@ -418,6 +458,12 @@ fn set_allow_fs_write_all(value: bool, cx: &mut App) { }); } +fn set_warn_confusable_unicode(value: bool, cx: &mut App) { + update_sandbox_permissions(cx, move |permissions| { + permissions.warn_confusable_unicode = Some(value); + }); +} + fn add_network_host(host: String, cx: &mut App) { update_sandbox_permissions(cx, move |permissions| { let hosts = &mut permissions.network_hosts.get_or_insert_default().0; diff --git a/docs/src/ai/sandboxing.md b/docs/src/ai/sandboxing.md index 715e331da7fade..5603d3c307f1e1 100644 --- a/docs/src/ai/sandboxing.md +++ b/docs/src/ai/sandboxing.md @@ -9,9 +9,11 @@ You can restrict what operations the [Zed Agent](./zed-agent.md) can run in mult [Tool Permissions](./tool-permissions.md), but these are of limited use when the agent wants to do things like run a complicated script in a terminal. -Sandboxing runs certain tool actions in an OS-level sandbox which limits filesystem access and network access, while -protecting Git metadata such as `.git` directories. This way, even if the agent wants to run an arbitrary script, that -script will only be able to write to the files and folders you have allowed it to. +Sandboxing instead uses OS features to forcibly restrict which resources a tool +call has access to. This does _not_ rely on an agent following a particular set +of instructions. If the agent attempts to access a resource that is restricted +by the sandbox, the OS will block it. See [How much can I trust the +sandbox?](#trust) for more details. [Tool Permissions](./tool-permissions.md) can be used in addition to sandboxing: @@ -23,27 +25,104 @@ terminal tabs, [External Agents](./external-agents.md), or [Terminal Threads](./ ## Sandboxed Tools {#sandboxed-tools} -Zed Agent sandboxing currently applies to the `terminal` tool. +Zed Agent sandboxing currently applies to the `terminal` and `fetch` tools. | Tool | What sandboxing limits | | ---------- | ----------------------------------------------------------------------------------------------------- | | `terminal` | Filesystem writes and outbound network access for commands the agent runs; Git metadata is protected. | +| `fetch` | Hosts which can be accessed. | -Other built-in tools, including `fetch`, are still governed by [Tool Permissions](./tool-permissions.md), -[Agent Profiles](./agent-profiles.md), and project trust, but they are not currently run inside this OS sandbox. +Tools are still governed by [Tool Permissions](./tool-permissions.md), [Agent +Profiles](./agent-profiles.md), and project trust, but they are not currently +run inside this OS sandbox. + +## Requirements {#requirements} + +Sandboxing is supported, in some form, on all platforms. In order to sandbox a +`terminal` tool call, the following is required: + +- On Linux, a runnable, non-setuid `bwrap` binary must be on the `$PATH`. See [Installing Bubblewrap](#installing-bubblewrap). +- On Windows, WSL must be available. + +There are no extra requirements on MacOS. + +The `fetch` tool has no extra requirements on any platform. ## Default Access {#default-access} By default, sandboxed Zed Agent tool actions have these restrictions: -| Access type | Default behavior | -| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Filesystem reads | Terminal commands can read most of the filesystem, including protected Git metadata. | -| Project writes | Terminal commands can write inside open project directories, except for protected Git metadata. | -| Git metadata | `.git` directories and linked worktree Git metadata remain readable but are not writable while sandboxed. | -| Temporary files | Terminal commands receive a writable temporary location. The exact behavior differs by platform. | -| Other writes | Writes outside the default writable locations are blocked unless you approve a broader sandbox request. | -| Outbound networking | Network access is blocked unless you approve a host-specific or unrestricted network sandbox request. Host-specific enforcement is not available on every platform. | +| Access type | Default behavior | +| ------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Filesystem reads | Terminal commands can read most of the filesystem, including protected Git metadata. | +| Project writes | Terminal commands can write inside open project directories, except for protected Git metadata. | +| Git metadata | `.git` directories and linked worktree Git metadata remain readable but are not writable while sandboxed. | +| Temporary files | Terminal commands receive a writable temporary location. The exact behavior differs by platform. | +| Other writes | Writes outside the default writable locations are blocked unless you approve a broader sandbox request. | +| Outbound networking | Network access is blocked unless you approve a host-specific or unrestricted network sandbox request. Host-specific enforcement is not available on every platform. | +| Local IPC sockets | Sandboxed commands cannot open Unix-domain sockets (for example, to the desktop session bus or a container daemon), which could otherwise be used to run commands outside the sandbox. | + +## How much can I trust the sandbox? {#trust} + +Enabling sandboxing dramatically reduces the risk of various kinds of attacks. +However, it does not fully eliminate them. + +Firstly, sandboxing relies on OS-level features, which may contain bugs. +Operating systems have historically had bugs in security features. And while we +have tested thoroughly, there may also be bugs in Zed's implementation. These +could allow privilege escalation - for example, allow an agent to write to a +file that it should only have read access to. + +Sandboxing also only applies the restrictions that the user requested. If an +agent requests write access to your home directory, sandboxing will (and +should!) do nothing to prevent an agent adding a malicious key to `$HOME/.ssh`. + +Be careful with what you grant the agent. At any point in time, you can view the +state of the sandbox by hovering the padlock icon in the top right of the +thread. If an agent is requesting an overly broad permission, deny it, and ask +it to use a smaller grant. When requesting elevated privileges, agents must +provide a `reason`, which is displayed in the prompt. Read it, and decide +whether it makes sense before approving. + +Also, sandboxing restricts **only** what the `terminal` and `fetch` tools in the +Zed agent can do. It has **no effect** on other parts of Zed, including: + +- Language servers +- The built-in git client +- The regular terminal +- And more... + +Even when sandboxing is enabled, you should remain vigilant. A malicious or +unaligned agent may use these side channels to escalate privileges. For example: + +- An agent may add a malicious Rust procedural macro to your codebase, which + will be automatically executed by `rust-analyzer` **outside the sandbox**. +- An agent may modify a `Makefile` to inject a malicious script, which is + executed **outside the sandbox** when you next run `make` in the built-in + terminal. +- The agent cannot write to your repository's protected `.git` directory, but it + can create a submodule under your project whose Git metadata (including config + such as `core.fsmonitor`) it fully controls. That metadata may then be executed + **outside the sandbox** when you subsequently run Git commands in a regular + terminal. Your shell prompt may even execute Git commands every time it + renders! + +There are steps you can take to mitigate these issues. For example: + +- disable language servers that execute user-defined code from the project (such + as Rust procedural macros). +- use a shell prompt that reports Git status without executing + repository-defined programs. +- review the diff before running `git commit` + +But none of this changes the fundamental principle: **a sandbox is not a +substitute for good security practices**. It is one layer in a defense-in-depth +strategy. + +Zed's default profile aims to strike a balance between security and convenience, +but we encourage you to tune your settings based on your own security +requirements and risk profile. A disabled sandbox is not a very effective +sandbox. ## Approval Prompts {#approval-prompts} @@ -88,7 +167,7 @@ The available options are: | `allow_all_hosts` | Allow sandboxed tools to reach any host without prompting. | | `write_paths` | Directory subtrees that sandboxed terminal commands may write to without prompting. Paths are absolute. | | `allow_fs_write_all` | Allow sandboxed terminal commands to write anywhere except protected Git metadata without prompting. | -| `allow_unsandboxed` | Allow terminal commands to run outside the sandbox without prompting when the agent explicitly requests it. | +| `allow_unsandboxed` | Turn sandboxing off entirely for Zed Agent terminal commands. The fetch tool will have no restrictions. | Prefer narrow grants, such as a specific host or write path, over `allow_all_hosts`, `allow_fs_write_all`, or `allow_unsandboxed`. @@ -99,10 +178,6 @@ Git metadata writes are not grantable while a terminal command is sandboxed. Thi linked worktree metadata, refs, the index, hooks, local Git config, and other Git-controlled metadata files. Approving a specific writable path or `allow_fs_write_all` does not make Git metadata writable. -When a Git command genuinely needs to update Git metadata, such as `git commit`, `git fetch`, `git checkout`, or `git -rebase`, approve unsandboxed execution instead. For read-only operations, prefer Git flags that avoid optional metadata -writes when possible. For example, use `git --no-optional-locks status` instead of `git status`. - ## Platform Support {#platform-support} Sandboxing uses different operating system mechanisms on each platform. The user-facing prompts are similar, but the @@ -121,6 +196,7 @@ Sandboxed terminal commands: - cannot write protected Git metadata, even if you approve broader write access - cannot write elsewhere unless you approve additional paths or broader write access - cannot reach the network unless you approve network access +- can reach only an allowlist of macOS system (Mach) services that developer tooling needs; services that could be abused to escape the sandbox (LaunchServices and launchd, which can launch processes outside it), read the clipboard (the pasteboard), or capture audio are not reachable When network access is approved on macOS, Zed uses an HTTP/HTTPS proxy so access can be limited to approved hosts. Tools that do not honor proxy environment variables, such as SSH, FTP, and raw socket clients, may not work even after host-specific network access is approved. @@ -128,7 +204,7 @@ For networked terminal commands, prefer HTTPS URLs over SSH URLs when possible. ### Linux {#linux} -On Linux, Zed uses Bubblewrap (`bwrap`) for sandboxing. +On Linux, Zed uses [Bubblewrap][bubblewrap] (`bwrap`) for sandboxing. Zed only uses a non-setuid `bwrap` binary. Its sandbox is built entirely on unprivileged user namespaces, so a setuid-root `bwrap` provides no extra functionality, and running one would mean executing root-privileged setup with arguments partly @@ -139,7 +215,7 @@ Sandboxed terminal commands: - can read the filesystem, including protected Git metadata contents - can write inside open project directories, except protected Git metadata -- can write to `/tmp`, which is backed by a fresh temporary filesystem and is cleared between terminal tool calls +- can write to `/tmp`, which is backed by a fresh temporary filesystem and is cleared between terminal tool calls (when you approve unrestricted filesystem writes, `/tmp` is instead your real host `/tmp` rather than a fresh temporary filesystem) - cannot write protected Git metadata - cannot write elsewhere unless you approve additional paths or broader write access - cannot reach the network unless you approve network access @@ -151,6 +227,54 @@ after host-specific network access is approved. If Bubblewrap is unavailable or cannot create a sandbox in the current environment, Zed may run the command without the OS sandbox and show a warning in the tool output. +#### Installing Bubblewrap {#installing-bubblewrap} + +Zed needs a runnable, non-setuid `bwrap` binary on your `$PATH`. Installing +`bubblewrap` from your distribution's package manager is usually all you need. + +You can test whether it's working with: + +```sh +bwrap --ro-bind / / -- echo "working" +``` + +"Non-setuid" here refers to the [setuid bit][setuid bit]. Historically, +bubblewrap has shipped both a setuid and non-setuid binary. The setuid binary is +being phased out for security concerns, and so Zed's sandbox _explicitly rejects +setuid `bwrap` binaries_. + +##### Ubuntu-specific requirements {#installing-bubblewrap-ubuntu} + +> **Note:** The following does not affect Ubuntu on WSL. + +Bubblewrap relies on a Linux kernel feature known as "namespaces". Unprivileged +users on many systems can create namespaces, but historically, this feature has +been used for a variety of attacks. + +In response to this, in Ubuntu 23.10, Canonical [added a security +measure][ubuntu blog] that restricts unprivileged user namespaces. These +restrictions are enforced by AppArmor. + +Because of this, you may also need to install an AppArmor profile for bubblewrap +after you install it. This is a configuration file that gives bubblewrap the +ability to create namespaces without needing `sudo`. + +```sh +sudo apt install bubblewrap + +# On Ubuntu 25.04 and later, `apparmor` ships with a profile for bubblewrap by default. +# Make sure you're up-to-date +sudo apt install --only-upgrade apparmor + +# On older versions, manually install the profile +sudo apt update +sudo apt install apparmor-profiles apparmor-utils +sudo install -m 0644 \ + /usr/share/apparmor/extra-profiles/bwrap-userns-restrict \ + /etc/apparmor.d/bwrap-userns-restrict +sudo apparmor_parser -r /etc/apparmor.d/bwrap-userns-restrict +``` + ### Windows {#windows} On Windows, Zed Agent sandboxing is supported only when the agent action runs inside WSL. @@ -167,8 +291,8 @@ When running inside WSL, the Linux sandboxing behavior applies, including the re - network access is all-or-nothing rather than host-specific, so host-specific network requests are rejected and the agent must request unrestricted network access when network access is needed If WSL is not installed, or if you choose to run a command without the sandbox, Zed falls back to the standard terminal -behavior of running in your native shell. It selects the shell using the usual preference order: Git Bash (or scoop's -bash) when one is installed, otherwise PowerShell, and finally `cmd.exe`. Because the command then runs against native +behavior of running in your native shell. It selects the shell using the usual preference order: a bash (scoop's bash or +Git Bash) when one is installed, otherwise PowerShell, and finally `cmd.exe`. Because the command then runs against native Windows paths instead of WSL's Linux filesystem, path conventions change accordingly (for example `C:\...` or `/c/...` rather than WSL's `/mnt/c/...`), so a command written for the sandboxed WSL shell may behave differently. @@ -184,3 +308,7 @@ When reviewing a sandbox prompt, prefer the narrowest permission that lets the t If a command fails because the sandbox blocked access, ask the agent why it needs that access before approving a broader request. + +[bubblewrap]: https://github.com/containers/bubblewrap +[setuid bit]: https://en.wikipedia.org/wiki/Setuid +[ubuntu blog]: https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces diff --git a/nix/tests/sandboxing/default.nix b/nix/tests/sandboxing/default.nix index 185c70bbe4cc5f..7aee3c7537d020 100644 --- a/nix/tests/sandboxing/default.nix +++ b/nix/tests/sandboxing/default.nix @@ -23,6 +23,7 @@ # { read = "/path"; succeeds = true; } # read a host file # { write = "/path"; succeeds = false; } # write a host file # { network = "echo1"; succeeds = true; } # connect to an echo server +# { socketPath = "/run/x.sock"; succeeds = false; } # connect a unix socket # { canCreate = false; error = "bwrap_not_found"; } # Sandbox::can_create # # plus optional policy fields applied to that check (defaults shown): @@ -71,6 +72,14 @@ let }; }; + # A unix-domain socket, owned by a process *outside* the sandbox, that a + # sandboxed command must not be able to `connect()` to. It lives under `/run` + # (which the sandbox `--ro-bind`s along with the rest of `/`) and NOT under + # `/tmp` (which the restricted-fs sandbox masks with a tmpfs, hiding anything + # there), so it stays visible inside the sandbox and the block is what's + # actually under test. + unixSocketPath = "/run/zed-sandbox-test.sock"; + # Quiet boot + a couple of cores; shared by every machine-under-test. baseMachine = { boot.consoleLogLevel = lib.mkForce 3; # be quiet pls :) @@ -80,6 +89,18 @@ let memorySize = 1024; cores = 2; }; + + # A unix-socket echo server outside the sandbox, so a sandboxed `connect()` + # has a real peer: without a listener the connect would fail with + # ECONNREFUSED even absent the sandbox, giving a false "blocked" pass. + systemd.services.unix-echo-server = { + description = "Unix-domain-socket echo server"; + wantedBy = [ "multi-user.target" ]; + serviceConfig = { + ExecStart = "${pkgs.socat}/bin/socat -d UNIX-LISTEN:${unixSocketPath},fork,reuseaddr EXEC:cat"; + Restart = "on-failure"; + }; + }; }; # Per-scenario host configuration. Each entry layers onto `baseMachine`. @@ -158,6 +179,10 @@ let machine.wait_until_succeeds("getent hosts echo1", timeout=30) machine.wait_until_succeeds("getent hosts echo2", timeout=30) + # The unix-socket checks need a real peer outside the sandbox; wait for + # the listener to be up before running the helper. + machine.wait_until_succeeds("test -S ${unixSocketPath}", timeout=30) + # The helper logs each check tagged `[sandbox_test]:`. `succeed` fails the # whole test on a non-zero exit; we print its output so the per-check # results show up in the build log. @@ -322,6 +347,31 @@ in succeeds = false; } + # ---- Unix-domain socket escape ---------------------------------------- + # A sandboxed command must NOT be able to connect to a unix-domain socket + # owned by a process outside the sandbox (session-IPC escape). Currently + # FAILS (no seccomp guard yet); becomes a regression test once the + # socket(AF_UNIX) seccomp filter lands. + { + fs = "restricted"; + writablePaths = [ "/sandbox-test/writable" ]; + networkAccess = "blocked"; + socketPath = unixSocketPath; + succeeds = false; + } + + # The unix-socket block must hold regardless of network policy: our design + # decouples unix-socket blocking from the network grant, so even an + # unrestricted-network command must not reach an outside-the-sandbox + # session IPC socket. Also currently FAILS until the seccomp filter lands. + { + fs = "restricted"; + writablePaths = [ "/sandbox-test/writable" ]; + networkAccess = "unrestricted"; + socketPath = unixSocketPath; + succeeds = false; + } + # On a working host the sandbox can be created. { fs = "restricted";