diff --git a/src/openhuman/security/policy.rs b/src/openhuman/security/policy.rs index 203aec076..39e405438 100644 --- a/src/openhuman/security/policy.rs +++ b/src/openhuman/security/policy.rs @@ -774,9 +774,92 @@ impl SecurityPolicy { } } + // Symlink-safe check (#1927). The string-level checks above can be + // bypassed by creating a symlink inside the workspace that points to + // a forbidden tree (e.g. `evil -> /etc/shadow`). Canonicalize the + // path and re-validate `workspace_only` containment + forbidden_paths + // against the resolved location. + if let Some(canonical) = self.try_canonicalize_under_workspace(path) { + let workspace_root = self + .workspace_dir + .canonicalize() + .unwrap_or_else(|_| self.workspace_dir.clone()); + if self.workspace_only && !canonical.starts_with(&workspace_root) { + log::trace!( + "[security:policy] path blocked: symlink escapes workspace (requested={}, resolved={}, workspace={})", + path, + canonical.display(), + workspace_root.display() + ); + return false; + } + // If the resolved path stays inside the workspace, trust the + // workspace boundary over forbidden_paths — otherwise a workspace + // that lives under e.g. `/tmp` (common in tests and sandboxes) + // would block every legitimate access. forbidden_paths is meant + // to catch escapes *outside* the workspace, which the workspace + // containment check above already validates. + let inside_workspace = canonical.starts_with(&workspace_root); + if !inside_workspace { + for forbidden in &self.forbidden_paths { + let forbidden_expanded = if let Some(stripped) = forbidden.strip_prefix("~/") { + std::env::var("HOME") + .ok() + .map(|h| PathBuf::from(h).join(stripped)) + .unwrap_or_else(|| PathBuf::from(forbidden)) + } else { + PathBuf::from(forbidden) + }; + let forbidden_canonical = forbidden_expanded + .canonicalize() + .unwrap_or(forbidden_expanded); + if canonical.starts_with(&forbidden_canonical) { + log::trace!( + "[security:policy] path blocked: symlink resolves to forbidden tree (requested={}, resolved={}, forbidden={})", + path, + canonical.display(), + forbidden_canonical.display() + ); + return false; + } + } + } + } + true } + /// Resolve a user-supplied path under the workspace, canonicalizing it + /// (or its parent) when present on disk. Used by [`Self::is_path_allowed`] + /// to defend against symlink-based escapes that pass the string-level + /// checks. Returns `None` only when neither the path nor its parent can + /// be resolved on disk — in that case the caller falls back to the + /// string-level checks alone (which is the safe default for fresh paths + /// whose entire chain does not yet exist). + fn try_canonicalize_under_workspace(&self, path: &str) -> Option { + let expanded = if let Some(stripped) = path.strip_prefix("~/") { + std::env::var("HOME") + .ok() + .map(|h| PathBuf::from(h).join(stripped))? + } else { + PathBuf::from(path) + }; + let absolute = if expanded.is_absolute() { + expanded + } else { + self.workspace_dir.join(&expanded) + }; + if let Ok(canonical) = absolute.canonicalize() { + return Some(canonical); + } + // Path itself does not exist (e.g. a write-to-new-file call). Try + // canonicalizing the parent + appending the basename so we still + // catch parent chains that resolve via symlink to a forbidden tree. + let parent = absolute.parent()?; + let name = absolute.file_name()?; + parent.canonicalize().ok().map(|p| p.join(name)) + } + /// Validate that a resolved path is still inside the workspace. /// Call this AFTER joining `workspace_dir` + relative path and canonicalizing. pub fn is_resolved_path_allowed(&self, resolved: &Path) -> bool { diff --git a/src/openhuman/security/policy_tests.rs b/src/openhuman/security/policy_tests.rs index 1ee332567..c89eb0cad 100644 --- a/src/openhuman/security/policy_tests.rs +++ b/src/openhuman/security/policy_tests.rs @@ -393,6 +393,91 @@ fn dotfile_in_workspace_allowed() { assert!(p.is_path_allowed(".env")); } +// -- is_path_allowed — symlink safety (#1927) --------------------- + +#[cfg(unix)] +#[test] +fn symlink_inside_workspace_escaping_outside_is_blocked() { + use std::os::unix::fs::symlink; + + let workspace = tempfile::tempdir().expect("workspace tempdir"); + let outside = tempfile::tempdir().expect("outside tempdir"); + let target = outside.path().join("secret.txt"); + std::fs::write(&target, "secret").expect("write secret"); + + let link = workspace.path().join("evil"); + symlink(&target, &link).expect("create symlink"); + + let p = SecurityPolicy { + workspace_dir: workspace.path().to_path_buf(), + workspace_only: true, + forbidden_paths: vec![], + ..SecurityPolicy::default() + }; + + // String-level checks pass: "evil" has no "..", isn't absolute, and is + // not in forbidden_paths. The canonicalize step must catch the symlink + // pointing outside the workspace root. + assert!( + !p.is_path_allowed("evil"), + "symlink that escapes the workspace must be blocked" + ); +} + +#[cfg(unix)] +#[test] +fn symlink_to_forbidden_tree_is_blocked() { + use std::os::unix::fs::symlink; + + let workspace = tempfile::tempdir().expect("workspace tempdir"); + let forbidden = tempfile::tempdir().expect("forbidden tempdir"); + let target = forbidden.path().join("secret"); + std::fs::write(&target, "x").expect("write secret"); + + let link = workspace.path().join("link-to-forbidden"); + symlink(&target, &link).expect("create symlink"); + + let p = SecurityPolicy { + workspace_dir: workspace.path().to_path_buf(), + // Disable workspace_only so the assertion isolates the forbidden_paths + // path (the symlink escapes the workspace, which would also trip + // workspace_only — but here we want to prove the forbidden_paths + // check itself canonicalizes). + workspace_only: false, + forbidden_paths: vec![forbidden.path().to_string_lossy().to_string()], + ..SecurityPolicy::default() + }; + + // The string "link-to-forbidden" does not start with the forbidden + // tempdir path, so the string-level check passes. Canonical resolution + // must catch that it resolves into the forbidden tree. + assert!( + !p.is_path_allowed("link-to-forbidden"), + "symlink that resolves into a forbidden tree must be blocked" + ); +} + +#[test] +fn write_to_not_yet_existing_path_in_workspace_still_allowed() { + // After adding the symlink-safe canonicalize step, writing to a + // not-yet-existing path inside the workspace must still pass — the + // parent-dir fallback canonicalizes the parent and confirms it is + // inside the workspace root. + let workspace = tempfile::tempdir().expect("workspace tempdir"); + let p = SecurityPolicy { + workspace_dir: workspace.path().to_path_buf(), + workspace_only: true, + forbidden_paths: vec![], + ..SecurityPolicy::default() + }; + + assert!(p.is_path_allowed("new-file.txt")); + // Whole parent chain missing too — helper returns None, and we fall + // back to the string-level checks (which would pass for a + // workspace-relative non-traversal path). + assert!(p.is_path_allowed("not-yet-existing/subdir/file.txt")); +} + // -- from_config -------------------------------------------------- #[test] diff --git a/src/openhuman/tools/impl/filesystem/file_read.rs b/src/openhuman/tools/impl/filesystem/file_read.rs index 3a692c4e6..dc0654b3b 100644 --- a/src/openhuman/tools/impl/filesystem/file_read.rs +++ b/src/openhuman/tools/impl/filesystem/file_read.rs @@ -323,7 +323,16 @@ mod tests { let result = tool.execute(json!({"path": "escape.txt"})).await.unwrap(); assert!(result.is_error); - assert!(result.output().contains("escapes workspace")); + // After the symlink-safe canonical check landed in + // SecurityPolicy::is_path_allowed (#1927), the policy layer blocks + // the escape before file_read's own resolved-path check runs — the + // error becomes "Path not allowed by security policy". Either + // message signals defense-in-depth worked. + let out = result.output(); + assert!( + out.contains("escapes workspace") || out.contains("not allowed"), + "expected escape/not-allowed error, got: {out}" + ); let _ = tokio::fs::remove_dir_all(&root).await; } diff --git a/src/openhuman/tools/impl/filesystem/file_write.rs b/src/openhuman/tools/impl/filesystem/file_write.rs index 2b97765ca..a917c2b8f 100644 --- a/src/openhuman/tools/impl/filesystem/file_write.rs +++ b/src/openhuman/tools/impl/filesystem/file_write.rs @@ -319,7 +319,14 @@ mod tests { .unwrap(); assert!(result.is_error); - assert!(result.output().contains("escapes workspace")); + // SecurityPolicy now blocks symlink escapes at the is_path_allowed + // layer (#1927) — error becomes "Path not allowed by security + // policy" rather than the deeper "escapes workspace" message. + let out = result.output(); + assert!( + out.contains("escapes workspace") || out.contains("not allowed"), + "expected escape/not-allowed error, got: {out}" + ); assert!(!outside.join("hijack.txt").exists()); let _ = tokio::fs::remove_dir_all(&root).await; @@ -395,9 +402,13 @@ mod tests { .unwrap(); assert!(result.is_error, "writing through symlink must be blocked"); + // The symlink-safe is_path_allowed check (#1927) blocks at the + // policy layer before the tool's own symlink-target detection + // runs; accept either error message. + let out = result.output(); assert!( - result.output().contains("symlink"), - "error should mention symlink" + out.contains("symlink") || out.contains("not allowed"), + "error should mention symlink or policy block, got: {out}" ); // Verify original file was not modified