fix(sandbox): harden OCI workspace handling

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

Author: matthewgrossman

.agents/skills/openshell-cli/SKILL.md

@@ -223,8 +223,8 @@ openshell sandbox ssh-config my-sandbox >> ~/.ssh/config
 # Upload local files to the sandbox working directory
 openshell sandbox upload my-sandbox ./src
 
-# Download files from sandbox
-openshell sandbox download my-sandbox /sandbox/output ./local-output
+# Download files from sandbox (replace /workspace with the path from `pwd -P`)
+openshell sandbox download my-sandbox /workspace/output ./local-output
 ```
 
 Uploads honor `.gitignore` by default. Add `--no-git-ignore` only when ignored files are intentionally in scope.

architecture/compute-runtimes.md

@@ -147,13 +147,15 @@ primary GID. It does not rewrite the account files.
 
 Docker and Podman use an absolute OCI working directory as the workspace. An
 empty or root (`/`) declaration falls back to `/sandbox`, which OpenShell
-creates inside the container when needed. Before direct or SSH children start,
-the supervisor transfers ownership of only the workspace directory to the
-completed UID/GID. Image-provided contents and nested mounts retain their
-ownership. The resolved workspace is also the child cwd and `HOME`, the
-automatic writable policy path, and Podman's managed workspace-volume mount
-target. Kubernetes/OpenShift keep their `/sandbox` PVC and `fsGroup` behavior,
-and VM keeps its `/sandbox` guest initialization path.
+creates inside the container when needed. Protected system trees and
+OpenShell-reserved paths cannot become workspaces. The container runtime starts
+the supervisor from `/`; before direct or SSH children start, the supervisor
+validates the resolved path and transfers ownership of only the workspace
+directory to the completed UID/GID. Image-provided contents and nested mounts
+retain their ownership. The resolved workspace is also the child cwd and
+`HOME`, the automatic writable policy path, and Podman's managed
+workspace-volume mount target. Kubernetes/OpenShift keep their `/sandbox` PVC
+and `fsGroup` behavior, and VM keeps its `/sandbox` guest initialization path.
 
 Sandbox creation fails before the workload becomes ready when a required image
 identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0.

crates/openshell-cli/src/ssh.rs

@@ -1115,7 +1115,15 @@ async fn ssh_run_capture_stdout(session: &SshSessionConfig, command: &str) -> Re
             output.status
         ));
     }
-    Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
+    decode_ssh_probe_stdout(output.stdout)
+}
+
+fn decode_ssh_probe_stdout(stdout: Vec<u8>) -> Result<String> {
+    let stdout = String::from_utf8(stdout)
+        .map_err(|error| miette::miette!("ssh probe returned non-UTF-8 output: {error}"))?;
+    let stdout = stdout.strip_suffix('\n').unwrap_or(&stdout);
+    let stdout = stdout.strip_suffix('\r').unwrap_or(stdout);
+    Ok(stdout.to_string())
 }
 
 fn validate_discovered_workspace_root(root: &str) -> Result<String> {
@@ -2092,6 +2100,19 @@ mod tests {
         }
     }
 
+    #[test]
+    fn ssh_probe_output_only_removes_the_protocol_line_ending() {
+        assert_eq!(
+            decode_ssh_probe_stdout(b"/workspace/project \n".to_vec()).unwrap(),
+            "/workspace/project "
+        );
+        assert_eq!(
+            decode_ssh_probe_stdout(b"/workspace/project\r\n".to_vec()).unwrap(),
+            "/workspace/project"
+        );
+        assert!(decode_ssh_probe_stdout(vec![0xff]).is_err());
+    }
+
     #[test]
     fn build_single_file_tar_cmd_inserts_double_dash_before_basename() {
         // Without `--`, a basename such as `--checkpoint-action=...` would be

crates/openshell-core/src/driver_mounts.rs

@@ -32,6 +32,46 @@ const RESERVED_MOUNT_TARGETS: &[&str] = &[
     "/run/netns",
 ];
 
+/// Container filesystem trees that image metadata must never turn into an
+/// agent-owned writable workspace.
+const PROTECTED_WORKSPACE_TREES: &[&str] = &[
+    "/bin",
+    "/boot",
+    "/dev",
+    "/etc",
+    "/lib",
+    "/lib64",
+    "/proc",
+    "/root",
+    "/run",
+    "/sbin",
+    "/sys",
+    "/usr/bin",
+    "/usr/lib",
+    "/usr/lib64",
+    "/usr/local/bin",
+    "/usr/local/lib",
+    "/usr/local/lib64",
+    "/usr/local/sbin",
+    "/usr/local/share",
+    "/usr/sbin",
+    "/usr/share",
+    "/var/log",
+];
+
+/// Broad container roots that are unsafe as workspaces themselves, while
+/// application-specific descendants remain valid.
+const PROTECTED_WORKSPACE_ROOTS: &[&str] = &[
+    "/home",
+    "/mnt",
+    "/opt",
+    "/srv",
+    "/usr",
+    "/usr/local",
+    "/var",
+    "/var/lib",
+];
+
 /// Compatibility workspace used when an OCI image has no usable working
 /// directory and by drivers whose workspace remains fixed.
 pub const DEFAULT_WORKSPACE_ROOT: &str = "/sandbox";
@@ -139,8 +179,11 @@ pub fn resolve_oci_workspace_root(working_dir: &str) -> Result<String, String> {
     if working_dir.is_empty() || working_dir == "/" {
         return Ok(DEFAULT_WORKSPACE_ROOT.to_string());
     }
-    if working_dir.as_bytes().contains(&0) {
-        return Err("OCI WorkingDir must not contain NUL bytes".to_string());
+    if working_dir != working_dir.trim() {
+        return Err("OCI WorkingDir must not contain surrounding whitespace".to_string());
+    }
+    if working_dir.chars().any(char::is_control) {
+        return Err("OCI WorkingDir must not contain control characters".to_string());
     }
     if !working_dir.starts_with('/') {
         return Err(format!(
@@ -159,13 +202,32 @@ pub fn resolve_oci_workspace_root(working_dir: &str) -> Result<String, String> {
         ));
     }
 
-    Ok(working_dir.trim_end_matches('/').to_string())
+    let workspace_root = working_dir.trim_end_matches('/').to_string();
+    let workspace_path = Path::new(&workspace_root);
+    if PROTECTED_WORKSPACE_ROOTS
+        .iter()
+        .any(|protected| workspace_path == Path::new(protected))
+        || PROTECTED_WORKSPACE_TREES
+            .iter()
+            .any(|protected| path_is_or_under(workspace_path, Path::new(protected)))
+        || RESERVED_MOUNT_TARGETS.iter().any(|reserved| {
+            let reserved = Path::new(reserved);
+            path_is_or_under(workspace_path, reserved) || path_is_or_under(reserved, workspace_path)
+        })
+    {
+        return Err(format!(
+            "OCI WorkingDir '{working_dir}' conflicts with a protected container path"
+        ));
+    }
+
+    Ok(workspace_root)
 }
 
-/// Reject a user-supplied mount that would replace the resolved workspace
-/// root. Mounts below the workspace remain valid.
+/// Reject a user-supplied mount that would replace or contain the resolved
+/// workspace root. Mounts below the workspace remain valid.
 pub fn validate_workspace_mount_target(target: &str, workspace_root: &str) -> Result<(), String> {
-    if normalize_mount_target(target) == workspace_root {
+    let normalized_target = normalize_mount_target(target);
+    if path_is_or_under(Path::new(workspace_root), Path::new(&normalized_target)) {
         return Err(format!(
             "mount target '{target}' is reserved for the OpenShell workspace"
         ));
@@ -202,6 +264,8 @@ mod tests {
         validate_workspace_mount_target("/sandbox/", "/sandbox").unwrap_err();
         validate_workspace_mount_target("/workspace/", "/sandbox").unwrap();
         validate_workspace_mount_target("/workspace/cache", "/workspace").unwrap();
+        validate_workspace_mount_target("/workspace", "/workspace/project").unwrap_err();
+        validate_workspace_mount_target("/workspace-other", "/workspace/project").unwrap();
     }
 
     #[test]
@@ -227,6 +291,8 @@ mod tests {
             "/workspace/./project",
             "/workspace//project",
             "/workspace\0project",
+            "/workspace ",
+            "/workspace\nproject",
         ] {
             assert!(
                 resolve_oci_workspace_root(invalid).is_err(),
@@ -235,6 +301,39 @@ mod tests {
         }
     }
 
+    #[test]
+    fn oci_workspace_root_rejects_protected_container_paths() {
+        for invalid in [
+            "/etc",
+            "/etc/project",
+            "/usr",
+            "/usr/bin/project",
+            "/var",
+            "/var/log/project",
+            "/opt",
+            "/opt/openshell/project",
+        ] {
+            assert!(
+                resolve_oci_workspace_root(invalid).is_err(),
+                "expected protected workspace '{invalid}' to be rejected"
+            );
+        }
+
+        for valid in [
+            "/app",
+            "/home/app",
+            "/opt/app",
+            "/usr/src/app",
+            "/var/lib/app",
+        ] {
+            assert_eq!(
+                resolve_oci_workspace_root(valid).unwrap(),
+                valid,
+                "expected application workspace '{valid}' to remain valid"
+            );
+        }
+    }
+
     #[test]
     fn container_target_rejects_reserved_openshell_tls_legacy_path() {
         let err = validate_container_mount_target("/etc/openshell-tls/client").unwrap_err();

crates/openshell-driver-docker/README.md

@@ -33,8 +33,9 @@ root (`/`) declaration falls back to `/sandbox`, which OpenShell creates when
 necessary. The supervisor transfers ownership of only the workspace directory
 to the completed identity before direct or SSH children start. Existing
 contents and nested bind or volume mounts retain their ownership. The workspace
-is the child cwd and `HOME`; an invalid, symlinked, or unpreparable workspace
-fails sandbox startup.
+is the child cwd and `HOME`. Protected system trees and OpenShell-reserved
+paths cannot become workspaces. The supervisor starts from `/`, then reports an
+invalid, symlinked, or unpreparable workspace as a readiness failure.
 
 Docker containers join an OpenShell-managed bridge network. The driver injects
 `host.openshell.internal` and `host.docker.internal` so supervisors have stable
@@ -86,9 +87,10 @@ optional `selinux_label` of `shared` (applies `:z`) or `private` (applies
 `subpath`. User-supplied bind and volume mounts are read-only by default; set
 `read_only: false` to make them writable. Mount `source`, `target`, and
 `subpath` values must not contain surrounding whitespace. Mount targets must be
-absolute container paths and must not replace the resolved workspace root.
-Nested workspace mounts remain valid. Mounts also must not overlap OpenShell
-supervisor files, `/etc/openshell`, `/etc/openshell-tls`, or `/run/netns`.
+absolute container paths and must not replace or contain the resolved workspace
+root. Nested workspace mounts remain valid. Mounts also must not overlap
+OpenShell supervisor files, `/etc/openshell`, `/etc/openshell-tls`, or
+`/run/netns`.
 
 Example named-volume usage:
 

crates/openshell-driver-docker/src/lib.rs

@@ -2436,6 +2436,9 @@ fn build_container_create_body_for_image(
     Ok(ContainerCreateBody {
         image: Some(image.id.clone()),
         user: Some("0".to_string()),
+        // The image workspace may need to be created or rejected by the
+        // supervisor, so do not let the OCI runtime chdir there first.
+        working_dir: Some("/".to_string()),
         env: Some(build_environment_for_oci_user(sandbox, config, &image.user)),
         entrypoint: Some(vec![SUPERVISOR_MOUNT_PATH.to_string()]),
         // Clear the image CMD so Docker does not append inherited args to the

crates/openshell-driver-docker/src/tests.rs

@@ -584,6 +584,7 @@ fn container_creation_uses_inspected_immutable_image() {
 
     assert_eq!(body.image.as_deref(), Some("sha256:immutable"));
     assert_eq!(body.user.as_deref(), Some("0"));
+    assert_eq!(body.working_dir.as_deref(), Some("/"));
     assert_eq!(
         body.cmd.as_deref(),
         Some(&["--workdir".to_string(), "/workspace/project".to_string()][..])
@@ -614,6 +615,26 @@ fn container_creation_rejects_invalid_oci_working_dir() {
     assert!(err.message().contains("must be an absolute container path"));
 }
 
+#[test]
+fn container_creation_rejects_protected_oci_working_dir() {
+    let metadata = DockerImageMetadata {
+        id: "sha256:immutable".to_string(),
+        user: "1234:1235".to_string(),
+        working_dir: "/etc".to_string(),
+    };
+    let err = build_container_create_body_for_image(
+        &test_sandbox(),
+        &runtime_config(),
+        &DockerSandboxDriverConfig::default(),
+        None,
+        &metadata,
+    )
+    .unwrap_err();
+
+    assert_eq!(err.code(), tonic::Code::FailedPrecondition);
+    assert!(err.message().contains("protected container path"));
+}
+
 #[test]
 fn container_creation_reserves_resolved_workspace_root_but_allows_nested_mounts() {
     let metadata = DockerImageMetadata {
@@ -638,6 +659,27 @@ fn container_creation_reserves_resolved_workspace_root_but_allows_nested_mounts(
             .contains("reserved for the OpenShell workspace")
     );
 
+    let ancestor_mount: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({
+        "mounts": [{"type": "tmpfs", "target": "/workspace"}]
+    }))
+    .unwrap();
+    let nested_metadata = DockerImageMetadata {
+        working_dir: "/workspace/project".to_string(),
+        ..metadata.clone()
+    };
+    let err = build_container_create_body_for_image(
+        &test_sandbox(),
+        &runtime_config(),
+        &ancestor_mount,
+        None,
+        &nested_metadata,
+    )
+    .unwrap_err();
+    assert!(
+        err.message()
+            .contains("reserved for the OpenShell workspace")
+    );
+
     let nested_mount: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({
         "mounts": [{"type": "tmpfs", "target": "/workspace/cache"}]
     }))

crates/openshell-driver-podman/README.md

@@ -23,8 +23,10 @@ necessary. The supervisor transfers ownership of only the workspace directory
 to the completed identity before direct or SSH children start. Existing
 contents and nested bind or volume mounts retain their ownership. The workspace
 is the child cwd and `HOME`, and the Podman-managed workspace volume is mounted
-there so normal volume copy-up preserves image content. An invalid, symlinked,
-or unpreparable workspace fails sandbox startup.
+there so normal volume copy-up preserves image content. Protected system trees
+and OpenShell-reserved paths cannot become workspaces. The supervisor starts
+from `/`, then reports an invalid, symlinked, or unpreparable workspace as a
+readiness failure.
 
 For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md).
 
@@ -97,8 +99,8 @@ read-only by default; set `read_only: false` to make them writable. Podman
 image and volume mounts do not support `subpath` in OpenShell driver config.
 Mount `source` and `target` values must not contain surrounding whitespace.
 Mount targets must be absolute container paths and must not replace the
-resolved workspace root. Nested workspace mounts remain valid. Mounts also
-must not overlap OpenShell supervisor files, `/etc/openshell`,
+resolved workspace root or any of its parents. Nested workspace mounts remain
+valid. Mounts also must not overlap OpenShell supervisor files, `/etc/openshell`,
 `/etc/openshell-tls`, or `/run/netns`.
 
 Example named-volume usage: