diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a5bafb1b..309183e04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,13 @@ air-gapped signing, extensions # Unreleased +* feat: `--untrusted`, on `build`, `deploy` and `sync`, holds a project someone else handed over to what it may read and run. Every file the project reads — manifests, argument and environment files, local recipe templates, modules — must be under the project root and reached through no symlink. Each build step must be a pre-built module and each sync step a plugin, taken from a regular file in the project; the steps a recipe renders to are held to the same list. Anything else is refused before the command acts on it, naming what was found. What the CLI keeps under `.icp` is held to the same rule, so a symlink planted there is refused rather than written through. * chore: macOS on Intel (`x86_64-apple-darwin`) is no longer a release target. Releases ship no Intel macOS archive, the shell installer refuses that platform, and npm no longer publishes `@icp-sdk/icp-cli-darwin-x64`, so installing `@icp-sdk/icp-cli` on an Intel Mac warns of an unsupported platform. Homebrew dropped Intel macOS in September 2026, and GitHub is retiring its Intel macOS runners. v1.8.0 is the last release with an Intel macOS build. +## Experimental + +* fix(bundle): `icp project bundle` holds only regular files and directories. A symlink inside a plugin's `files:` or `dirs:` directory used to be recorded as a link with its target unchecked, so one pointing outside the project reached that file on whatever machine unpacked the bundle. A link to somewhere inside the project is now copied as what it points at; one that leads outside the project, dangles, or leads back to a directory being copied fails the bundle, naming the link and where it leads. The same holds for a `files:` entry that is itself a symlink. + # v1.8.0 * fix: `icp sync` no longer needs controller rights. Its check that each canister is running used `canister_status`, which only a controller or a listed status viewer may read, so an identity the canister merely authorizes (a developer preparing an SNS-owned frontend for a commit proposal, say) was refused before anything was uploaded. It now probes with a query, as `icp deploy` already does after starting a canister. A stopped canister is still refused with the same advice, after the probe's 30-second wait rather than at once. diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index b0dc0bf0d..b547b4964 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -12,7 +12,13 @@ use icp_project::store_artifact::ArtifactStore; use std::time::Duration; use crate::{agent, identity, identity::PasswordFunc}; -use icp_project::{Lazy, ProjectLoadImpl, host::Host, manifest, store_id}; +use icp_project::{ + Lazy, ProjectLoad, ProjectLoadImpl, + files::{Confined, FileSystem, HostFileSystem, RootNotAbsolute}, + host::Host, + manifest::{self, ProjectRootLocate as _}, + store_id, untrusted, +}; #[derive(Debug, Snafu)] pub enum ContextInitError { @@ -36,13 +42,25 @@ pub enum ContextInitError { PackageCache { source: icp_project::fs::lock::LockError, }, + + #[snafu(display("failed to locate the project to treat as untrusted"))] + UntrustedRoot { + source: manifest::ProjectRootLocateError, + }, + + #[snafu(display("failed to confine the untrusted project to its directory"))] + Confine { source: RootNotAbsolute }, } +/// With `untrusted`, the project is read through a filesystem confined to +/// its root and vetted by [`icp_project::untrusted`] before any command acts +/// on it. The root is located up front for that, so there has to be a project. pub fn initialize( project_root_override: Option, debug: bool, password_func: PasswordFunc, pem_session_duration: Option, + untrusted: bool, ) -> Result { // Setup global directory structure let dirs = Arc::new(Directories::new().context(DirectoriesSnafu)?); @@ -75,16 +93,35 @@ pub fn initialize( let cwd: PathBuf = PathBuf::try_from(current_dir().context(CwdSnafu)?).context(Utf8PathSnafu)?; + // A relative override is taken from the working directory: the + // filesystem an untrusted project is confined to matches every path + // against the root as spelled, so the root has to be a whole path. + let project_root_override = project_root_override.map(|dir| cwd.join(dir)); let project_root_locate = Arc::new(manifest::ProjectRootLocateImpl::new( cwd, project_root_override, )); + // An untrusted project is confined to its directory: what it reads, and + // what the CLI keeps under `.icp` on its behalf. + let confined = if untrusted { + let root = project_root_locate.locate().context(UntrustedRootSnafu)?; + Some(Confined::new(&root).context(ConfineSnafu)?) + } else { + None + }; + // Canister ID Store - let ids = Arc::new(store_id::AccessImpl::new(project_root_locate.clone())); + let ids = Arc::new(store_id::AccessImpl::new( + project_root_locate.clone(), + confined.clone(), + )); // Canister Artifact Store (wasm) - let artifacts = Arc::new(ArtifactStore::new(project_root_locate.clone())); + let artifacts = Arc::new(ArtifactStore::new( + project_root_locate.clone(), + confined.clone(), + )); // Prepare http client let http_client = reqwest::Client::new(); @@ -100,15 +137,20 @@ pub fn initialize( pkg_cache.clone(), )); + // The project's files come from this machine: the whole of it for the + // user's own project, the project directory alone for an untrusted one. + let files: Arc = match &confined { + Some(confined) => Arc::new(confined.clone()), + None => Arc::new(HostFileSystem), + }; + // Recipes let recipe = Arc::new(RecipeFetcher { http_client, pkg_cache, + files: files.clone(), }); - // The project's files come from this machine. - let files = Arc::new(icp_project::files::HostFileSystem); - // Canister builder let builder = Arc::new(Builder::new(wasm.clone(), files.clone())); @@ -125,8 +167,11 @@ pub fn initialize( files: files.clone(), }; - let pload = Lazy::new(pload); - let pload = Arc::new(pload); + let pload: Arc = if untrusted { + Arc::new(Lazy::new(untrusted::Vetted::new(pload, files.clone()))) + } else { + Arc::new(Lazy::new(pload)) + }; // Telemetry data bag (written by subsystems, read at session finish) let telemetry_data = Arc::new(crate::telemetry_data::TelemetryData::default()); @@ -152,6 +197,7 @@ pub fn initialize( // Network accessor let netaccess = Arc::new(crate::network::Accessor { project_root_locate: project_root_locate.clone(), + confined, descriptors: dirs.port_descriptor(), agent: agent_creator.clone(), }); diff --git a/crates/icp-app/src/network/accessor.rs b/crates/icp-app/src/network/accessor.rs index a97d770b5..075a43ddf 100644 --- a/crates/icp-app/src/network/accessor.rs +++ b/crates/icp-app/src/network/accessor.rs @@ -8,6 +8,7 @@ use candid::Principal; use snafu::{ResultExt, Snafu}; use url::Url; +use icp_project::files::{Confined, FsError}; use icp_project::manifest::{ProjectRootLocate, ProjectRootLocateError}; use icp_project::network::{ Access, AccessError, CollectFriendlyDomains, Configuration, NetworkAccess, NetworkUrls, @@ -26,6 +27,9 @@ use crate::network::{ pub enum LocateNetworkDirectoryError { #[snafu(display("failed to find project root"))] ProjectRootLocate { source: ProjectRootLocateError }, + + #[snafu(display("network '{network}' keeps its state where an untrusted project may not"))] + Confine { source: FsError, network: String }, } /// Where a network keeps its on-disk state. @@ -57,6 +61,10 @@ pub struct Accessor { // Project root pub project_root_locate: Arc, + // What an untrusted project is confined to, which its network + // directories are then held to as well + pub confined: Option, + // Port descriptors dir pub descriptors: PathBuf, @@ -74,14 +82,20 @@ impl Accessor { .project_root_locate .locate() .context(ProjectRootLocateSnafu)?; - Ok(NetworkDirectory::new( + let nd = NetworkDirectory::new( &network.name, &dir.join(ICP_BASE) .join(CACHE_DIR) .join("networks") .join(&network.name), &self.descriptors, - )) + ); + if let Some(confined) = &self.confined { + nd.held_by(confined).context(ConfineSnafu { + network: &network.name, + })?; + } + Ok(nd) } } diff --git a/crates/icp-app/src/network/directory.rs b/crates/icp-app/src/network/directory.rs index 2018feec9..6344ea04b 100644 --- a/crates/icp-app/src/network/directory.rs +++ b/crates/icp-app/src/network/directory.rs @@ -168,6 +168,21 @@ impl NetworkDirectory { Ok(()) } + /// Refuse the files this directory is read through — its lock and its + /// descriptor — unless `confined`, the filesystem an untrusted project is + /// held to, would give them. + pub(crate) fn held_by( + &self, + confined: &icp_project::files::Confined, + ) -> Result<(), icp_project::files::FsError> { + let paths = NetworkRootPaths { + network_root: self.network_root.clone(), + }; + confined.resolve(&paths.lock_file())?; + confined.resolve(&paths.network_descriptor_path())?; + Ok(()) + } + /// Locked directory access for the local network root. pub fn root(&self) -> Result, LockError> { DirectoryStructureLock::open_or_create(NetworkRootPaths { diff --git a/crates/icp-app/src/recipe.rs b/crates/icp-app/src/recipe.rs index 37b278e91..3bc3b1571 100644 --- a/crates/icp-app/src/recipe.rs +++ b/crates/icp-app/src/recipe.rs @@ -14,7 +14,7 @@ use crate::package::{ read_cached_uri_recipe, }; use icp_project::{ - fs::read, + files::{FileSystem, FsError}, manifest::recipe::{Recipe, RecipeType}, prelude::*, }; @@ -30,6 +30,8 @@ pub struct RecipeFetcher { pub http_client: reqwest::Client, /// Package cache for caching downloaded recipe templates pub pkg_cache: std::sync::Arc, + /// The project's files, which a local template is read through + pub files: std::sync::Arc, } /// The result of the fetch stage, as this resolver sees it. @@ -76,7 +78,7 @@ enum TemplateSource { #[derive(Debug, Snafu)] pub enum RecipeFetchError { #[snafu(display("failed to read local recipe template file"))] - ReadFile { source: icp_project::fs::IoError }, + ReadFile { source: FsError }, #[snafu(display("failed to decode UTF-8 string"))] DecodeUtf8 { source: FromUtf8Error }, @@ -133,7 +135,7 @@ impl RecipeFetcher { // Retrieve the template, using cache for remote/registry sources let (tmpl, should_cache) = match &tmpl_source { TemplateSource::LocalPath(path) => { - let bytes = read(path).context(ReadFileSnafu)?; + let bytes = self.files.read(path).await.context(ReadFileSnafu)?; (parse_bytes_to_string(bytes)?, false) } @@ -360,6 +362,7 @@ mod tests { RecipeFetcher { http_client: reqwest::Client::new(), pkg_cache: std::sync::Arc::new(crate::package::open(cache_dir.to_owned()).unwrap()), + files: std::sync::Arc::new(icp_project::files::HostFileSystem), } } diff --git a/crates/icp-cli/src/commands/build.rs b/crates/icp-cli/src/commands/build.rs index e6dcfc876..85b226709 100644 --- a/crates/icp-cli/src/commands/build.rs +++ b/crates/icp-cli/src/commands/build.rs @@ -9,7 +9,7 @@ use tracing::info; use icp_project::operations::build::build_many; use crate::{ - options::{EnvironmentOpt, arg_struct_change_help}, + options::{EnvironmentOpt, UntrustedOpt, arg_struct_change_help}, render::rendered, }; @@ -22,6 +22,9 @@ pub(crate) struct BuildArgs { #[command(flatten)] pub(crate) environment: BuildEnvironmentOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } arg_struct_change_help!( diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index 781814947..8e6e6377e 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -20,7 +20,7 @@ use tracing::info; use crate::options::EnvironmentOpt; use crate::{ commands::{args::ArgsOpt, canister::create}, - options::{IdentityOpt, arg_struct_change_help}, + options::{IdentityOpt, UntrustedOpt, arg_struct_change_help}, render::rendered, }; @@ -97,6 +97,9 @@ pub(crate) struct DeployArgs { /// `upgrade_args` in the manifest. #[command(flatten)] pub(crate) args_opt: ArgsOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } arg_struct_change_help!( diff --git a/crates/icp-cli/src/commands/sync.rs b/crates/icp-cli/src/commands/sync.rs index a82eba23a..fcb28e048 100644 --- a/crates/icp-cli/src/commands/sync.rs +++ b/crates/icp-cli/src/commands/sync.rs @@ -12,7 +12,7 @@ use tracing::info; use icp_project::operations::{readiness::wait_until_serving_queries, sync::sync_many}; use crate::{ - options::{EnvironmentOpt, IdentityOpt}, + options::{EnvironmentOpt, IdentityOpt, UntrustedOpt}, render::rendered, }; @@ -32,6 +32,9 @@ pub(crate) struct SyncArgs { #[command(flatten)] pub(crate) identity: IdentityOpt, + + #[command(flatten)] + pub(crate) untrusted: UntrustedOpt, } pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), anyhow::Error> { diff --git a/crates/icp-cli/src/complete.rs b/crates/icp-cli/src/complete.rs index be3e877ee..d181d91ce 100644 --- a/crates/icp-cli/src/complete.rs +++ b/crates/icp-cli/src/complete.rs @@ -66,6 +66,7 @@ fn context() -> Option<&'static Context> { false, Arc::new(|| Err("cannot prompt while completing".to_string())), None, + false, ) .ok() }) diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index 8cf2e5e64..c449b1f1a 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -214,6 +214,7 @@ async fn run() -> Result<(), Error> { cli.debug, password_func, pem_session_duration, + command.untrusted(), )?; // Extension commands are left out of telemetry: telemetry parses against @@ -266,6 +267,21 @@ enum Invocation { Extension(icp_app::extension::Installed, ArgMatches), } +impl Invocation { + /// Whether the command was asked to treat the project as untrusted. The + /// flag belongs to the commands that act on what a project says — build, + /// deploy, sync — but it is the context every command runs in that + /// applies it, so it is read off the command before that is composed. + fn untrusted(&self) -> bool { + match self { + Invocation::BuiltIn(Command::Build(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(Command::Deploy(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(Command::Sync(args)) => args.untrusted.untrusted, + Invocation::BuiltIn(_) | Invocation::Extension(..) => false, + } + } +} + /// Dispatch the command to its handler. async fn dispatch(ctx: &icp_app::context::Context, command: Command) -> Result<(), Error> { match command { diff --git a/crates/icp-cli/src/options.rs b/crates/icp-cli/src/options.rs index 7a3bd4446..5a477a3d5 100644 --- a/crates/icp-cli/src/options.rs +++ b/crates/icp-cli/src/options.rs @@ -40,6 +40,19 @@ impl From for IdentitySelection { } } +/// `--untrusted`, for a command that acts on what a project says: build, +/// deploy, sync. A deploy service handed a project by an agent is what asks +/// for it, and those are the commands it runs. +#[derive(Args, Clone, Debug, Default)] +pub(crate) struct UntrustedOpt { + /// Hold the project to what one someone else handed over may do: every + /// file it reads is under the project root and reached through no + /// symlink, each build step is a pre-built module and each sync step a + /// plugin. Anything else is refused + #[arg(long)] + pub(crate) untrusted: bool, +} + #[derive(Args, Clone, Debug, Default)] #[clap(group(ArgGroup::new("environment-select").multiple(false)))] pub(crate) struct EnvironmentOpt { diff --git a/crates/icp-cli/tests/bundle_tests.rs b/crates/icp-cli/tests/bundle_tests.rs index 669c38556..31d71b4c5 100644 --- a/crates/icp-cli/tests/bundle_tests.rs +++ b/crates/icp-cli/tests/bundle_tests.rs @@ -964,6 +964,390 @@ fn bundle_rejects_source_outside_project() { .stderr(contains("my-canister").and(contains("outside the project directory"))); } +/// A project with one canister whose plugin sync step declares `files: +/// seed: `, for the symlink tests: what `data/` holds is up to each. +#[cfg(unix)] +fn plugin_project_with_files_entry(ctx: &TestContext, entry: &str) -> PathBuf { + let project_dir = ctx.create_project_dir("icp"); + let wasm_src = ctx.make_asset("example_icp_mo.wasm"); + write(&project_dir.join("plugin.wasm"), b"\x00asm\x01\x00\x00\x00") + .expect("failed to write plugin wasm"); + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{wasm_src}' "$ICP_WASM_OUTPUT_PATH" + sync: + steps: + - type: plugin + path: plugin.wasm + files: + seed: {entry} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + project_dir +} + +/// Every entry of the archive at `path`, as its archive path, entry type and +/// contents. +#[cfg(unix)] +fn archive_entries(path: &Path) -> Vec<(String, tar::EntryType, Vec)> { + let bytes = fs::read(path.as_std_path()).expect("failed to read bundle"); + let gz = GzDecoder::new(BufReader::new(bytes.as_slice())); + let mut archive = Archive::new(gz); + let mut out = Vec::new(); + for entry in archive.entries().expect("failed to read archive entries") { + let mut entry = entry.expect("failed to read archive entry"); + let path = entry + .path() + .expect("failed to get entry path") + .to_string_lossy() + .into_owned(); + let kind = entry.header().entry_type(); + let mut data = Vec::new(); + entry.read_to_end(&mut data).expect("failed to read entry"); + out.push((path, kind, data)); + } + out +} + +/// A symlink inside a plugin directory that points somewhere inside the +/// project is copied into the bundle as what it points at — a file, or a whole +/// directory — so the archive holds regular files only and a project may link +/// to its own files freely. +#[cfg(unix)] +#[test] +fn bundle_copies_symlinks_inside_the_project_as_what_they_point_at() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + create_dir_all(&project_dir.join("common")).expect("failed to create dir"); + write_string(&project_dir.join("data/real.txt"), "real").expect("failed to write file"); + write_string(&project_dir.join("common/c.txt"), "common").expect("failed to write file"); + std::os::unix::fs::symlink("real.txt", project_dir.join("data/link.txt")) + .expect("failed to create symlink"); + std::os::unix::fs::symlink("../common", project_dir.join("data/shared")) + .expect("failed to create symlink"); + + let bundle_path = project_dir.join("bundle.tar.gz"); + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", bundle_path.as_str()]) + .assert() + .success(); + + let entries = archive_entries(&bundle_path); + assert!( + entries + .iter() + .all(|(_, kind, _)| !kind.is_symlink() && !kind.is_hard_link()), + "the archive holds a link: {:?}", + entries.iter().map(|(p, k, _)| (p, *k)).collect::>() + ); + for (path, contents) in [ + ("plugins/my-canister/0/files/data/real.txt", "real"), + ("plugins/my-canister/0/files/data/link.txt", "real"), + ("plugins/my-canister/0/files/data/shared/c.txt", "common"), + ] { + let (_, kind, data) = entries + .iter() + .find(|(p, _, _)| p == path) + .unwrap_or_else(|| panic!("{path} missing from {entries:?}")); + assert!(kind.is_file(), "{path} is a {kind:?}"); + assert_eq!(data, contents.as_bytes(), "{path}"); + } +} + +/// A symlink inside a plugin directory that leads outside the project fails +/// the bundle, naming the link and where it leads, rather than being recorded +/// as a link for the unpacking machine to follow. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_pointing_outside_the_project() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + let outside = ctx.home_path().join("outside-secret.txt"); + write_string(&outside, "secret").expect("failed to write outside file"); + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink(&outside, project_dir.join("data/leak.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("leak.txt") + .and(contains("outside-secret.txt")) + .and(contains("outside the project directory")), + ); +} + +/// A symlink to nothing cannot be copied as what it points at, and is named +/// rather than skipped. +#[cfg(unix)] +#[test] +fn bundle_rejects_dangling_symlink() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink("nowhere.txt", project_dir.join("data/gone.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("gone.txt") + .and(contains("nowhere.txt")) + .and(contains("does not exist")), + ); +} + +/// A symlink back to a directory the bundle is in the middle of copying would +/// copy it without end; it is refused, naming the link. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_cycle() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data/inner")).expect("failed to create dir"); + std::os::unix::fs::symlink("../../data", project_dir.join("data/inner/back")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("back").and(contains("cycle"))); +} + +/// A symlink that loops cannot be resolved at all, and that is the reason +/// given, rather than the link being called dangling. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlink_loop() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + std::os::unix::fs::symlink("self", project_dir.join("data/self")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("cannot resolve") + .and(contains("data/self")) + .and(contains("does not exist").not()), + ); +} + +/// The output path is checked against the copied directories by its spelling, +/// which a symlink inside one of them can get around: the walk then meets the +/// bundle it is writing, and refuses it rather than copying a partial archive +/// into itself. +#[cfg(unix)] +#[test] +fn bundle_rejects_output_reached_through_symlink() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "assets"); + + create_dir_all(&project_dir.join("assets")).expect("failed to create dir"); + create_dir_all(&project_dir.join("shared")).expect("failed to create dir"); + write_string(&project_dir.join("shared/s.txt"), "s").expect("failed to write file"); + std::os::unix::fs::symlink("../shared", project_dir.join("assets/shared")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "shared/bundle.tar.gz"]) + .assert() + .failure() + .stderr( + contains("assets/shared/bundle.tar.gz") + .and(contains("own output file")) + .and(contains("still being written")), + ); +} + +/// The same when the output path is itself a dangling symlink into such a +/// directory: where the bundle is really written is known only once it is +/// created, and that is what the walk is told to refuse. +#[cfg(unix)] +#[test] +fn bundle_rejects_output_that_is_a_link_into_a_copied_directory() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "assets"); + + create_dir_all(&project_dir.join("assets")).expect("failed to create dir"); + create_dir_all(&project_dir.join("shared")).expect("failed to create dir"); + std::os::unix::fs::symlink("../shared", project_dir.join("assets/shared")) + .expect("failed to create symlink"); + std::os::unix::fs::symlink("shared/actual.tar.gz", project_dir.join("bundle.tar.gz")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("assets/shared/actual.tar.gz").and(contains("own output file"))); +} + +/// A `files:` entry that is itself a link to the output is refused like one +/// met on the walk, rather than read into the bundle half-written. +#[cfg(unix)] +#[test] +fn bundle_rejects_a_files_entry_that_links_to_the_output() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + std::os::unix::fs::symlink("bundle.tar.gz", project_dir.join("config.txt")) + .expect("failed to create symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("config.txt").and(contains("own output file"))); +} + +/// Nor is a `files:` entry that is not a regular file read: a named pipe +/// nobody writes to would hold the bundle up forever. +#[cfg(unix)] +#[test] +fn bundle_rejects_a_files_entry_that_is_not_a_regular_file() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + let made = std::process::Command::new("mkfifo") + .arg(project_dir.join("config.txt")) + .status() + .expect("mkfifo"); + assert!(made.success(), "mkfifo"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .timeout(std::time::Duration::from_secs(60)) + .assert() + .failure() + .stderr(contains("config.txt").and(contains("neither a file nor a directory"))); +} + +/// Each directory link is resolved from where the last one led, not from the +/// start again, so a chain of them longer than the host follows in one path +/// (40 on Linux) is still copied. +#[cfg(unix)] +#[test] +fn bundle_follows_a_long_chain_of_directory_links() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + const LINKS: usize = 45; + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + for i in 0..=LINKS { + create_dir_all(&project_dir.join(format!("chain/d{i}"))).expect("failed to create dir"); + } + std::os::unix::fs::symlink("../chain/d0", project_dir.join("data/next")) + .expect("failed to create symlink"); + for i in 0..LINKS { + std::os::unix::fs::symlink( + format!("../d{}", i + 1), + project_dir.join(format!("chain/d{i}/next")), + ) + .expect("failed to create symlink"); + } + write_string( + &project_dir.join(format!("chain/d{LINKS}/leaf.txt")), + "leaf", + ) + .expect("failed to write file"); + + let bundle_path = project_dir.join("bundle.tar.gz"); + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", bundle_path.as_str()]) + .assert() + .success(); + + let leaf = format!( + "plugins/my-canister/0/files/data/{}leaf.txt", + "next/".repeat(LINKS + 1) + ); + let entries = archive_entries(&bundle_path); + assert!( + entries + .iter() + .any(|(path, _, data)| *path == leaf && data == b"leaf"), + "{leaf} missing" + ); +} + +/// A file whose name is not UTF-8 cannot be named in the archive. The bundle +/// says so rather than leaving the file out, as the glob machinery elsewhere +/// does when it lists a directory. +#[cfg(target_os = "linux")] +#[test] +fn bundle_rejects_a_name_that_is_not_utf8() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "data"); + + create_dir_all(&project_dir.join("data")).expect("failed to create dir"); + let stray = project_dir + .join("data") + .into_std_path_buf() + .join(OsStr::from_bytes(b"caf\xe9.txt")); + fs::write(&stray, b"x").expect("failed to write file"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("not UTF-8").and(contains("data"))); +} + +/// A `files:` entry that is itself a symlink to a file outside the project is +/// refused like any other source outside the project, where the lexical check +/// before the build would have let it through and the read followed it. +#[cfg(unix)] +#[test] +fn bundle_rejects_symlinked_plugin_file_outside_the_project() { + let ctx = TestContext::new(); + let project_dir = plugin_project_with_files_entry(&ctx, "config.txt"); + + 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"); + + ctx.icp() + .current_dir(&project_dir) + .args(["project", "bundle", "--output", "bundle.tar.gz"]) + .assert() + .failure() + .stderr(contains("my-canister").and(contains("outside the project directory"))); +} + /// The environment reaching build steps as `ICP_CLI_ENVIRONMENT` defaults to `ic` /// for a bundle, rather than the `local` the rest of the CLI defaults to. #[test] diff --git a/crates/icp-cli/tests/untrusted_tests.rs b/crates/icp-cli/tests/untrusted_tests.rs new file mode 100644 index 000000000..38587b163 --- /dev/null +++ b/crates/icp-cli/tests/untrusted_tests.rs @@ -0,0 +1,368 @@ +//! `--untrusted` holds a project to what one from elsewhere may read and +//! run: see `icp_project::untrusted`. Each test here sets up a tree the CLI +//! would act on as the user's own, and shows the flag refusing it, naming +//! what it refused. + +use indoc::indoc; +use predicates::{prelude::PredicateBooleanExt, str::contains}; + +use crate::common::TestContext; +use icp_project::fs::write_string; + +mod common; + +/// A project directory holding a module of its own, and beside it a file +/// outside the project: `outside.bin`, and `outside.wasm`, a module. +fn project(ctx: &TestContext, manifest: &str) -> camino::Utf8PathBuf { + let project_dir = ctx.create_project_dir("icp"); + let wasm = ctx.make_asset("example_icp_mo.wasm"); + let outside = project_dir.parent().unwrap(); + std::fs::copy(&wasm, project_dir.join("backend.wasm")).expect("copy wasm"); + std::fs::copy(&wasm, outside.join("outside.wasm")).expect("copy wasm outside"); + std::fs::write(outside.join("outside.bin"), b"secret").expect("write outside"); + write_string(&project_dir.join("icp.yaml"), manifest).expect("write manifest"); + project_dir +} + +const PREBUILT: &str = indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: backend.wasm +"#}; + +const SCRIPT: &str = indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: script + command: cp backend.wasm "$ICP_WASM_OUTPUT_PATH" +"#}; + +#[test] +fn a_bundle_shaped_project_builds_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project(&ctx, PREBUILT); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .success(); +} + +#[test] +fn a_script_step_is_refused_as_untrusted_but_not_otherwise() { + let ctx = TestContext::new(); + let project_dir = project(&ctx, SCRIPT); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("more than an untrusted project may") + .and(contains( + "canister 'backend' has a build step that is not a pre-built module", + )) + .and(contains("(script) cp backend.wasm")), + ); +} + +/// A recipe is read and rendered like any other; it is the steps it renders +/// to that are held to the list. +#[test] +fn a_recipe_is_rendered_and_its_steps_held_to_the_list() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + recipe: + type: file://recipe.hbs + "#}, + ); + let recipe = project_dir.join("recipe.hbs"); + + std::fs::write( + &recipe, + "build:\n steps:\n - type: pre-built\n path: backend.wasm\n", + ) + .expect("write recipe"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .success(); + + std::fs::write( + &recipe, + "build:\n steps:\n - type: script\n command: cp backend.wasm \"$ICP_WASM_OUTPUT_PATH\"\n", + ) + .expect("write recipe"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("not a pre-built module").and(contains("(script) cp backend.wasm"))); +} + +/// What the project reads is held to its root: an argument file outside it +/// is read for the user's own project and refused for an untrusted one. +#[test] +fn a_file_read_from_outside_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + init_args: + path: ../outside.bin + format: bin + build: + steps: + - type: pre-built + path: backend.wasm + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("outside.bin").and(contains("is outside"))); +} + +/// A local recipe template is read through the same filesystem. +#[test] +fn a_recipe_template_outside_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + recipe: + type: file://../recipe.hbs + "#}, + ); + std::fs::write( + project_dir.parent().unwrap().join("recipe.hbs"), + "build:\n steps:\n - type: pre-built\n path: backend.wasm\n", + ) + .expect("write recipe"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("recipe.hbs").and(contains("is outside"))); +} + +#[test] +fn a_module_outside_the_project_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: ../outside.wasm + "#}, + ); + + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("names '") + .and(contains("outside.wasm")) + .and(contains("not a regular file in the project")), + ); +} + +/// A symlink is refused wherever it leads, out of the project or within it: +/// as the module a step names, and as a file the project reads. +#[cfg(unix)] +#[test] +fn a_symlink_is_refused_as_untrusted_wherever_it_leads() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + init_args: + path: args.bin + format: bin + build: + steps: + - type: pre-built + path: linked.wasm + "#}, + ); + let module = project_dir.join("linked.wasm"); + let args = project_dir.join("args.bin"); + let outside = project_dir.parent().unwrap().to_path_buf(); + std::fs::write(&args, b"args").expect("write args"); + + for target in [ + outside.join("outside.wasm"), + project_dir.join("backend.wasm"), + ] { + std::os::unix::fs::symlink(&target, &module).expect("symlink"); + ctx.icp() + .current_dir(&project_dir) + .args(["build"]) + .assert() + .success(); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("linked.wasm").and(contains("not a regular file in the project"))); + std::fs::remove_file(&module).expect("remove link"); + } + + std::fs::copy(project_dir.join("backend.wasm"), &module).expect("copy module"); + std::fs::remove_file(&args).expect("remove args"); + std::os::unix::fs::symlink(outside.join("outside.bin"), &args).expect("symlink"); + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("args.bin").and(contains("is a symlink"))); +} + +/// A path that mentions a symlink is refused even where a `..` after it +/// would, by the spelling alone, step back to a file of the project's own: +/// the host follows the link and takes `..` from where it leads. +#[cfg(unix)] +#[test] +fn a_path_that_steps_back_out_of_a_symlink_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project( + &ctx, + indoc! {r#" + canisters: + - name: backend + build: + steps: + - type: pre-built + path: jump/../backend.wasm + "#}, + ); + let elsewhere = project_dir.parent().unwrap().join("elsewhere/child"); + std::fs::create_dir_all(&elsewhere).expect("mkdir"); + std::os::unix::fs::symlink(&elsewhere, project_dir.join("jump")).expect("symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr( + contains("jump/../backend.wasm").and(contains("not a regular file in the project")), + ); +} + +/// What the CLI keeps under `.icp` is held to the project too: a symlink +/// planted where the built module is stored is refused, not written through. +#[cfg(unix)] +#[test] +fn a_symlink_under_dot_icp_is_refused_as_untrusted() { + let ctx = TestContext::new(); + let project_dir = project(&ctx, PREBUILT); + let victim = project_dir.parent().unwrap().join("victim"); + std::fs::write(&victim, b"mine").expect("write victim"); + let artifacts = project_dir.join(".icp/cache/artifacts"); + std::fs::create_dir_all(&artifacts).expect("mkdir"); + std::os::unix::fs::symlink(&victim, artifacts.join("backend")).expect("symlink"); + + ctx.icp() + .current_dir(&project_dir) + .args(["build", "--untrusted"]) + .assert() + .failure() + .stderr(contains("failed to save wasm artifact")); + assert_eq!(std::fs::read(&victim).expect("read victim"), b"mine"); +} + +/// The root may be spelled relatively: it is taken from the working directory. +#[test] +fn a_relative_project_root_is_taken_from_the_working_directory() { + let ctx = TestContext::new(); + let project_dir = project(&ctx, PREBUILT); + let parent = project_dir.parent().unwrap(); + + ctx.icp() + .current_dir(parent) + .args(["build", "--untrusted", "--project-root-override", "icp"]) + .assert() + .success(); + + ctx.icp() + .current_dir(parent) + .env("ICP_PROJECT_ROOT", "icp") + .args(["build", "--untrusted"]) + .assert() + .success(); +} + +/// The flag is on the three commands that act on what a project says, and +/// each refuses before it reaches any network. It is on no other command. +#[test] +fn the_flag_is_on_build_deploy_and_sync_alone() { + let ctx = TestContext::new(); + let project_dir = project(&ctx, SCRIPT); + + for command in ["build", "deploy", "sync"] { + ctx.icp() + .current_dir(&project_dir) + .args([command, "--untrusted"]) + .assert() + .failure() + .stderr(contains("more than an untrusted project may")); + } + + ctx.icp() + .current_dir(&project_dir) + .args(["identity", "list", "--untrusted"]) + .assert() + .failure() + .stderr(contains("unexpected argument '--untrusted'")); +} diff --git a/crates/icp-project/src/files.rs b/crates/icp-project/src/files.rs index 860cee6a9..e9b4936d8 100644 --- a/crates/icp-project/src/files.rs +++ b/crates/icp-project/src/files.rs @@ -88,6 +88,202 @@ pub trait Scratch: Send + Sync { fn path(&self) -> &Path; } +/// This machine's filesystem, held to one directory, for a project that is +/// not the user's own tree: one handed over by someone else. +/// +/// Every path must name something under the root, reached through no +/// symlink. A relative path is joined to the root, `.` is dropped, `..` is +/// applied, and a path that comes to rest outside the root is refused with +/// [`OutsideRoot`]. Each component below the root is then looked at as it is +/// on disk, and a path that passes through a symlink is refused with +/// [`ThroughLink`], wherever the link leads, even where a later `..` steps +/// back out of it. The predicates answer `false` for a refused path, and +/// scratch directories are made under the root. +/// +/// This confines what is read and written through it, and nothing else: a +/// `script` build step in a manifest still runs any command, so whoever +/// holds a tree they do not trust must also refuse such steps or sandbox them. +/// +/// The loader reads every path through the [`FileSystem`] it is given, so +/// this holds all of them. A local recipe template is the exception: the +/// [`Resolve`](crate::canister::recipe::Resolve) implementation reads it, and +/// must read it through this filesystem or refuse it. +/// +/// Known issue: a predicate can only answer `true` or `false`, so the reason +/// a path was refused is lost there, and a caller that checks before it +/// reads reports a refused path as missing. The loader does this for a +/// canister directory and for a dependency's manifest: `canisters: +/// [../elsewhere]` fails as "not found" rather than as outside the root. +/// +/// The root is absolute, and is matched as the caller spelled it, not as it +/// canonicalizes. It and everything above it are the caller's, and may be +/// reached through a link; only what lies below is held to shape. The tree +/// is taken to be the caller's own while it is used, with nothing else +/// rewriting it between the look and the read. +#[cfg(feature = "host")] +#[derive(Debug, Clone)] +pub struct Confined { + root: PathBuf, +} + +/// A root a [`Confined`] filesystem was asked to hold to that is not absolute. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +#[snafu(display( + "'{root}' is not an absolute path, which the directory a project is confined to must be" +))] +pub struct RootNotAbsolute { + pub root: PathBuf, +} + +/// A path a [`Confined`] filesystem was asked for that is not under its root. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +#[snafu(display("'{path}' is outside '{root}', the directory this project is confined to"))] +pub struct OutsideRoot { + pub path: PathBuf, + pub root: PathBuf, +} + +/// A path a [`Confined`] filesystem was asked for that passes through a +/// symlink. +#[cfg(feature = "host")] +#[derive(Debug, Snafu)] +#[snafu(display( + "'{path}' passes through '{link}', which is a symlink, and every path in this project must \ + name a regular file or directory under its root" +))] +pub struct ThroughLink { + pub path: PathBuf, + pub link: PathBuf, +} + +#[cfg(feature = "host")] +impl Confined { + /// Confine the host filesystem to `root`, which must be absolute. + pub fn new(root: &Path) -> Result { + snafu::ensure!(root.is_absolute(), RootNotAbsoluteSnafu { root }); + Ok(Self { + root: root.to_path_buf(), + }) + } + + /// The directory everything is held to. + pub fn root(&self) -> &Path { + &self.root + } + + /// Where `path` lies under the root: joined to the root if relative, + /// with `.` dropped and `..` applied, and refused if it would come to + /// rest outside, or passes through a symlink on the way. What a caller + /// that needs a real path for something that does not read through the + /// trait — a runtime mounting a directory — asks for. + pub fn resolve(&self, path: &Path) -> Result { + let outside = || { + FsError::new(OutsideRoot { + path: path.to_path_buf(), + root: self.root.clone(), + }) + }; + let relative = if path.is_absolute() { + path.strip_prefix(&self.root).map_err(|_| outside())? + } else { + path + }; + // Each component is looked at as it is on disk the moment it is + // named, before any `..` after it is applied: the host would follow a + // link and take `..` from where it leads, so a path that so much as + // mentions one is refused, dangling or not. + let mut resolved = self.root.clone(); + for component in relative.components() { + match component { + Utf8Component::Normal(part) => { + resolved.push(part); + if std::fs::symlink_metadata(&resolved) + .is_ok_and(|m| m.file_type().is_symlink()) + { + return Err(FsError::new(ThroughLink { + path: path.to_path_buf(), + link: resolved, + })); + } + } + Utf8Component::CurDir => {} + Utf8Component::ParentDir => { + if resolved == self.root || !resolved.pop() { + return Err(outside()); + } + } + Utf8Component::RootDir | Utf8Component::Prefix(_) => return Err(outside()), + } + } + Ok(resolved) + } +} + +#[cfg(feature = "host")] +#[async_trait] +impl FileSystem for Confined { + async fn read(&self, path: &Path) -> Result, FsError> { + HostFileSystem.read(&self.resolve(path)?).await + } + + async fn read_to_string(&self, path: &Path) -> Result { + HostFileSystem.read_to_string(&self.resolve(path)?).await + } + + async fn write(&self, path: &Path, contents: &[u8]) -> Result<(), FsError> { + HostFileSystem.write(&self.resolve(path)?, contents).await + } + + async fn create_dir_all(&self, path: &Path) -> Result<(), FsError> { + HostFileSystem.create_dir_all(&self.resolve(path)?).await + } + + async fn copy(&self, from: &Path, to: &Path) -> Result<(), FsError> { + HostFileSystem + .copy(&self.resolve(from)?, &self.resolve(to)?) + .await + } + + async fn exists(&self, path: &Path) -> bool { + match self.resolve(path) { + Ok(path) => HostFileSystem.exists(&path).await, + Err(_) => false, + } + } + + async fn is_file(&self, path: &Path) -> bool { + match self.resolve(path) { + Ok(path) => HostFileSystem.is_file(&path).await, + Err(_) => false, + } + } + + async fn is_dir(&self, path: &Path) -> bool { + match self.resolve(path) { + Ok(path) => HostFileSystem.is_dir(&path).await, + Err(_) => false, + } + } + + async fn read_dir(&self, path: &Path) -> Result, FsError> { + HostFileSystem.read_dir(&self.resolve(path)?).await + } + + async fn canonicalize(&self, path: &Path) -> Option { + HostFileSystem.canonicalize(&self.resolve(path).ok()?).await + } + + async fn scratch_dir(&self) -> Result, FsError> { + let dir = camino_tempfile::Builder::new() + .prefix(".scratch-") + .tempdir_in(&self.root) + .map_err(FsError::new)?; + Ok(Box::new(HostScratch(dir))) + } +} + #[cfg(feature = "host")] /// The [`FileSystem`] backed by this machine's filesystem. #[derive(Debug, Default, Clone, Copy)] @@ -385,6 +581,193 @@ async fn list( mod tests { use super::*; + /// Every spelling of a path resolves under the root or is refused: a + /// relative one is joined, `.` and `..` are applied, an absolute one is + /// taken only under the root, and none may pass through a symlink. + #[cfg(feature = "host")] + mod confined { + use super::*; + + fn root() -> (camino_tempfile::Utf8TempDir, Confined) { + let dir = camino_tempfile::Utf8TempDir::new().expect("temp dir"); + std::fs::create_dir_all(dir.path().join("project/canisters")).expect("mkdir"); + std::fs::write(dir.path().join("project/icp.yaml"), b"canisters: []").expect("write"); + std::fs::write(dir.path().join("project/canisters/a.wasm"), b"wasm").expect("write"); + std::fs::write(dir.path().join("outside.txt"), b"secret").expect("write"); + let fs = Confined::new(&dir.path().join("project")).expect("absolute"); + (dir, fs) + } + + #[tokio::test] + async fn paths_resolve_under_the_root_by_any_spelling_inside_it() { + let (_dir, fs) = root(); + let root = fs.root().to_path_buf(); + for spelling in [ + "canisters/a.wasm", + "./canisters/a.wasm", + "canisters/../canisters/a.wasm", + root.join("canisters/a.wasm").as_str(), + ] { + assert_eq!(fs.read(Path::new(spelling)).await.expect(spelling), b"wasm"); + assert!(fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert_eq!( + fs.canonicalize(Path::new(spelling)).await, + HostFileSystem + .canonicalize(&root.join("canisters/a.wasm")) + .await, + "{spelling}" + ); + } + assert!(fs.is_dir(Path::new("canisters")).await); + assert!(fs.is_dir(&root).await); + assert_eq!( + fs.read_dir(&root).await.expect("list"), + vec![root.join("canisters"), root.join("icp.yaml")] + ); + assert!( + fs.resolve(Path::new("made/later")) + .unwrap() + .starts_with(&root) + ); + } + + #[tokio::test] + async fn paths_outside_the_root_are_refused_however_spelled() { + let (dir, fs) = root(); + let outside = dir.path().join("outside.txt"); + assert!( + outside.is_file(), + "the file exists, for the host filesystem to read" + ); + for spelling in [ + "../outside.txt", + "canisters/../../outside.txt", + outside.as_str(), + "/etc/hostname", + "../../../../../../etc/hostname", + ] { + let err = fs + .read(Path::new(spelling)) + .await + .expect_err(spelling) + .to_string(); + assert!(err.contains("is outside"), "{spelling}: {err}"); + assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert!( + fs.canonicalize(Path::new(spelling)).await.is_none(), + "{spelling}" + ); + assert!(fs.resolve(Path::new(spelling)).is_err(), "{spelling}"); + } + // Nor may anything be written there. + let err = fs + .write(Path::new("../planted"), b"x") + .await + .unwrap_err() + .to_string(); + assert!(err.contains("is outside"), "{err}"); + assert!(!dir.path().join("planted").exists()); + } + + /// A temp dir on macOS is reached through a symlink (`/var`), so a + /// root compared by what it canonicalizes to would refuse every + /// absolute path spelled the way the caller got it. + #[cfg(unix)] + #[tokio::test] + async fn the_root_is_matched_as_the_caller_spells_it() { + let (dir, _) = root(); + std::os::unix::fs::symlink(dir.path().join("project"), dir.path().join("alias")) + .expect("symlink"); + let spelled = dir.path().join("alias"); + let fs = Confined::new(&spelled).expect("absolute"); + assert_eq!( + fs.read(&spelled.join("icp.yaml")).await.unwrap(), + b"canisters: []" + ); + assert_eq!( + fs.read(Path::new("icp.yaml")).await.unwrap(), + b"canisters: []" + ); + assert!(fs.is_dir(&spelled).await); + } + + #[test] + fn a_root_that_is_not_absolute_is_refused() { + let err = Confined::new(Path::new("project")).unwrap_err(); + assert!(err.to_string().contains("not an absolute path"), "{err}"); + } + + /// A path through a symlink is refused wherever the link leads: out + /// of the root, back inside it, or nowhere. A link in the middle of + /// a path counts as much as one at its end. + #[cfg(unix)] + #[tokio::test] + async fn a_path_through_a_symlink_is_refused_wherever_it_leads() { + use std::os::unix::fs::symlink; + let (dir, fs) = root(); + let project = fs.root().to_path_buf(); + symlink(dir.path().join("outside.txt"), project.join("out.txt")).expect("symlink"); + symlink(project.join("icp.yaml"), project.join("in.yaml")).expect("symlink"); + symlink(project.join("nowhere"), project.join("dangling")).expect("symlink"); + symlink(project.join("canisters"), project.join("dir-link")).expect("symlink"); + + for (spelling, link) in [ + ("out.txt", "out.txt"), + ("in.yaml", "in.yaml"), + ("dangling", "dangling"), + ("dir-link/a.wasm", "dir-link"), + ("./dir-link/../dir-link/a.wasm", "dir-link"), + // The host would take `..` from where the link leads; the + // spelling alone would cancel the link out and find the file. + ("dir-link/../canisters/a.wasm", "dir-link"), + ("dangling/../icp.yaml", "dangling"), + ] { + let err = fs + .read(Path::new(spelling)) + .await + .expect_err(spelling) + .to_string(); + assert!(err.contains("passes through"), "{spelling}: {err}"); + assert!(err.contains(link), "{spelling}: {err}"); + assert!(!fs.exists(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_file(Path::new(spelling)).await, "{spelling}"); + assert!(!fs.is_dir(Path::new(spelling)).await, "{spelling}"); + assert!( + fs.canonicalize(Path::new(spelling)).await.is_none(), + "{spelling}" + ); + } + // Nor may a link be written through. + let err = fs + .write(Path::new("out.txt"), b"x") + .await + .unwrap_err() + .to_string(); + assert!(err.contains("passes through"), "{err}"); + assert_eq!( + std::fs::read(dir.path().join("outside.txt")).unwrap(), + b"secret" + ); + } + + #[tokio::test] + async fn scratch_is_made_under_the_root() { + let (_dir, fs) = root(); + let scratch = fs.scratch_dir().await.expect("scratch"); + assert!(scratch.path().starts_with(fs.root())); + let out = scratch.path().join("out/module.wasm"); + fs.create_dir_all(&scratch.path().join("out")) + .await + .expect("mkdir"); + fs.write(&out, b"built").await.expect("write"); + assert_eq!(fs.read(&out).await.unwrap(), b"built"); + let path = scratch.path().to_path_buf(); + drop(scratch); + assert!(!path.exists()); + } + } + /// Builds a tree of empty files, creating parents as needed, and returns its /// root. fn tree(paths: &[&str]) -> camino_tempfile::Utf8TempDir { diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index ec17458fc..831620667 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -43,6 +43,7 @@ pub mod random; pub mod signal; pub mod store_artifact; pub mod store_id; +pub mod untrusted; /// The per-project state directory. `cache` holds what can be thrown away and /// rebuilt (ids for managed networks, build artifacts); `data` holds what @@ -219,6 +220,12 @@ pub enum ProjectLoadError { Project { source: project::ConsolidateManifestError, }, + + #[snafu(display("the project asks for more than an untrusted project may"))] + Untrusted { + #[snafu(source(from(untrusted::Refused, Box::new)))] + source: Box, + }, } #[async_trait] diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index e561f9641..af18a8f5a 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -248,6 +248,53 @@ pub enum BundleError { path: PathBuf, source: crate::files::FsError, }, + + #[snafu(display( + "'{path}' resolves to '{target}', outside the project directory '{root}'; a symlink \ + there points out of the project, and a bundle holds only what is inside it" + ))] + LinkEscapesProject { + path: PathBuf, + target: PathBuf, + root: PathBuf, + }, + + #[snafu(display("'{path}' is a symlink to '{target}', which does not exist"))] + DanglingLink { path: PathBuf, target: String }, + + #[snafu(display( + "'{path}' is a symlink back to '{target}', a directory the bundle is already copying; \ + a bundle cannot hold a cycle" + ))] + LinkCycle { path: PathBuf, target: PathBuf }, + + #[snafu(display("cannot resolve '{path}' to add it to the bundle"))] + ResolveArchiveEntry { + path: PathBuf, + source: std::io::Error, + }, + + #[snafu(display( + "'{path}' is neither a file nor a directory, which are all a bundle can hold" + ))] + UnsupportedEntry { path: PathBuf }, + + #[snafu(display("failed to list '{path}' to add it to the bundle"))] + ListDir { + path: PathBuf, + source: std::io::Error, + }, + + #[snafu(display( + "'{dir}' holds an entry named '{name}' that is not UTF-8, which a bundle cannot carry" + ))] + NameNotUtf8 { dir: PathBuf, name: String }, + + #[snafu(display( + "'{path}' is the bundle's own output file '{output}', reached through a symlink, and \ + it is still being written; choose an output path outside what the bundle copies" + ))] + OutputReachedThroughLink { path: PathBuf, output: PathBuf }, } /// In-memory bytes destined for a single tar entry. @@ -510,6 +557,7 @@ pub async fn create_bundle( write_archive( files, output, + &canonical_project_dir, &manifests, &bundle_artifacts, &args_files, @@ -1444,13 +1492,13 @@ struct ArchiveWriter { impl ArchiveWriter { fn new(inner: W) -> Self { let mut builder = Builder::new(inner); - // Record symlinks as Symlink entries rather than slurping their targets — keeps secrets - // outside the project from leaking via a symlinked asset dir. + // Every path handed to the builder is canonical, so there is no link left for it to + // follow; this is the backstop that keeps a link out of the archive should one get + // through, where it would otherwise be slurped as whatever it reaches. builder.follow_symlinks(false); - // Strip mtime/uid/gid from entry headers so they are metadata-normalized across machines. - // Note this does not make the archive fully byte-reproducible: `append_dir` relies on - // `append_dir_all`, which walks `read_dir` in the filesystem's order, so entry ordering - // within a directory can still differ between machines. + // Strip mtime/uid/gid from entry headers so they are metadata-normalized across + // machines, and `dir` appends a directory's entries in sorted order, so the same tree + // gives the same archive wherever it is bundled. builder.mode(tar::HeaderMode::Deterministic); Self { builder } } @@ -1468,19 +1516,89 @@ impl ArchiveWriter { } /// Appends a directory's whole contents, walking the host filesystem rather - /// than the [`FileSystem`] seam the rest of bundling reads through. + /// than the [`FileSystem`] seam the rest of bundling reads through, which + /// cannot say whether a path is a link. /// - /// `tar`'s walk is what keeps a symlink a symlink, per - /// [`new`](ArchiveWriter::new); the seam reports a link's target as the file - /// it points at and has no way to say otherwise, so reading through it would - /// slurp whatever the link reaches instead. Bundling from anything but a - /// host filesystem needs the seam to describe links first. - fn dir(&mut self, src_path: &Path, archive_prefix: &str) -> Result<(), BundleError> { + /// Every entry is resolved through its symlinks and must land under `root`, + /// the canonical project directory: it is then appended as what it points + /// at, in sorted order. A link that leads outside, dangles, leads back to + /// a directory being copied, or reaches `output` is refused by name. + fn dir( + &mut self, + src_path: &Path, + archive_prefix: &str, + root: &Path, + output: &Path, + ) -> Result<(), BundleError> { + let real = resolve_within_root(src_path, src_path, root)?; + self.walk( + src_path, + real, + archive_prefix, + root, + output, + &mut Vec::new(), + ) + } + + /// `real` is where `src_path` resolves, and `open` the canonical + /// directories on the way down to it, which tells a link back to one of + /// them from a second link to a directory already copied. + fn walk( + &mut self, + src_path: &Path, + real: PathBuf, + archive_path: &str, + root: &Path, + output: &Path, + open: &mut Vec, + ) -> Result<(), BundleError> { + if open.contains(&real) { + return LinkCycleSnafu { + path: src_path.to_path_buf(), + target: real, + } + .fail(); + } self.builder - .append_dir_all(archive_prefix, src_path.as_std_path()) + .append_dir(archive_path, real.as_std_path()) .context(WriteArchiveEntrySnafu { - path: PathBuf::from(archive_prefix), - }) + path: PathBuf::from(archive_path), + })?; + // Listed and resolved from where the directory really is, so the links + // crossed to get here are not followed again for every entry — a long + // enough chain of them is more than the host follows in one path. Each + // entry is still named by the path it was reached by, which is what a + // message about it should say. + let names = list_dir(&real)?; + open.push(real.clone()); + for name in names { + let entry = src_path.join(&name); + let entry_archive_path = format!("{archive_path}/{name}"); + let target = resolve_within_root(&real.join(&name), &entry, root)?; + let meta = std::fs::metadata(&target).context(ResolveArchiveEntrySnafu { + path: entry.clone(), + })?; + if meta.is_dir() { + self.walk(&entry, target, &entry_archive_path, root, output, open)?; + } else if target == output { + return OutputReachedThroughLinkSnafu { + path: entry, + output: output.to_path_buf(), + } + .fail(); + } else if meta.is_file() { + self.builder + .append_path_with_name(target.as_std_path(), &entry_archive_path) + .context(WriteArchiveEntrySnafu { + path: PathBuf::from(entry_archive_path), + })?; + } else { + return UnsupportedEntrySnafu { path: entry }.fail(); + } + } + open.pop(); + Ok(()) } fn into_inner(self) -> Result { @@ -1491,6 +1609,7 @@ impl ArchiveWriter { async fn write_archive( files: &dyn FileSystem, output: &Path, + canonical_project_dir: &Path, manifests: &[InstanceManifest], artifacts: &BundleArtifacts, args_files: &[ArgsFile], @@ -1528,6 +1647,10 @@ async fn write_archive( let file = File::create(output.as_std_path()).context(CreateOutputSnafu { path: output.to_path_buf(), })?; + // Resolved now that it exists, for the walk, which can reach the output + // through a symlink: before this it may itself have been a dangling link, + // whose target only creating it brought about. + let canonical_output = canonicalize(files, output).await?; let gz = GzEncoder::new(BufWriter::new(file), Compression::default()); let mut archive = ArchiveWriter::new(gz); @@ -1564,17 +1687,44 @@ async fn write_archive( } for d in &artifacts.plugin_dirs { - archive.dir(&d.src_path, &d.archive_prefix)?; + archive.dir( + &d.src_path, + &d.archive_prefix, + canonical_project_dir, + &canonical_output, + )?; } for pf in &artifacts.plugin_files { - let data = files - .read(&pf.src_path) - .await - .context(ReadPluginFileSnafu { - canister: pf.canister_name.clone(), - file: pf.orig_file.clone(), - })?; + // Validated lexically before the build, when it need not have existed; + // now it does, so a symlink in it is followed and held to the project + // like every other entry. + let src = canonicalize_within_project( + files, + &pf.src_path, + canonical_project_dir, + &pf.canister_name, + ) + .await?; + // Held to what the walk holds an entry to: not the bundle itself, and + // a regular file, since a pipe nobody writes to is read forever. + if src == canonical_output { + return OutputReachedThroughLinkSnafu { + path: pf.src_path.clone(), + output: canonical_output, + } + .fail(); + } + if !files.is_file(&src).await { + return UnsupportedEntrySnafu { + path: pf.src_path.clone(), + } + .fail(); + } + let data = files.read(&src).await.context(ReadPluginFileSnafu { + canister: pf.canister_name.clone(), + file: pf.orig_file.clone(), + })?; archive.bytes(&pf.archive_path, &data)?; } @@ -1819,6 +1969,65 @@ fn is_absolute_bind_mount_host(mount: &str) -> bool { !h.is_empty() && (h[0] == b'/' || h[0] == b'\\') } +/// The names in a directory, sorted. [`crate::fs::read_dir`] skips a name that +/// is not UTF-8, which would leave a file out of the bundle; here it is refused. +fn list_dir(dir: &Path) -> Result, BundleError> { + let mut names = Vec::new(); + for entry in std::fs::read_dir(dir).context(ListDirSnafu { path: dir })? { + let entry = entry.context(ListDirSnafu { path: dir })?; + let name = entry + .file_name() + .into_string() + .map_err(|name| BundleError::NameNotUtf8 { + dir: dir.to_path_buf(), + name: name.to_string_lossy().into_owned(), + })?; + names.push(name); + } + names.sort(); + Ok(names) +} + +/// Where `path` comes to rest with every symlink followed, which must be under +/// `root`. `named` is the path a message about it gives: the one it was reached +/// by. Walks the host directly, as [`ArchiveWriter::dir`] does: it is the +/// one place bundling has to know whether a path is a link, and the +/// [`FileSystem`] seam cannot say. +fn resolve_within_root(path: &Path, named: &Path, root: &Path) -> Result { + let real = match dunce::canonicalize(path) { + Ok(real) => real, + Err(source) => { + // A link whose target is missing is dangling; a link that loops, + // or leads somewhere unreadable, fails for its own reason, kept. + if source.kind() == std::io::ErrorKind::NotFound + && let Ok(target) = std::fs::read_link(path) + { + return DanglingLinkSnafu { + path: named.to_path_buf(), + target: target.display().to_string(), + } + .fail(); + } + return Err(BundleError::ResolveArchiveEntry { + path: named.to_path_buf(), + source, + }); + } + }; + let real = PathBuf::from_path_buf(real).map_err(|_| BundleError::CanonicalizePath { + path: named.to_path_buf(), + })?; + if !real.starts_with(root) { + return LinkEscapesProjectSnafu { + path: named.to_path_buf(), + target: real, + root: root.to_path_buf(), + } + .fail(); + } + Ok(real) +} + async fn canonicalize(files: &dyn FileSystem, path: &Path) -> Result { files .canonicalize(path) diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index e8f28cfbc..83eb475f7 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -1690,6 +1690,115 @@ pub async fn consolidate_manifest( }) } +/// A project loaded through a [`Confined`] filesystem reads nothing outside +/// its root, whatever path its manifest names: the loader resolves every +/// path it reads itself through the filesystem it is given, so the one rule +/// holds them all. (A local recipe template is read by the [`Resolve`] +/// implementation instead, which is why these tests supply none.) +#[cfg(all(test, feature = "host"))] +mod confined_tests { + use super::*; + use crate::canister::recipe::{Fetched, Resolve, ResolveError}; + use crate::files::{Confined, HostFileSystem}; + use crate::manifest::recipe::Recipe; + use camino_tempfile::Utf8TempDir; + + struct NoRecipes; + + #[async_trait::async_trait] + impl Resolve for NoRecipes { + async fn resolve(&self, _recipe: &Recipe) -> Result { + unreachable!("no manifest here names a recipe") + } + } + + /// A tree with a project directory and, beside it, what the project must + /// not reach: an argument file, and a canister directory of its own. + fn tree(manifest: &str) -> Utf8TempDir { + let dir = Utf8TempDir::new().expect("temp dir"); + let project = dir.path().join("project"); + std::fs::create_dir_all(&project).expect("mkdir"); + std::fs::write(project.join(PROJECT_MANIFEST), manifest).expect("write"); + std::fs::write(dir.path().join("outside.bin"), b"secret").expect("write"); + std::fs::create_dir_all(dir.path().join("elsewhere")).expect("mkdir"); + std::fs::write( + dir.path().join("elsewhere").join(CANISTER_MANIFEST), + "name: stray\nbuild:\n steps:\n - type: pre-built\n path: stray.wasm\n", + ) + .expect("write"); + dir + } + + async fn load(files: &dyn FileSystem, pdir: &Path) -> Result { + let m: ProjectManifest = load_manifest_from_path(files, &pdir.join(PROJECT_MANIFEST)) + .await + .map_err(|e| chain(&e))?; + consolidate_manifest(files, pdir, &NoRecipes, &m) + .await + .map_err(|e| chain(&e)) + } + + fn chain(e: &(dyn std::error::Error + 'static)) -> String { + let mut text = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + text.push_str(": "); + text.push_str(&cause.to_string()); + source = cause.source(); + } + text + } + + #[tokio::test] + async fn an_argument_file_outside_the_root_is_read_by_the_host_and_refused_confined() { + let dir = tree(indoc::indoc! {" + canisters: + - name: backend + init_args: + path: ../outside.bin + format: bin + build: + steps: + - type: pre-built + path: backend.wasm + "}); + let pdir = dir.path().join("project"); + + let project = load(&HostFileSystem, &pdir) + .await + .expect("the host reads anywhere"); + let (_, backend) = &project.canisters["backend"]; + assert_eq!( + backend.init_args, + Some(CanisterArgs::Binary(b"secret".to_vec())) + ); + + let confined = Confined::new(&pdir).expect("absolute"); + let err = load(&confined, &pdir) + .await + .expect_err("confined, the read is refused"); + assert!(err.contains("init_args"), "{err}"); + assert!(err.contains("is outside"), "{err}"); + } + + #[tokio::test] + async fn a_canister_directory_outside_the_root_is_found_by_the_host_and_not_confined() { + let dir = tree("canisters:\n - ../elsewhere\n"); + let pdir = dir.path().join("project"); + + let project = load(&HostFileSystem, &pdir) + .await + .expect("the host reads anywhere"); + assert!(project.canisters.contains_key("stray")); + + let confined = Confined::new(&pdir).expect("absolute"); + let err = load(&confined, &pdir) + .await + .expect_err("confined, the directory is not there"); + assert!(err.contains("../elsewhere"), "{err}"); + } +} + #[cfg(all(test, feature = "host"))] mod recipe_sync_tests { use super::*; diff --git a/crates/icp-project/src/store_artifact.rs b/crates/icp-project/src/store_artifact.rs index 707fe734c..30488a0db 100644 --- a/crates/icp-project/src/store_artifact.rs +++ b/crates/icp-project/src/store_artifact.rs @@ -6,6 +6,7 @@ use std::{collections::HashMap, sync::Mutex}; #[cfg(feature = "host")] use crate::{ CACHE_DIR, ICP_BASE, + files::{Confined, FsError}, fs::{ lock::{DirectoryStructureLock, PathsAccess}, read, write, @@ -66,6 +67,7 @@ pub enum LookupArtifactError { #[cfg(feature = "host")] pub struct ArtifactStore { project_root_locate: Arc, + confined: Option, } #[cfg(feature = "host")] @@ -124,23 +126,46 @@ impl PathsAccess for ArtifactPaths { #[cfg(feature = "host")] impl ArtifactStore { - pub fn new(project_root_locate: Arc) -> Self { + /// `confined` is given for a project that is not trusted: the store's + /// files are then held to it, so a symlink planted where the store keeps + /// one is refused rather than written or read through. + pub fn new( + project_root_locate: Arc, + confined: Option, + ) -> Self { Self { project_root_locate, + confined, } } - /// Locked directory access for the artifact store. It will create the directory if it does not exist. - fn lock(&self) -> Result, crate::fs::lock::LockError> { + fn paths(&self) -> ArtifactPaths { let project_root = self .project_root_locate .locate() .expect("failed to locate project root"); - let artifact_dir = project_root - .join(ICP_BASE) - .join(CACHE_DIR) - .join("artifacts"); - DirectoryStructureLock::open_or_create(ArtifactPaths { dir: artifact_dir }) + ArtifactPaths { + dir: project_root + .join(ICP_BASE) + .join(CACHE_DIR) + .join("artifacts"), + } + } + + /// Refuse the lock file and `name`'s artifact unless the filesystem the + /// project is confined to, if it is, would give them. + fn held(&self, name: &str) -> Result<(), FsError> { + if let Some(confined) = &self.confined { + let paths = self.paths(); + confined.resolve(&paths.lock_file())?; + confined.resolve(&paths.artifact_by_name(name))?; + } + Ok(()) + } + + /// Locked directory access for the artifact store. It will create the directory if it does not exist. + fn lock(&self) -> Result, crate::fs::lock::LockError> { + DirectoryStructureLock::open_or_create(self.paths()) } } @@ -181,6 +206,7 @@ impl Access for ArtifactStore { } .fail(); } + self.held(name).map_err(|e| save_store(name, e))?; self.lock() .map_err(|e| save_store(name, e))? .with_write(async |store| { @@ -198,6 +224,7 @@ impl Access for ArtifactStore { } .fail(); } + self.held(name).map_err(|e| lookup_store(name, e))?; self.lock() .map_err(|e| lookup_store(name, e))? .with_read(async |store| { @@ -301,3 +328,51 @@ mod tests { assert!(artifact_name_overflow(&deep).is_some()); } } + +#[cfg(all(test, unix, feature = "host"))] +mod confined_tests { + use super::*; + use crate::manifest::ProjectRootLocateImpl; + + /// A link planted where the store keeps an artifact, or its lock, is + /// refused when the project is confined, rather than written through. + #[tokio::test] + async fn an_artifact_behind_a_symlink_is_refused_when_confined() { + let dir = camino_tempfile::tempdir().unwrap(); + let root = dir.path().join("project"); + let artifacts = root.join(ICP_BASE).join(CACHE_DIR).join("artifacts"); + std::fs::create_dir_all(&artifacts).unwrap(); + std::fs::write(root.join("icp.yaml"), "canisters: []").unwrap(); + let victim = dir.path().join("victim"); + std::fs::write(&victim, "mine").unwrap(); + let locate = Arc::new(ProjectRootLocateImpl::new(root.clone(), Some(root.clone()))); + let store = ArtifactStore::new(locate, Some(Confined::new(&root).unwrap())); + + for planted in ["backend", ".lock"] { + let link = artifacts.join(planted); + std::os::unix::fs::symlink(&victim, &link).unwrap(); + let err = store.save("backend", b"wasm").await.unwrap_err(); + assert!( + matches!(err, SaveError::SaveStore { .. }), + "{planted}: {err}" + ); + let err = store.lookup("backend").await.unwrap_err(); + assert!( + matches!(err, LookupArtifactError::LookupStore { .. }), + "{planted}: {err}" + ); + assert_eq!( + std::fs::read_to_string(&victim).unwrap(), + "mine", + "{planted}" + ); + std::fs::remove_file(&link).unwrap(); + } + + store + .save("backend", b"wasm") + .await + .expect("no link, saved"); + assert_eq!(store.lookup("backend").await.unwrap(), b"wasm"); + } +} diff --git a/crates/icp-project/src/store_id.rs b/crates/icp-project/src/store_id.rs index f4e438df2..f63a84088 100644 --- a/crates/icp-project/src/store_id.rs +++ b/crates/icp-project/src/store_id.rs @@ -7,6 +7,7 @@ use crate::manifest::ProjectRootLocateError; #[cfg(feature = "host")] use crate::{ CACHE_DIR, DATA_DIR, ICP_BASE, + files::Confined, fs::{create_dir_all, json, remove_file}, manifest::ProjectRootLocate, prelude::*, @@ -161,17 +162,34 @@ impl std::error::Error for StoreCause { #[cfg(feature = "host")] pub struct AccessImpl { project_root_locate: Arc, + confined: Option, lock: Mutex<()>, } #[cfg(feature = "host")] impl AccessImpl { - pub fn new(project_root_locate: Arc) -> Self { + /// `confined` is given for a project that is not trusted: the store's + /// files are then held to it, so a symlink planted where the store keeps + /// one is refused rather than written or read through. + pub fn new( + project_root_locate: Arc, + confined: Option, + ) -> Self { Self { project_root_locate, + confined, lock: Mutex::new(()), } } + + /// Refuse `fpath` unless the filesystem the project is confined to, if + /// it is, would give it. + fn held(&self, fpath: &Path) -> Result<(), StoreCause> { + if let Some(confined) = &self.confined { + confined.resolve(fpath).map_err(StoreCause::new)?; + } + Ok(()) + } } #[cfg(feature = "host")] @@ -187,6 +205,11 @@ impl Access for AccessImpl { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; + self.held(&fpath) + .map_err(|source| RegisterError::RegisterStore { + source, + env: env.to_owned(), + })?; create_dir_all(fpath.parent().unwrap()).map_err(|e| RegisterError::RegisterStore { source: StoreCause::new(e), env: env.to_owned(), @@ -227,6 +250,11 @@ impl Access for AccessImpl { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; + self.held(&fpath) + .map_err(|source| UnregisterError::UnregisterStore { + source, + env: env.to_owned(), + })?; // Load the file let mut mapping = load_mapping(&fpath).map_err(|e| UnregisterError::UnregisterStore { @@ -254,6 +282,11 @@ impl Access for AccessImpl { ) -> Result { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; + self.held(&fpath) + .map_err(|source| LookupIdError::LookupStore { + source, + env: env.to_owned(), + })?; load_mapping(&fpath) .map_err(|e| LookupIdError::LookupStore { source: StoreCause::new(e), @@ -270,6 +303,11 @@ impl Access for AccessImpl { fn lookup_by_environment(&self, is_cache: bool, env: &str) -> Result { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; + self.held(&fpath) + .map_err(|source| LookupIdError::LookupStore { + source, + env: env.to_owned(), + })?; load_mapping(&fpath).map_err(|e| LookupIdError::LookupStore { source: StoreCause::new(e), env: env.to_owned(), @@ -279,6 +317,11 @@ impl Access for AccessImpl { fn cleanup(&self, is_cache: bool, env: &str) -> Result<(), CleanupError> { let _g = self.lock.lock().expect("failed to acquire id store lock"); let fpath = self.get_fpath_for_env(is_cache, env)?; + self.held(&fpath) + .map_err(|source| CleanupError::CleanupStore { + source, + env: env.to_owned(), + })?; if fpath.exists() { remove_file(&fpath).map_err(|e| CleanupError::CleanupStore { source: StoreCause::new(e), @@ -446,3 +489,34 @@ pub mod mock { } } } + +#[cfg(all(test, unix, feature = "host"))] +mod confined_tests { + use super::*; + use crate::manifest::ProjectRootLocateImpl; + + /// A link planted where the store keeps a mapping is refused when the + /// project is confined, rather than read or written through. + #[test] + fn a_mapping_behind_a_symlink_is_refused_when_confined() { + let dir = camino_tempfile::tempdir().unwrap(); + let root = dir.path().join("project"); + let mappings = root.join(ICP_BASE).join(DATA_DIR).join("mappings"); + std::fs::create_dir_all(&mappings).unwrap(); + std::fs::write(root.join("icp.yaml"), "canisters: []").unwrap(); + let victim = dir.path().join("victim.json"); + std::fs::write(&victim, "{}").unwrap(); + std::os::unix::fs::symlink(&victim, mappings.join("ic.ids.json")).unwrap(); + + let locate = Arc::new(ProjectRootLocateImpl::new(root.clone(), Some(root.clone()))); + let store = AccessImpl::new(locate, Some(Confined::new(&root).unwrap())); + + let err = store + .register(false, "ic", "backend", Principal::anonymous()) + .unwrap_err(); + assert!(matches!(err, RegisterError::RegisterStore { .. }), "{err}"); + let err = store.lookup(false, "ic", "backend").unwrap_err(); + assert!(matches!(err, LookupIdError::LookupStore { .. }), "{err}"); + assert_eq!(std::fs::read_to_string(&victim).unwrap(), "{}"); + } +} diff --git a/crates/icp-project/src/untrusted.rs b/crates/icp-project/src/untrusted.rs new file mode 100644 index 000000000..212db8f87 --- /dev/null +++ b/crates/icp-project/src/untrusted.rs @@ -0,0 +1,428 @@ +//! Holding a project someone else handed over to what it may read and run. +//! +//! The CLI trusts a project the way it trusts the user's own checkout: a +//! build step may run any command, a sync step any script, and a manifest +//! may name any path on the machine. A project from elsewhere — an archive +//! an agent passed to a deploy service, a checkout the user has not read — +//! may not. Two things hold it: +//! +//! - what it reads goes through a filesystem confined to the project root — +//! on a host, [`Confined`](crate::files::Confined) — so that every path a +//! manifest names is under the root and reached through no symlink. The +//! id and artifact stores under `.icp` are held to the same filesystem; +//! - what it runs is on a list, stated as what is allowed so that a kind of +//! step added later is refused until someone decides otherwise: a build +//! step is a pre-built module, a sync step is a plugin, and the file either +//! names is a regular file that filesystem will give. +//! +//! A module or plugin named by URL is not on the list, and the steps a recipe +//! renders to are held to it like any other. A checksum is not asked for: the +//! same untrusted hand wrote it and the module. [`vet`] applies the list to a +//! loaded [`Project`], and [`Vetted`] wraps a loader so that every load is +//! vetted before the project is handed out. + +use std::sync::Arc; + +use async_trait::async_trait; +use snafu::{ResultExt, Snafu, ensure}; + +use crate::{ + Project, ProjectLoad, ProjectLoadError, UntrustedSnafu, + files::FileSystem, + manifest::{ + adapter::prebuilt::{LocalSource, SourceField}, + canister::{BuildStep, SyncStep}, + }, + prelude::*, +}; + +/// Why a loaded project may not be acted on as untrusted. +#[derive(Debug, Snafu)] +pub enum Refused { + #[snafu(display( + "canister '{canister}' has a build step that is not a pre-built module, which is all an \ + untrusted project may have: {step}" + ))] + BuildStep { canister: String, step: String }, + + #[snafu(display( + "canister '{canister}' has a sync step that is not a plugin, which is all an untrusted \ + project may have: {step}" + ))] + SyncStep { canister: String, step: String }, + + #[snafu(display( + "canister '{canister}' names '{path}', which is not a regular file in the project: an \ + untrusted project runs only a file under its root, reached through no symlink" + ))] + NotAFile { canister: String, path: PathBuf }, +} + +/// The file `step` names, if it is the one build step an untrusted project +/// may have: a pre-built module from a file. +fn local_module(step: &BuildStep) -> Option<&LocalSource> { + match step { + BuildStep::Prebuilt(adapter) => local_source(&adapter.source), + BuildStep::Script(_) => None, + } +} + +/// The file `step` names, if it is the one sync step an untrusted project +/// may have: a plugin from a file. +fn local_plugin(step: &SyncStep) -> Option<&LocalSource> { + match step { + SyncStep::Plugin(adapter) => local_source(&adapter.source), + SyncStep::Script(_) => None, + } +} + +fn local_source(source: &SourceField) -> Option<&LocalSource> { + match source { + SourceField::Local(local) => Some(local), + SourceField::Remote(_) => None, + } +} + +/// Hold a loaded project to the list. +/// +/// `files` is the filesystem the project is confined to, and it is what +/// says whether the file a step names is one: under the root, through no +/// symlink, and a regular file. A plugin is opened by its runtime rather +/// than read through `files`, and a read of a pipe nobody writes to waits +/// forever, so the question is asked here, before anything opens the file. +pub async fn vet(project: &Project, files: &dyn FileSystem) -> Result<(), Refused> { + for (_, (canister_dir, canister)) in &project.canisters { + let build = canister.build.steps.iter().map(|step| { + local_module(step).ok_or_else(|| { + BuildStepSnafu { + canister: &canister.name, + step: one_line(step), + } + .build() + }) + }); + let sync = canister.sync.steps.iter().map(|step| { + local_plugin(step).ok_or_else(|| { + SyncStepSnafu { + canister: &canister.name, + step: one_line(step), + } + .build() + }) + }); + for local in build.chain(sync) { + let path = canister_dir.join(&local?.path); + ensure!( + files.is_file(&path).await, + NotAFileSnafu { + canister: &canister.name, + path, + } + ); + } + } + Ok(()) +} + +/// A step as it displays, on one line: its kind, then the lines after the +/// first run together with `;` between. +fn one_line(step: &impl std::fmt::Display) -> String { + let text = step.to_string(); + let mut lines = text.lines().map(str::trim).filter(|line| !line.is_empty()); + let kind = lines.next().unwrap_or_default(); + let rest = lines.collect::>().join("; "); + if rest.is_empty() { + kind.to_owned() + } else { + format!("{kind} {rest}") + } +} + +/// A [`ProjectLoad`] that hands out only a project [`vet`] accepts. +pub struct Vetted { + loader: L, + files: Arc, +} + +impl Vetted { + /// `files` is what the paths are resolved through: the filesystem the + /// project is on. + pub fn new(loader: L, files: Arc) -> Self { + Self { loader, files } + } +} + +#[async_trait] +impl ProjectLoad for Vetted { + async fn load(&self) -> Result { + let project = self.loader.load().await?; + vet(&project, self.files.as_ref()) + .await + .context(UntrustedSnafu)?; + Ok(project) + } + + async fn exists(&self) -> Result { + self.loader.exists().await + } + + fn member_dir(&self) -> Option { + self.loader.member_dir() + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::*; + use crate::{ + MockProjectLoader, + files::{FsError, Scratch}, + manifest::{ + adapter::{prebuilt, script}, + canister::{BuildSteps, SyncSteps}, + }, + }; + + /// A filesystem that knows which paths are regular files, which is all + /// the vet asks of one. + struct Disk(HashSet); + + #[async_trait] + impl FileSystem for Disk { + async fn is_file(&self, path: &Path) -> bool { + self.0.contains(path) + } + async fn read(&self, _: &Path) -> Result, FsError> { + unreachable!("the vet reads nothing") + } + async fn read_to_string(&self, _: &Path) -> Result { + unreachable!("the vet reads nothing") + } + async fn write(&self, _: &Path, _: &[u8]) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn create_dir_all(&self, _: &Path) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn copy(&self, _: &Path, _: &Path) -> Result<(), FsError> { + unreachable!("the vet writes nothing") + } + async fn exists(&self, _: &Path) -> bool { + unreachable!("the vet asks only for a regular file") + } + async fn is_dir(&self, _: &Path) -> bool { + unreachable!("the vet asks only for a regular file") + } + async fn read_dir(&self, _: &Path) -> Result, FsError> { + unreachable!("the vet reads nothing") + } + async fn canonicalize(&self, _: &Path) -> Option { + unreachable!("the vet asks only for a regular file") + } + async fn scratch_dir(&self) -> Result, FsError> { + unreachable!("the vet writes nothing") + } + } + + /// The minimal project is at `/project`, and this is its disk: a module + /// and a plugin. + fn disk() -> Arc { + Arc::new(Disk(HashSet::from([ + "/project/backend.wasm".into(), + "/project/p.wasm".into(), + ]))) + } + + async fn project_with(build: Vec, sync: Vec) -> Project { + let mut project = MockProjectLoader::minimal().load().await.unwrap(); + let (_, canister) = project.canisters.get_mut("backend").unwrap(); + canister.build = BuildSteps { steps: build }; + canister.sync = SyncSteps { steps: sync }; + project + } + + async fn vetted(build: Vec, sync: Vec) -> Result<(), Refused> { + vet(&project_with(build, sync).await, disk().as_ref()).await + } + + fn local(path: &str) -> BuildStep { + BuildStep::Prebuilt(prebuilt::Adapter { + source: SourceField::Local(prebuilt::LocalSource { path: path.into() }), + sha256: None, + }) + } + + fn script(command: &str) -> script::Adapter { + script::Adapter { + command: script::CommandField::Command(command.to_owned()), + } + } + + fn plugin(source: &str) -> SyncStep { + SyncStep::Plugin(Box::new( + serde_yaml::from_str(&format!("{source}\nsha256: abc")).unwrap(), + )) + } + + #[tokio::test] + async fn a_bundle_shaped_project_is_accepted() { + vetted(vec![local("backend.wasm")], vec![plugin("path: p.wasm")]) + .await + .expect("pre-built from the project, plugin from the project"); + } + + /// A script step and a remote module are the two other build steps there + /// are. Neither is on the list, and the message says what the list is + /// and what the step was. + #[tokio::test] + async fn a_build_step_that_is_not_a_local_prebuilt_module_is_refused() { + let err = vetted(vec![BuildStep::Script(script("make"))], vec![]) + .await + .unwrap_err(); + assert!(matches!(err, Refused::BuildStep { ref canister, .. } if canister == "backend")); + let text = err.to_string(); + assert!(text.contains("not a pre-built module"), "{text}"); + assert!(text.contains("(script) make"), "{text}"); + + let several = script::Adapter { + command: script::CommandField::Commands(vec!["make".into(), "cp a b".into()]), + }; + let err = vetted(vec![BuildStep::Script(several)], vec![]) + .await + .unwrap_err(); + assert!(err.to_string().contains("(script) make; cp a b"), "{err}"); + + let remote = BuildStep::Prebuilt(prebuilt::Adapter { + source: SourceField::Remote(prebuilt::RemoteSource { + url: "https://example.com/backend.wasm".into(), + }), + sha256: Some("abc".into()), + }); + let err = vetted(vec![remote], vec![]).await.unwrap_err(); + assert!(matches!(err, Refused::BuildStep { .. })); + assert!( + err.to_string().contains("example.com/backend.wasm"), + "{err}" + ); + + // One good step does not carry a bad one. + let err = vetted( + vec![local("backend.wasm"), BuildStep::Script(script("make"))], + vec![], + ) + .await + .unwrap_err(); + assert!(matches!(err, Refused::BuildStep { .. })); + } + + #[tokio::test] + async fn a_sync_step_that_is_not_a_local_plugin_is_refused() { + let err = vetted( + vec![local("backend.wasm")], + vec![SyncStep::Script(script("upload"))], + ) + .await + .unwrap_err(); + assert!(matches!(err, Refused::SyncStep { .. })); + let text = err.to_string(); + assert!(text.contains("not a plugin"), "{text}"); + assert!(text.contains("script"), "{text}"); + + let err = vetted( + vec![local("backend.wasm")], + vec![plugin("url: https://example.com/plugin.wasm")], + ) + .await + .unwrap_err(); + assert!(matches!(err, Refused::SyncStep { .. })); + assert!(err.to_string().contains("example.com/plugin.wasm"), "{err}"); + } + + #[tokio::test] + async fn a_vetted_loader_refuses_what_vet_refuses() { + let project = project_with(vec![BuildStep::Script(script("make"))], vec![]).await; + let loader = Vetted::new(MockProjectLoader::new(project), disk()); + assert!(loader.exists().await.unwrap()); + let err = loader.load().await.unwrap_err(); + assert!(matches!(err, ProjectLoadError::Untrusted { .. }), "{err}"); + + let loader = Vetted::new(MockProjectLoader::minimal(), disk()); + loader + .load() + .await + .expect("the minimal project is pre-built from its own file"); + } + + /// What a step names must be a regular file the filesystem will give, + /// for a module and for a plugin alike. + #[tokio::test] + async fn what_a_step_names_must_be_a_regular_file() { + let err = vetted(vec![local("missing.wasm")], vec![]) + .await + .unwrap_err(); + assert!( + matches!(err, Refused::NotAFile { ref path, .. } if path == "/project/missing.wasm"), + "{err}" + ); + assert!(err.to_string().contains("not a regular file"), "{err}"); + + let err = vetted(vec![local("backend.wasm")], vec![plugin("path: gone.wasm")]) + .await + .unwrap_err(); + assert!(matches!(err, Refused::NotAFile { .. }), "{err}"); + } + + /// On a real disk, through the filesystem the CLI confines an untrusted + /// project to: a module of the project's own is accepted, with the root + /// spelled through a link as a macOS temp dir is, and one outside the + /// root, behind a link wherever it leads, or not a regular file is + /// refused, the pipe before anything tries to read it. + #[cfg(all(unix, feature = "host"))] + #[tokio::test] + async fn on_disk_a_step_names_a_regular_file_under_the_root_through_no_link() { + use crate::files::Confined; + use std::os::unix::fs::symlink; + + let dir = camino_tempfile::tempdir().unwrap(); + let real = dir.path().join("project"); + std::fs::create_dir(&real).unwrap(); + let root = dir.path().join("alias"); + symlink(&real, &root).unwrap(); + std::fs::write(root.join("backend.wasm"), b"module").unwrap(); + std::fs::write(dir.path().join("secret.wasm"), b"secret").unwrap(); + symlink(root.join("backend.wasm"), root.join("in.wasm")).unwrap(); + symlink(dir.path().join("secret.wasm"), root.join("out.wasm")).unwrap(); + symlink(dir.path(), root.join("outdir")).unwrap(); + std::fs::create_dir(root.join("dir.wasm")).unwrap(); + let made = std::process::Command::new("mkfifo") + .arg(root.join("pipe.wasm")) + .status() + .unwrap(); + assert!(made.success(), "mkfifo"); + let files = Confined::new(&root).unwrap(); + + let mut project = project_with(vec![local("backend.wasm")], vec![]).await; + project.dir = root.clone(); + project.canisters.get_mut("backend").unwrap().0 = root.clone(); + vet(&project, &files) + .await + .expect("the project's own module"); + + for path in [ + "../secret.wasm", + "in.wasm", + "out.wasm", + "outdir/secret.wasm", + "dir.wasm", + "pipe.wasm", + ] { + project.canisters.get_mut("backend").unwrap().1.build = BuildSteps { + steps: vec![local(path)], + }; + let err = vet(&project, &files).await.unwrap_err(); + assert!(matches!(err, Refused::NotAFile { .. }), "{path}: {err}"); + } + } +} diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9c14edebb..53f370922 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -136,6 +136,7 @@ Build canisters ###### **Options:** * `-e`, `--environment ` — Override the environment to build for. By default, the local environment is used +* `--untrusted` — Hold the project to what one someone else handed over may do: every file it reads is under the project root and reached through no symlink, each build step is a pre-built module and each sync step a plugin. Anything else is refused @@ -1071,6 +1072,7 @@ using --args or --args-file: - `bin`: Raw binary (only valid for file references) +* `--untrusted` — Hold the project to what one someone else handed over may do: every file it reads is under the project root and reached through no symlink, each build step is a pre-built module and each sync step a plugin. Anything else is refused @@ -1919,6 +1921,7 @@ Synchronize canisters * `--proxy ` — Principal of a proxy canister to route sync plugin calls to the target canister through * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as +* `--untrusted` — Hold the project to what one someone else handed over may do: every file it reads is under the project root and reached through no symlink, each build step is a pre-built module and each sync step a plugin. Anything else is refused