feat(sync-plugin): forbid symlinks in plugin dirs/files

CHANGELOG.md

@@ -7,6 +7,10 @@ bump. Currently experimental: sync plugins.
 
 # Unreleased
 
+## Experimental
+
+* feat(sync-plugin): `plugin` sync steps now reject any `dirs`/`files` entry that is, or traverses, a symlink. Together with the existing relative-path and `..` checks, this keeps a declared path from resolving to a target outside the canister directory. The restriction may be relaxed in a future release if a safe use case emerges.
+
 # v1.0.1
 
 * feat: `icp identity import` can now be used with a `--delegation` flag to import a delegated identity. This is most useful for containers or other internal-only delegations; for anything involving a network, `icp identity delegation request` remains the recommended way to work with delegations.

crates/icp-cli/tests/sync_tests.rs

@@ -465,6 +465,110 @@ async fn sync_plugin_registers_seed_data() {
         );
 }
 
+/// A `dirs:` entry that is a symlink (here pointing outside the project) is
+/// rejected before the plugin runs, so a preopen cannot escape the canister
+/// directory. Symlinks are forbidden outright for now — see
+/// `crates/icp-sync-plugin/DESIGN.md`.
+#[cfg(unix)]
+#[tokio::test]
+async fn sync_plugin_rejects_symlinked_dir() {
+    let ctx = TestContext::new();
+    let project_dir = ctx.create_project_dir("icp");
+
+    let (canister_wasm, plugin_wasm) = build_sync_plugin_example();
+
+    // A real directory *outside* the project, and a symlink to it inside the
+    // project that the manifest declares as a `dirs:` entry.
+    let outside = ctx.home_path().join("outside-seed-data");
+    create_dir_all(&outside).expect("failed to create outside dir");
+    write_string(&outside.join("fruit-01.txt"), "apple").expect("failed to write fruit-01.txt");
+    std::os::unix::fs::symlink(&outside, project_dir.join("seed-data"))
+        .expect("failed to create symlink");
+
+    let pm = formatdoc! {r#"
+        canisters:
+          - name: my-canister
+            build:
+              steps:
+                - type: script
+                  command: cp '{canister_wasm}' "$ICP_WASM_OUTPUT_PATH"
+            sync:
+              steps:
+                - type: plugin
+                  path: {plugin_wasm}
+                  dirs:
+                    - seed-data
+
+        {NETWORK_RANDOM_PORT}
+        {ENVIRONMENT_RANDOM_PORT}
+    "#};
+    write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest");
+
+    let _g = ctx.start_network_in(&project_dir, "random-network").await;
+    ctx.ping_until_healthy(&project_dir, "random-network");
+
+    clients::icp(&ctx, &project_dir, Some("random-environment".to_string()))
+        .mint_cycles(10 * TRILLION);
+
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args(["deploy", "--environment", "random-environment"])
+        .assert()
+        .failure()
+        .stderr(contains("symlink").and(contains("seed-data")));
+}
+
+/// A `files:` entry that is a symlink (here pointing outside the project) is
+/// rejected before the host reads it, so a read cannot escape the canister
+/// directory.
+#[cfg(unix)]
+#[tokio::test]
+async fn sync_plugin_rejects_symlinked_file() {
+    let ctx = TestContext::new();
+    let project_dir = ctx.create_project_dir("icp");
+
+    let (canister_wasm, plugin_wasm) = build_sync_plugin_example();
+
+    // A real file *outside* the project, and a symlink to it inside the project
+    // that the manifest declares as a `files:` entry.
+    let outside = ctx.home_path().join("outside-secret.txt");
+    write_string(&outside, "secret").expect("failed to write outside file");
+    std::os::unix::fs::symlink(&outside, project_dir.join("config.txt"))
+        .expect("failed to create symlink");
+
+    let pm = formatdoc! {r#"
+        canisters:
+          - name: my-canister
+            build:
+              steps:
+                - type: script
+                  command: cp '{canister_wasm}' "$ICP_WASM_OUTPUT_PATH"
+            sync:
+              steps:
+                - type: plugin
+                  path: {plugin_wasm}
+                  files:
+                    - config.txt
+
+        {NETWORK_RANDOM_PORT}
+        {ENVIRONMENT_RANDOM_PORT}
+    "#};
+    write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest");
+
+    let _g = ctx.start_network_in(&project_dir, "random-network").await;
+    ctx.ping_until_healthy(&project_dir, "random-network");
+
+    clients::icp(&ctx, &project_dir, Some("random-environment".to_string()))
+        .mint_cycles(10 * TRILLION);
+
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args(["deploy", "--environment", "random-environment"])
+        .assert()
+        .failure()
+        .stderr(contains("symlink").and(contains("config.txt")));
+}
+
 #[tokio::test]
 async fn sync_script_icp_env_vars() {
     let ctx = TestContext::new();

crates/icp-sync-plugin/Cargo.toml

@@ -23,5 +23,8 @@ wasmtime-wasi.workspace = true
 [build-dependencies]
 camino.workspace = true
 
+[dev-dependencies]
+camino-tempfile.workspace = true
+
 [lints]
 workspace = true

crates/icp-sync-plugin/DESIGN.md

@@ -63,7 +63,7 @@ pub fn run_plugin(
     wasm_path: Utf8PathBuf,
     base_dir: Utf8PathBuf,
     dirs: Vec<String>,
-    files: Vec<(String, String)>,
+    files: Vec<String>,
     target_canister_id: Principal,
     agent: Agent,
     proxy: Option<Principal>,
@@ -73,11 +73,32 @@ pub fn run_plugin(
 ) -> Result<Vec<String>, RunPluginError>
 ```
 
-`dirs` and `files` come directly from the manifest adapter. The runtime preopens
-each `dir` from `base_dir.join(dir)` and passes `files` inline in
-`SyncExecInput`. The returned `Vec<String>` is the plugin's persistent stderr
-lines (see stdio capture below); `stdio`, when set, receives the rolling
-progress lines live.
+`dirs` and `files` are the manifest-relative path strings, straight from the
+adapter. The runtime owns *all* filesystem access anchored at `base_dir`: it
+preopens each `dir` from `base_dir.join(dir)` and reads each `file` from
+`base_dir.join(file)`, passing the contents inline in `SyncExecInput`. Keeping
+both inside the runtime means the path-safety logic (below) lives in one place
+and stays private to this crate — the CLI just forwards strings. The returned
+`Vec<String>` is the plugin's persistent stderr lines (see stdio capture below);
+`stdio`, when set, receives the rolling progress lines live.
+
+### Declared-path safety (no symlinks)
+
+Declared `dirs`/`files` entries are resolved on the host *before* the WASI
+sandbox boundary, so a lexical "relative, no `..`" check is not enough on two
+counts. First, a Windows drive-relative path such as `C:foo` carries a `Prefix`
+component yet is not "absolute", so joining it would discard `base_dir`;
+`escapes_base` (in `path.rs`) rejects `..`, root, and drive-prefix components,
+mirroring the bundler's checks. Second, a declared entry that *is* a symlink —
+or that traverses a symlinked parent component — would let a preopen or a read
+resolve outside the canister directory; `first_symlink_component` walks each
+component of the declared path under `base_dir` and rejects the entry if any
+prefix is a symlink (returning the offending sub-path relative to `base_dir`, so
+errors don't leak absolute on-disk paths). Both helpers are crate-private and
+applied uniformly to `dirs` and `files`. Symlinks are forbidden outright for
+now; the restriction can be relaxed later if a safe use case emerges. (Symlinks
+*inside* a preopen that escape it are a separate concern, already rejected by
+the WASI sandbox — cap-std — at runtime.)
 
 ### `HostState` and bindgen
 
@@ -145,5 +166,7 @@ pub struct Adapter {
 ### `crates/icp/src/canister/sync/plugin.rs`
 
 Resolves the wasm (local read or remote HTTP fetch into the package cache),
-verifies sha256, reads the inline `files` (rejecting absolute or `..` paths),
-then calls `icp_sync_plugin::run_plugin(...)`.
+verifies sha256, then calls `icp_sync_plugin::run_plugin(...)`, forwarding the
+manifest's `dirs`/`files` strings unchanged. The runtime — not the CLI — opens
+those paths and enforces the path-safety checks, so the CLI no longer touches
+the plugin's input files itself.

crates/icp-sync-plugin/src/lib.rs

@@ -1,3 +1,4 @@
+mod path;
 mod runtime;
 
 pub use runtime::{RunPluginError, run_plugin};

crates/icp-sync-plugin/src/path.rs

@@ -0,0 +1,177 @@
+//! Path-safety helpers used by the host runtime to validate declared `dirs`/`files`
+//! entries before preopening directories or reading files under the canister base dir.
+
+use camino::{Utf8Component, Utf8Path, Utf8PathBuf};
+
+/// Returns `true` if `rel` cannot be safely joined onto a base directory
+/// because it contains a component that would escape it: `..`, a filesystem
+/// root, or a (Windows) drive prefix such as `C:` — the latter makes a path
+/// drive-relative even without a leading separator, so `is_absolute()` returns
+/// `false` yet joining it discards the base. Mirrors the escape checks in the
+/// bundler (`crates/icp-cli/src/operations/bundle.rs`).
+///
+/// Callers reject such paths before resolving them; `first_symlink_component`
+/// only inspects `Normal` components and so would not otherwise catch these.
+pub(crate) fn escapes_base(rel: &str) -> bool {
+    Utf8Path::new(rel).components().any(|c| {
+        matches!(
+            c,
+            Utf8Component::ParentDir | Utf8Component::RootDir | Utf8Component::Prefix(_)
+        )
+    })
+}
+
+/// Walks `rel` one component at a time under `base` and returns the first
+/// sub-path of `rel` (relative to `base`) that is a symlink, if any.
+///
+/// Declared `dirs`/`files` entries are resolved on the host *before* the WASI
+/// sandbox boundary, so a symlinked entry — or an entry that traverses a
+/// symlinked directory — would let a preopen or a host read escape `base` to an
+/// arbitrary location on disk (the lexical [`escapes_base`] check does not catch
+/// this). Rejecting any symlink in the declared portion keeps every preopen and
+/// read anchored within `base`. Symlinks *inside* a preopen that escape it are
+/// separately rejected by the WASI sandbox (cap-std) at runtime.
+///
+/// The returned path is relative to `base` (e.g. `link` or `link/inner`),
+/// matching what the user wrote in the manifest, so it can be surfaced in an
+/// error without leaking the absolute on-disk location.
+///
+/// `base` itself may be reached through symlinks (e.g. the project lives under
+/// a symlinked path); only the declared relative portion is checked.
+///
+/// `rel` is expected to be relative and free of `..` (callers validate that via
+/// [`escapes_base`] first); `.` components are ignored. Components that do not
+/// exist are not symlinks, so a missing path returns `None` and the subsequent
+/// read or preopen surfaces the not-found error.
+pub(crate) fn first_symlink_component(base: &Utf8Path, rel: &str) -> Option<Utf8PathBuf> {
+    let mut host = base.to_path_buf();
+    let mut relative = Utf8PathBuf::new();
+    for component in Utf8Path::new(rel).components() {
+        if let Utf8Component::Normal(name) = component {
+            host.push(name);
+            relative.push(name);
+            match std::fs::symlink_metadata(host.as_std_path()) {
+                Ok(meta) if meta.file_type().is_symlink() => return Some(relative),
+                _ => {}
+            }
+        }
+    }
+    None
+}
+
+#[cfg(test)]
+mod escapes_base_tests {
+    use super::*;
+
+    #[test]
+    fn plain_relative_paths_are_safe() {
+        assert!(!escapes_base("a/b"));
+        assert!(!escapes_base("./a"));
+        assert!(!escapes_base("a/b/file.txt"));
+    }
+
+    #[test]
+    fn parent_and_root_components_escape() {
+        assert!(escapes_base("../a"));
+        assert!(escapes_base("a/../b"));
+        // An absolute path carries a `RootDir` component on every platform.
+        assert!(escapes_base("/abs"));
+    }
+
+    // On Windows a drive-relative path like `C:foo` has a `Prefix` component
+    // yet is NOT absolute, so an `is_absolute()` check alone would admit it and
+    // joining it onto a base would discard the base. `escapes_base` must reject
+    // it. (On Unix the same string is just an ordinary filename — see below.)
+    #[cfg(windows)]
+    #[test]
+    fn windows_drive_and_unc_prefixes_escape() {
+        assert!(escapes_base("C:foo")); // drive-relative (prefix, no root)
+        assert!(escapes_base(r"C:\foo")); // absolute (prefix + root)
+        assert!(escapes_base(r"\\server\share\x")); // UNC prefix
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn unix_treats_drive_prefix_as_a_plain_name() {
+        // There is no `Prefix` parsing on Unix, so `C:foo` is just a (weird)
+        // filename with no escaping component.
+        assert!(!escapes_base("C:foo"));
+    }
+}
+
+#[cfg(all(test, unix))]
+mod symlink_tests {
+    use super::*;
+    use std::os::unix::fs::symlink;
+
+    use camino_tempfile::tempdir;
+
+    #[test]
+    fn plain_relative_path_has_no_symlink() {
+        let tmp = tempdir().unwrap();
+        let base = tmp.path();
+        std::fs::create_dir_all(base.join("a/b")).unwrap();
+        std::fs::write(base.join("a/b/file.txt"), b"hi").unwrap();
+
+        assert_eq!(first_symlink_component(base, "a/b"), None);
+        assert_eq!(first_symlink_component(base, "a/b/file.txt"), None);
+    }
+
+    #[test]
+    fn final_entry_is_symlink() {
+        let tmp = tempdir().unwrap();
+        let base = tmp.path();
+        std::fs::create_dir_all(base.join("real")).unwrap();
+        symlink(base.join("real"), base.join("link")).unwrap();
+
+        assert_eq!(
+            first_symlink_component(base, "link"),
+            Some(Utf8PathBuf::from("link"))
+        );
+    }
+
+    #[test]
+    fn intermediate_component_is_symlink() {
+        let tmp = tempdir().unwrap();
+        let base = tmp.path();
+        // base/real/inner exists; base/link -> base/real, so "link/inner"
+        // traverses a symlink even though "inner" itself is a real dir.
+        std::fs::create_dir_all(base.join("real/inner")).unwrap();
+        symlink(base.join("real"), base.join("link")).unwrap();
+
+        // The reported path is the offending sub-path relative to `base`,
+        // i.e. the symlinked component, not the trailing real directory.
+        assert_eq!(
+            first_symlink_component(base, "link/inner"),
+            Some(Utf8PathBuf::from("link"))
+        );
+    }
+
+    #[test]
+    fn missing_path_is_not_a_symlink() {
+        let tmp = tempdir().unwrap();
+        let base = tmp.path();
+        assert_eq!(first_symlink_component(base, "does/not/exist"), None);
+    }
+
+    #[test]
+    fn dot_components_are_ignored() {
+        let tmp = tempdir().unwrap();
+        let base = tmp.path();
+        std::fs::create_dir_all(base.join("a")).unwrap();
+        assert_eq!(first_symlink_component(base, "./a"), None);
+    }
+
+    #[test]
+    fn symlinked_base_is_allowed() {
+        // A symlink *above* the declared portion (i.e. reaching `base`) is fine;
+        // only components of `rel` are checked.
+        let tmp = tempdir().unwrap();
+        let real_base = tmp.path().join("real-base");
+        std::fs::create_dir_all(real_base.join("data")).unwrap();
+        let linked_base = tmp.path().join("linked-base");
+        symlink(&real_base, &linked_base).unwrap();
+
+        assert_eq!(first_symlink_component(&linked_base, "data"), None);
+    }
+}