diff --git a/Cargo.lock b/Cargo.lock index 53a37f820..84f41eced 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -433,6 +433,7 @@ dependencies = [ "async-recursion", "async-trait", "bon", + "brush-builtins", "brush-parser", "cached", "cfg-if", diff --git a/brush-core/Cargo.toml b/brush-core/Cargo.toml index d3f59d289..198b4c064 100644 --- a/brush-core/Cargo.toml +++ b/brush-core/Cargo.toml @@ -85,5 +85,10 @@ uuid = { version = "1.23.1", features = ["js"] } [dev-dependencies] anyhow = "1.0.102" +# Used only by integration tests that need a realistically-populated builtin +# table (e.g. `echo`, `.`/`source`) to exercise the capability-interceptor +# hooks. This is a dev-dependency cycle (brush-builtins depends on brush-core), +# which Cargo permits for dev-dependencies. +brush-builtins = { version = "^0.2.0", path = "../brush-builtins" } pretty_assertions = { version = "1.4.1", features = ["unstable"] } tempfile = "3.27.0" diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs index 0bd9c3b30..2f3184158 100644 --- a/brush-core/src/commands.rs +++ b/brush-core/src/commands.rs @@ -579,6 +579,23 @@ pub(crate) fn execute_external_command( }) .collect::>(); + // Give the configured command interceptor (capability confinement) a chance + // to deny this external spawn. This site is the single funnel for *all* + // external commands, including the path-separator branch (e.g. `/bin/rm`, + // `./x`) that bypasses PATH and the builtin table, so a policy here cannot + // be circumvented by spelling the command differently. + { + use crate::extensions::CommandInterceptor as _; + let hook_args: Vec = cmd_args.iter().map(|s| (*s).clone()).collect(); + if let extensions::ExecDecision::Deny(reason) = context + .shell + .command_interceptor() + .before_exec(executable_path, hook_args.as_slice()) + { + return Err(error::ErrorKind::ExecDenied(context.command_name.clone(), reason).into()); + } + } + // Before we lose ownership of the open files, figure out if stdin will be a terminal. let child_stdin_is_terminal = context .try_fd(openfiles::OpenFiles::STDIN_FD) diff --git a/brush-core/src/error.rs b/brush-core/src/error.rs index 4c0088a3c..bc715e332 100644 --- a/brush-core/src/error.rs +++ b/brush-core/src/error.rs @@ -318,6 +318,12 @@ pub enum ErrorKind { /// A glob pattern failed to match any files (failglob). #[error("no match: {0}")] NoMatch(String), + + /// Execution of an external command was denied by the configured + /// command interceptor (capability confinement). The first field is the + /// program; the second is the reason supplied by the interceptor. + #[error("{0}: execution denied: {1}")] + ExecDenied(String, String), } /// Trait implementable by built-in commands to represent errors. @@ -363,6 +369,7 @@ impl From<&ErrorKind> for results::ExecutionExitCode { ErrorKind::FunctionParseError(..) => Self::InvalidUsage, ErrorKind::TestCommandParseError(..) => Self::InvalidUsage, ErrorKind::FailedToExecuteCommand(..) => Self::CannotExecute, + ErrorKind::ExecDenied(..) => Self::CannotExecute, ErrorKind::FunctionNameShadowsSpecialBuiltin { .. } => Self::InvalidUsage, ErrorKind::IoError(io_err) => io_err.into(), ErrorKind::BuiltinError(inner, ..) => inner.as_exit_code(), diff --git a/brush-core/src/extensions.rs b/brush-core/src/extensions.rs index 487f89fc0..592e4803d 100644 --- a/brush-core/src/extensions.rs +++ b/brush-core/src/extensions.rs @@ -1,5 +1,7 @@ //! Definition of shell behavior traits and defaults. +use std::path::Path; + use crate::{Shell, error, extensions}; /// Trait for static shell extensions. Collects all associated types needed to @@ -7,21 +9,33 @@ use crate::{Shell, error, extensions}; pub trait ShellExtensions: Clone + Default + Send + Sync + 'static { /// Type of the error behavior implementation. type ErrorFormatter: ErrorFormatter; + + /// Type of the command-interceptor (capability-confinement) implementation. + /// + /// This component allows an embedding host to observe — and optionally + /// *deny* — external command execution and file opens as they happen, + /// in-process. See [`CommandInterceptor`] for the available hooks. + type CommandInterceptor: CommandInterceptor; } /// Shell extensions implementation constructed from component types. #[derive(Clone, Default)] -pub struct ShellExtensionsImpl { - _marker: std::marker::PhantomData, +pub struct ShellExtensionsImpl< + EF: ErrorFormatter = DefaultErrorFormatter, + CI: CommandInterceptor = DefaultCommandInterceptor, +> { + _marker: std::marker::PhantomData<(EF, CI)>, } -impl ShellExtensions for ShellExtensionsImpl { +impl ShellExtensions for ShellExtensionsImpl { type ErrorFormatter = EF; + type CommandInterceptor = CI; } /// Default shell extensions implementation. /// This is a type alias for the most common shell configuration. -pub type DefaultShellExtensions = ShellExtensionsImpl; +pub type DefaultShellExtensions = + ShellExtensionsImpl; /// Trait for defining shell error behaviors. pub trait ErrorFormatter: Clone + Default + Send + Sync + 'static { @@ -47,6 +61,89 @@ pub struct DefaultErrorFormatter; impl ErrorFormatter for DefaultErrorFormatter {} +/// Decision returned by [`CommandInterceptor::before_exec`] to control whether +/// an external command is allowed to spawn. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ExecDecision { + /// Allow the external command to be spawned (the default). + Allow, + /// Deny the external command. The contained string explains why; it is + /// surfaced to the shell as an [`error::Error`] and the command does not + /// run. + Deny(String), +} + +/// Decision returned by [`CommandInterceptor::before_open`] to control whether +/// a file may be opened. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum OpenDecision { + /// Allow the file to be opened (the default). + Allow, + /// Deny opening the file. The contained string explains why; it is surfaced + /// to the shell as an [`error::Error`] and the file is not opened. + Deny(String), +} + +/// Trait for intercepting potentially-sensitive shell operations so an +/// embedding host can apply capability confinement (object-capability style +/// authority attenuation) *in-process*. +/// +/// The default implementation ([`DefaultCommandInterceptor`]) allows +/// everything, making it byte-for-byte equivalent to a shell with no +/// interceptor at all. Embedders supply their own implementation via the +/// [`ShellExtensions::CommandInterceptor`] associated type to enforce a policy. +/// +/// # Why this exists +/// +/// Without these hooks, a hosting process cannot reliably confine command +/// execution in-process: a command whose name contains a path separator (e.g. +/// `/bin/rm` or `./script`) bypasses both the `PATH` search and the builtin +/// table and is executed directly. [`before_exec`](Self::before_exec) is called +/// at *every* external-spawn site — including that path-separator branch — so a +/// policy here cannot be circumvented by spelling the command differently. +pub trait CommandInterceptor: Clone + Default + Send + Sync + 'static { + /// Called immediately before an external command is spawned, at every spawn + /// site (including the path-separator branch that bypasses `PATH` and the + /// builtin table). Returning [`ExecDecision::Deny`] prevents the command + /// from running and fails it with an error. + /// + /// # Arguments + /// + /// * `program` - The program that is about to be executed. For commands + /// resolved via `PATH` this is the resolved absolute path; for + /// path-separator commands it is the path as written by the user. + /// * `args` - The argument strings that would be passed to the program + /// (not including `argv[0]`). + fn before_exec(&self, program: &str, args: &[String]) -> ExecDecision { + let _ = (program, args); + ExecDecision::Allow + } + + /// Called immediately before a file is opened via a filesystem path + /// (redirections and `source`/`.`). Returning [`OpenDecision::Deny`] + /// prevents the file from being opened and fails the operation with an + /// error. + /// + /// # Arguments + /// + /// * `path` - The absolute path that is about to be opened. + /// * `write` - Whether the open requests write access (`true`) or is + /// read-only (`false`). + fn before_open(&self, path: &Path, write: bool) -> OpenDecision { + let _ = (path, write); + OpenDecision::Allow + } +} + +/// Default command-interceptor implementation: allows all execs and opens. +/// +/// A shell configured with this interceptor behaves identically to a shell with +/// no interception at all. +#[derive(Clone, Default)] +pub struct DefaultCommandInterceptor; + +impl CommandInterceptor for DefaultCommandInterceptor {} + /// Trait for placeholder behavior (stub for future extension). pub trait PlaceholderBehavior: Clone + Default + Send + Sync + 'static {} diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs index 11d02bfaa..758deac46 100644 --- a/brush-core/src/shell.rs +++ b/brush-core/src/shell.rs @@ -61,6 +61,13 @@ pub struct Shell Clone for Shell { fn clone(&self) -> Self { Self { error_formatter: self.error_formatter.clone(), + command_interceptor: self.command_interceptor.clone(), traps: self.traps.clone(), open_files: self.open_files.clone(), working_dir: self.working_dir.clone(), @@ -212,6 +220,7 @@ impl Shell { // Instantiate the shell with some defaults. let mut shell = Self { error_formatter: options.error_formatter, + command_interceptor: options.command_interceptor, open_files: openfiles::OpenFiles::new(), options: runtime_options, name: options.shell_name, @@ -349,6 +358,14 @@ impl Shell { pub(crate) const fn last_exit_status_change_count(&self) -> usize { self.last_exit_status_change_count } + + /// Returns a reference to the shell's configured command interceptor + /// (capability-confinement) behavior. Hosts rarely need to call this + /// directly; the shell consults it automatically before spawning external + /// commands and opening files. + pub const fn command_interceptor(&self) -> &SE::CommandInterceptor { + &self.command_interceptor + } } #[inherent::inherent] @@ -548,3 +565,8 @@ impl ShellState for Shell { fn default_error_formatter() -> EF { EF::default() } + +#[cfg(feature = "serde")] +fn default_command_interceptor() -> CI { + CI::default() +} diff --git a/brush-core/src/shell/builder.rs b/brush-core/src/shell/builder.rs index 3f99a9249..600d8bf71 100644 --- a/brush-core/src/shell/builder.rs +++ b/brush-core/src/shell/builder.rs @@ -146,6 +146,9 @@ pub struct CreateOptions Default for Shell { fn default() -> Self { Self { error_formatter: SE::ErrorFormatter::default(), + command_interceptor: SE::CommandInterceptor::default(), traps: traps::TrapHandlerConfig::default(), open_files: openfiles::OpenFiles::default(), working_dir: PathBuf::default(), diff --git a/brush-core/src/shell/fs.rs b/brush-core/src/shell/fs.rs index de4b75cff..077418538 100644 --- a/brush-core/src/shell/fs.rs +++ b/brush-core/src/shell/fs.rs @@ -195,6 +195,23 @@ impl crate::Shell { let path_to_open = self.absolute_path(path.as_ref()); + // Consult the configured command interceptor (capability confinement) + // before opening. This is the single choke point through which all + // filesystem-path opens flow (redirections and `source`/`.`), so a + // policy applied here covers every path-based open in the shell. + { + use crate::extensions::CommandInterceptor as _; + let write = open_options_request_write(options); + if let crate::extensions::OpenDecision::Deny(reason) = + self.command_interceptor().before_open(&path_to_open, write) + { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!("open denied: {reason}"), + )); + } + } + // See if this is a reference to a file descriptor. These paths should // reflect the shell's current execution fds, which can differ from the // host process fds after redirections like here-docs. @@ -242,3 +259,38 @@ fn shell_fd_path_to_fd(path: &Path) -> Option { None } } + +/// Best-effort determination of whether an [`std::fs::OpenOptions`] requests +/// write access (including append). Used solely to inform the capability +/// interceptor's `before_open` hook of write-intent. +/// +/// `std::fs::OpenOptions` exposes no getters for its configured flags, so we +/// inspect its `Debug` representation, which is documented to render the +/// `write` and `append` booleans. If the format ever changes such that we +/// cannot determine intent, we conservatively report `true` (write) so that a +/// confinement policy never under-reports access. +fn open_options_request_write(options: &std::fs::OpenOptions) -> bool { + let rendered = format!("{options:?}"); + + // Reads the boolean value of a `name: ` field from the rendered + // debug string without slicing on byte offsets (which could panic on + // multi-byte boundaries). + let flag = |name: &str| -> Option { + let needle = format!("{name}: "); + let tail = rendered.split_once(&needle)?.1; + if tail.starts_with("true") { + Some(true) + } else if tail.starts_with("false") { + Some(false) + } else { + None + } + }; + + match (flag("write"), flag("append")) { + // We could read both flags: write access iff either is set. + (Some(write), Some(append)) => write || append, + // Could not parse the expected fields; fail safe toward "write". + _ => true, + } +} diff --git a/brush-core/tests/command_interceptor_tests.rs b/brush-core/tests/command_interceptor_tests.rs new file mode 100644 index 000000000..9cfac2fe6 --- /dev/null +++ b/brush-core/tests/command_interceptor_tests.rs @@ -0,0 +1,261 @@ +//! Integration tests for the `CommandInterceptor` capability-confinement hooks +//! on `ShellExtensions` (`before_exec` / `before_open`). +//! +//! These tests prove that an embedding host can deny external command execution +//! and file opens *in-process*, including for commands whose name contains a +//! path separator (e.g. `/bin/rm`). That path-separator branch historically +//! bypassed both the PATH search and the builtin table, so confining it is the +//! whole point of `before_exec`. +//! +//! These tests target unix. Denied commands use `/bin/rm` — its existence is +//! irrelevant because `before_exec` denies it *before* any spawn. Commands that +//! are actually executed use `/usr/bin/true`, which exists on both Linux and +//! macOS (note: `/bin/true` is absent on macOS). +#![cfg(unix)] +#![cfg(test)] +#![allow(clippy::panic_in_result_fn)] + +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use anyhow::Result; +use brush_core::extensions::{ + CommandInterceptor, ErrorFormatter, ExecDecision, OpenDecision, ShellExtensions, +}; + +/// An interceptor that denies any external program whose basename is in a deny +/// list, and denies write-opens of any path that is not under `allowed_write_dir`. +/// All decisions are recorded so tests can assert the hook actually fired. +#[derive(Clone, Default)] +struct PolicyInterceptor { + denied_basenames: Arc>, + allowed_write_dir: Arc>>, + exec_calls: Arc>>, + open_calls: Arc>>, +} + +impl PolicyInterceptor { + fn new(denied_basenames: &[&str], allowed_write_dir: Option) -> Self { + Self { + denied_basenames: Arc::new(denied_basenames.iter().map(|s| (*s).to_string()).collect()), + allowed_write_dir: Arc::new(Mutex::new(allowed_write_dir)), + exec_calls: Arc::new(Mutex::new(Vec::new())), + open_calls: Arc::new(Mutex::new(Vec::new())), + } + } + + fn exec_calls(&self) -> Vec { + self.exec_calls.lock().unwrap().clone() + } + + fn open_calls(&self) -> Vec<(PathBuf, bool)> { + self.open_calls.lock().unwrap().clone() + } +} + +impl CommandInterceptor for PolicyInterceptor { + fn before_exec(&self, program: &str, _args: &[String]) -> ExecDecision { + self.exec_calls.lock().unwrap().push(program.to_string()); + + // Match on the basename so that `rm` and `/bin/rm` are both caught. + let basename = Path::new(program) + .file_name() + .map_or_else(|| program.to_string(), |s| s.to_string_lossy().into_owned()); + + if self.denied_basenames.iter().any(|d| d == &basename) { + ExecDecision::Deny(format!("'{basename}' is not permitted by policy")) + } else { + ExecDecision::Allow + } + } + + fn before_open(&self, path: &Path, write: bool) -> OpenDecision { + self.open_calls + .lock() + .unwrap() + .push((path.to_path_buf(), write)); + + if !write { + return OpenDecision::Allow; + } + + match self.allowed_write_dir.lock().unwrap().as_ref() { + Some(dir) if path.starts_with(dir) => OpenDecision::Allow, + Some(_) => OpenDecision::Deny(format!( + "writes to {} are outside the permitted directory", + path.display() + )), + None => OpenDecision::Allow, + } + } +} + +/// Wire `PolicyInterceptor` into a `ShellExtensions` bundle that otherwise uses +/// brush's default behaviors. +#[derive(Clone, Default)] +struct PolicyExtensions; + +impl ShellExtensions for PolicyExtensions { + type ErrorFormatter = DefaultFormatter; + type CommandInterceptor = PolicyInterceptor; +} + +#[derive(Clone, Default)] +struct DefaultFormatter; +impl ErrorFormatter for DefaultFormatter {} + +/// Builds a shell that uses the provided interceptor, skipping profile/rc and +/// environment inheritance so the test is hermetic, but with the default +/// builtin table registered (so `echo`, `.`/`source`, etc. behave normally). +async fn shell_with_interceptor( + interceptor: PolicyInterceptor, +) -> Result> { + let builtins = + brush_builtins::default_builtins::(brush_builtins::BuiltinSet::BashMode); + + let mut shell = brush_core::Shell::builder_with_extensions::() + .command_interceptor(interceptor) + .builtins(builtins) + .do_not_inherit_env(true) + .skip_well_known_vars(true) + .build() + .await?; + + // Provide a deterministic PATH so bare-name external commands can resolve. + run(&mut shell, "export PATH=/bin:/usr/bin").await?; + + Ok(shell) +} + +async fn run(shell: &mut brush_core::Shell, cmd: &str) -> Result { + let params = shell.default_exec_params(); + let result = shell + .run_string(cmd, &brush_core::SourceInfo::default(), ¶ms) + .await?; + Ok(u8::from(result.exit_code)) +} + +/// `before_exec` must deny a command referenced by bare name (resolved via PATH). +#[tokio::test] +async fn denies_bare_name_command() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, "rm /tmp/does-not-matter").await?; + assert_ne!(code, 0, "denied `rm` must report a non-zero exit code"); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p.ends_with("rm")), + "before_exec should have been consulted for `rm`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// The load-bearing test: a command containing a path separator (`/bin/rm`) +/// historically bypassed PATH and the builtin table. `before_exec` must still +/// fire for it, proving the bypass is closed. +#[tokio::test] +async fn denies_absolute_path_command_closing_path_separator_bypass() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, "/bin/rm /tmp/does-not-matter").await?; + assert_ne!( + code, 0, + "denied `/bin/rm` (path-separator branch) must report a non-zero exit code" + ); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p == "/bin/rm"), + "before_exec must be consulted for the path-separator command `/bin/rm`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// A permitted command must still run normally — the default decision is Allow. +#[tokio::test] +async fn allows_permitted_command() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + // `/usr/bin/true` is a path-separator command too, so this also proves the + // Allow path of the path-separator branch. We use `/usr/bin/true` rather than + // `/bin/true` because the latter does not exist on macOS (where `true` lives + // only at `/usr/bin/true`); `/usr/bin/true` is present on both Linux and macOS. + let code = run(&mut shell, "/usr/bin/true").await?; + assert_eq!(code, 0, "permitted `/usr/bin/true` should succeed"); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p == "/usr/bin/true"), + "before_exec should have observed `/usr/bin/true`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// `before_open` must deny an output redirection that writes outside the +/// permitted directory, while allowing one inside it. +#[tokio::test] +async fn denies_write_outside_allowed_dir() -> Result<()> { + let allowed = tempfile::tempdir()?; + let forbidden = tempfile::tempdir()?; + + let interceptor = PolicyInterceptor::new(&[], Some(allowed.path().to_path_buf())); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + // Writing inside the allowed dir is permitted. + let allowed_file = allowed.path().join("ok.txt"); + let code = run(&mut shell, &format!("echo hi > {}", allowed_file.display())).await?; + assert_eq!(code, 0, "write inside the allowed dir should succeed"); + assert!( + allowed_file.exists(), + "the permitted file should have been created" + ); + + // Writing outside the allowed dir is denied. + let forbidden_file = forbidden.path().join("nope.txt"); + let code = run( + &mut shell, + &format!("echo hi > {}", forbidden_file.display()), + ) + .await?; + assert_ne!(code, 0, "write outside the allowed dir must fail"); + assert!( + !forbidden_file.exists(), + "the forbidden file must NOT have been created" + ); + + let open_calls = interceptor.open_calls(); + assert!( + open_calls.iter().any(|(p, w)| *w && p == &forbidden_file), + "before_open should have been consulted (with write=true) for the forbidden path; saw: {open_calls:?}" + ); + Ok(()) +} + +/// Reads must be allowed by this policy (write=false), proving the `write` flag +/// is threaded correctly and read-only opens aren't accidentally denied. +#[tokio::test] +async fn allows_read_open() -> Result<()> { + let dir = tempfile::tempdir()?; + let script = dir.path().join("snippet.sh"); + std::fs::write(&script, "X=hello\n")?; + + // allowed_write_dir is set to a *different* dir, so if reads were + // misclassified as writes they would be denied. + let other = tempfile::tempdir()?; + let interceptor = PolicyInterceptor::new(&[], Some(other.path().to_path_buf())); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, &format!(". {}", script.display())).await?; + assert_eq!(code, 0, "sourcing (read-only open) should be permitted"); + + let open_calls = interceptor.open_calls(); + assert!( + open_calls.iter().any(|(p, w)| !*w && p == &script), + "before_open should have observed a read-only open of the sourced file; saw: {open_calls:?}" + ); + Ok(()) +}