diff --git a/.github/workflows/dep_rust.yml b/.github/workflows/dep_rust.yml index a6acefd1c..c9a94f7ea 100644 --- a/.github/workflows/dep_rust.yml +++ b/.github/workflows/dep_rust.yml @@ -146,8 +146,8 @@ jobs: # with default features just test ${{ inputs.config }} ${{ inputs.hypervisor == 'mshv' && 'mshv2' || '""'}} - # with only one driver enabled (driver mshv/kvm feature is ignored on windows) + seccomp - just test ${{ inputs.config }} seccomp,${{ inputs.hypervisor == 'mshv' && 'mshv2' || inputs.hypervisor == 'mshv3' && 'mshv3' || 'kvm' }} + # with only one driver enabled (driver mshv/kvm feature is ignored on windows) + just test ${{ inputs.config }} ${{ inputs.hypervisor == 'mshv' && 'mshv2' || inputs.hypervisor == 'mshv3' && 'mshv3' || 'kvm' }} # make sure certain cargo features compile just check diff --git a/Cargo.lock b/Cargo.lock index 1ad1a78f9..0c823ae83 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1508,7 +1508,6 @@ dependencies = [ "proptest", "rand", "rust-embed", - "seccompiler", "serde", "serde_json", "serial_test", @@ -3076,15 +3075,6 @@ version = "3.0.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" -[[package]] -name = "seccompiler" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4ae55de56877481d112a559bbc12667635fdaf5e005712fd4e2b2fa50ffc884" -dependencies = [ - "libc", -] - [[package]] name = "semver" version = "1.0.26" diff --git a/Justfile b/Justfile index 274c076ac..67925f662 100644 --- a/Justfile +++ b/Justfile @@ -80,8 +80,8 @@ test-like-ci config=default-target hypervisor="kvm": @# with default features just test {{config}} {{ if hypervisor == "mshv" {"mshv2"} else {""} }} - @# with only one driver enabled + seccomp + build-metadata + init-paging - just test {{config}} seccomp,build-metadata,init-paging,{{ if hypervisor == "mshv" {"mshv2"} else if hypervisor == "mshv3" {"mshv3"} else {"kvm"} }} + @# with only one driver enabled + build-metadata + init-paging + just test {{config}} build-metadata,init-paging,{{ if hypervisor == "mshv" {"mshv2"} else if hypervisor == "mshv3" {"mshv3"} else {"kvm"} }} @# make sure certain cargo features compile just check @@ -145,7 +145,7 @@ like-ci config=default-target hypervisor="kvm": {{ if config == "release" { "just bench-ci main " + if hypervisor == "mshv" { "mshv2" } else if hypervisor == "mshv3" { "mshv3" } else { "kvm" } } else { "" } }} # runs all tests -test target=default-target features="": (test-unit target features) (test-isolated target features) (test-integration "rust" target features) (test-integration "c" target features) (test-seccomp target features) (test-doc target features) +test target=default-target features="": (test-unit target features) (test-isolated target features) (test-integration "rust" target features) (test-integration "c" target features) (test-doc target features) # runs unit tests test-unit target=default-target features="": @@ -170,12 +170,6 @@ test-integration guest target=default-target features="": @# run the rest of the integration tests {{if os() == "windows" { "$env:" } else { "" } }}GUEST="{{guest}}"{{if os() == "windows" { ";" } else { "" } }} {{ cargo-cmd }} test -p hyperlight-host {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F init-paging," + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} --test '*' -# runs seccomp tests -test-seccomp target=default-target features="": - @# run seccomp test with feature "seccomp" on and off - {{ cargo-cmd }} test --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host test_violate_seccomp_filters --lib {{ if features =="" {''} else { "--features " + features } }} -- --ignored - {{ cargo-cmd }} test --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host test_violate_seccomp_filters --no-default-features {{ if features =~"mshv2" {"--features init-paging,mshv2"} else {"--features mshv3,init-paging,kvm" } }} --lib -- --ignored - # tests compilation with no default features on different platforms test-compilation-no-default-features target=default-target: @# Linux should fail without a hypervisor feature (kvm, mshv, or mshv3) diff --git a/docs/security.md b/docs/security.md index 10f96bd46..3ee86780c 100644 --- a/docs/security.md +++ b/docs/security.md @@ -19,5 +19,3 @@ All communication between the host and the guest is done through a shared memory Hyperlight provides a mechanism for the host to register functions that may be called from the guest. This mechanism is useful to allow developers to provide guests with strictly controlled access to functionality we don't make available by default inside the VM. This mechanism likely represents the largest attack surface area of this project. To mitigate the risk, only functions that have been explicitly exposed to the guest by the host application, are allowed to be called from the guest. Any attempt to call other host functions will result in an error. - -Additionally, we provide an API for using Seccomp filters to further restrict the system calls available to the host-provided functions, to help limit the impact of the un-audited or un-managed functions. diff --git a/docs/signal-handlers-development-notes.md b/docs/signal-handlers-development-notes.md index fca9d31a9..3cde0dafc 100644 --- a/docs/signal-handlers-development-notes.md +++ b/docs/signal-handlers-development-notes.md @@ -1,17 +1,9 @@ # Signal Handling in Hyperlight -Hyperlight registers custom signal handlers to intercept and manage specific signals, primarily `SIGSYS` and `SIGRTMIN`. Here's an overview of the registration process: -- **Preserving Old Handlers**: When registering a new signal handler, Hyperlight first retrieves and stores the existing handler using `OnceCell`. This allows Hyperlight to delegate signals to the original handler if necessary. +Hyperlight registers custom signal handlers to intercept and manage specific signals, primarily `SIGRTMIN`. Here's an overview of the registration process: - **Custom Handlers**: - - **`SIGSYS` Handler**: Captures disallowed syscalls enforced by seccomp. If the signal originates from a hyperlight thread, Hyperlight logs the syscall details. Otherwise, it delegates the signal to the previously registered handler. - - **`SIGRTMIN` Handler**: Utilized for inter-thread signaling, such as execution cancellation. Similar to SIGSYS, it distinguishes between application and non-hyperlight threads to determine how to handle the signal. -- **Thread Differentiation**: Hyperlight uses thread-local storage (IS_HYPERLIGHT_THREAD) to identify whether the current thread is a hyperlight thread. This distinction ensures that signals are handled appropriately based on the thread's role. - -## Potential Issues and Considerations - -### Handler Invalidation - -**Issue**: After Hyperlight registers its custom signal handler and preserves the `old_handler`, if the host or another component modifies the signal handler for the same signal, it can lead to: - - **Invalidation of `old_handler`**: The stored old_handler reference may no longer point to a valid handler, causing undefined behavior when Hyperlight attempts to delegate signals. - - **Loss of Custom Handling**: Hyperlight's custom handler might not be invoked as expected, disrupting its ability to enforce syscall restrictions or manage inter-thread signals. - + - **`SIGRTMIN` Handler**: Utilized for inter-thread signaling, such as execution cancellation. +- **Killing a sandbox**: + - To stop a sandboxed process, a `SIGRTMIN` signal must be delivered to the thread running the sandboxed code. + - The sandbox provides an interface to obtain an interrupt handle, which includes the thread ID and a method to dispatch the signal. + - Hyperlight uses the `pthread_kill` function to send this signal directly to the targeted thread. diff --git a/src/hyperlight_host/Cargo.toml b/src/hyperlight_host/Cargo.toml index 76935b5b2..1f4899c12 100644 --- a/src/hyperlight_host/Cargo.toml +++ b/src/hyperlight_host/Cargo.toml @@ -75,7 +75,6 @@ windows-version = "0.1" lazy_static = "1.4.0" [target.'cfg(unix)'.dependencies] -seccompiler = { version = "0.5.0", optional = true } kvm-bindings = { version = "0.14", features = ["fam-wrappers"], optional = true } kvm-ioctls = { version = "0.24", optional = true } mshv-bindings2 = { package="mshv-bindings", version = "=0.2.1", optional = true } @@ -126,8 +125,7 @@ cfg_aliases = "0.2.1" built = { version = "0.8.0", optional = true, features = ["chrono", "git2"] } [features] -default = ["kvm", "mshv3", "seccomp", "build-metadata", "init-paging"] -seccomp = ["dep:seccompiler"] +default = ["kvm", "mshv3", "build-metadata", "init-paging"] function_call_metrics = [] executable_heap = [] # This feature enables printing of debug information to stdout in debug builds diff --git a/src/hyperlight_host/build.rs b/src/hyperlight_host/build.rs index bef001b22..6c90a485f 100644 --- a/src/hyperlight_host/build.rs +++ b/src/hyperlight_host/build.rs @@ -101,7 +101,6 @@ fn main() -> Result<()> { // the other features they want. mshv2: { all(feature = "mshv2", target_os = "linux") }, mshv3: { all(feature = "mshv3", not(feature="mshv2"), target_os = "linux") }, - seccomp: { all(feature = "seccomp", target_os = "linux", not(target_env = "musl")) }, } #[cfg(feature = "build-metadata")] diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index 9ecce9968..99e46b7c9 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -69,11 +69,6 @@ pub enum HyperlightError { #[error("Error converting CString {0:?}")] CStringConversionError(#[from] std::ffi::NulError), - /// A disallowed syscall was caught - #[error("Seccomp filter trapped on disallowed syscall (check STDERR for offending syscall)")] - #[cfg(seccomp)] - DisallowedSyscall, - /// A generic error with a message #[error("{0}")] Error(String), @@ -216,16 +211,6 @@ pub enum HyperlightError { #[error("Stack overflow detected")] StackOverflow(), - /// a backend error occurred with seccomp filters - #[error("Backend Error with Seccomp Filter {0:?}")] - #[cfg(seccomp)] - SeccompFilterBackendError(#[from] seccompiler::BackendError), - - /// an error occurred with seccomp filters - #[error("Error with Seccomp Filter {0:?}")] - #[cfg(seccomp)] - SeccompFilterError(#[from] seccompiler::Error), - /// Tried to restore snapshot to a sandbox that is not the same as the one the snapshot was taken from #[error("Snapshot was taken from a different sandbox")] SnapshotSandboxMismatch, diff --git a/src/hyperlight_host/src/func/host_functions.rs b/src/hyperlight_host/src/func/host_functions.rs index a0fcbb52f..6764a07c2 100644 --- a/src/hyperlight_host/src/func/host_functions.rs +++ b/src/hyperlight_host/src/func/host_functions.rs @@ -20,8 +20,8 @@ use hyperlight_common::flatbuffer_wrappers::function_types::{ParameterValue, Ret use super::utils::for_each_tuple; use super::{ParameterTuple, ResultType, SupportedReturnType}; +use crate::sandbox::UninitializedSandbox; use crate::sandbox::host_funcs::FunctionEntry; -use crate::sandbox::{ExtraAllowedSyscall, UninitializedSandbox}; use crate::{Result, new_error}; /// A sandbox on which (primitive) host functions can be registered @@ -33,15 +33,6 @@ pub trait Registerable { name: &str, hf: impl Into>, ) -> Result<()>; - /// Register a primitive host function whose worker thread has - /// extra permissive seccomp filters installed - #[cfg(seccomp)] - fn register_host_function_with_syscalls( - &mut self, - name: &str, - hf: impl Into>, - eas: Vec, - ) -> Result<()>; } impl Registerable for UninitializedSandbox { fn register_host_function( @@ -56,28 +47,6 @@ impl Registerable for UninitializedSandbox { let entry = FunctionEntry { function: hf.into().into(), - extra_allowed_syscalls: None, - parameter_types: Args::TYPE, - return_type: Output::TYPE, - }; - - (*hfs).register_host_function(name.to_string(), entry, &mut self.mgr) - } - #[cfg(seccomp)] - fn register_host_function_with_syscalls( - &mut self, - name: &str, - hf: impl Into>, - eas: Vec, - ) -> Result<()> { - let mut hfs = self - .host_funcs - .try_lock() - .map_err(|e| new_error!("Error locking at {}:{}: {}", file!(), line!(), e))?; - - let entry = FunctionEntry { - function: hf.into().into(), - extra_allowed_syscalls: Some(eas), parameter_types: Args::TYPE, return_type: Output::TYPE, }; @@ -195,13 +164,11 @@ pub(crate) fn register_host_function>, sandbox: &mut UninitializedSandbox, name: &str, - extra_allowed_syscalls: Option>, ) -> Result<()> { let func = func.into().into(); let entry = FunctionEntry { function: func, - extra_allowed_syscalls: extra_allowed_syscalls.clone(), parameter_types: Args::TYPE, return_type: Output::TYPE, }; diff --git a/src/hyperlight_host/src/lib.rs b/src/hyperlight_host/src/lib.rs index 10e8a898a..5f034dc79 100644 --- a/src/hyperlight_host/src/lib.rs +++ b/src/hyperlight_host/src/lib.rs @@ -76,8 +76,6 @@ pub mod metrics; /// outside this file. Types from this module needed for public consumption are /// re-exported below. pub mod sandbox; -#[cfg(seccomp)] -pub(crate) mod seccomp; /// Signal handling for Linux #[cfg(target_os = "linux")] pub(crate) mod signal_handlers; diff --git a/src/hyperlight_host/src/metrics/mod.rs b/src/hyperlight_host/src/metrics/mod.rs index 67a7b0eca..3a630fa44 100644 --- a/src/hyperlight_host/src/metrics/mod.rs +++ b/src/hyperlight_host/src/metrics/mod.rs @@ -133,11 +133,7 @@ mod tests { if #[cfg(feature = "function_call_metrics")] { use metrics::Label; - let expected_num_metrics = if cfg!(all(seccomp)) { - 3 // if seccomp enabled, the host call duration metric is emitted on a separate thread which this local recorder doesn't capture - } else { - 4 - }; + let expected_num_metrics = 4; // Verify that the histogram metrics are recorded correctly assert_eq!(snapshot.len(), expected_num_metrics); @@ -185,25 +181,6 @@ mod tests { ), "Histogram metric does not match expected value" ); - - if !cfg!(all(seccomp)) { - // 4. Host call duration - let histogram_key = CompositeKey::new( - metrics_util::MetricKind::Histogram, - Key::from_parts( - METRIC_HOST_FUNC_DURATION, - vec![Label::new("function_name", "HostPrint")], - ), - ); - let histogram_value = &snapshot.get(&histogram_key).unwrap().2; - assert!( - matches!( - histogram_value, - metrics_util::debugging::DebugValue::Histogram(histogram) if histogram.len() == 1 - ), - "Histogram metric does not match expected value" - ); - } } else { // Verify that the counter metrics are recorded correctly assert_eq!(snapshot.len(), 1); diff --git a/src/hyperlight_host/src/sandbox/host_funcs.rs b/src/hyperlight_host/src/sandbox/host_funcs.rs index b61a2cd77..52160539a 100644 --- a/src/hyperlight_host/src/sandbox/host_funcs.rs +++ b/src/hyperlight_host/src/sandbox/host_funcs.rs @@ -25,7 +25,6 @@ use hyperlight_common::flatbuffer_wrappers::host_function_details::HostFunctionD use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; use tracing::{Span, instrument}; -use super::ExtraAllowedSyscall; use crate::HyperlightError::HostFunctionNotFound; use crate::func::host_functions::TypeErasedHostFunction; use crate::mem::mgr::SandboxMemoryManager; @@ -58,7 +57,6 @@ impl From<&mut FunctionRegistry> for HostFunctionDetails { pub struct FunctionEntry { pub function: TypeErasedHostFunction, - pub extra_allowed_syscalls: Option>, pub parameter_types: &'static [ParameterType], pub return_type: ReturnType, } @@ -119,7 +117,6 @@ impl FunctionRegistry { fn call_host_func_impl(&self, name: &str, args: Vec) -> Result { let FunctionEntry { function, - extra_allowed_syscalls, parameter_types: _, return_type: _, } = self @@ -127,10 +124,8 @@ impl FunctionRegistry { .get(name) .ok_or_else(|| HostFunctionNotFound(name.to_string()))?; - // Create a new thread when seccomp is enabled on Linux - maybe_with_seccomp(name, extra_allowed_syscalls.as_deref(), || { - crate::metrics::maybe_time_and_emit_host_call(name, || function.call(args)) - }) + // Make the host function call + crate::metrics::maybe_time_and_emit_host_call(name, || function.call(args)) } } @@ -153,58 +148,3 @@ pub(super) fn default_writer_func(s: String) -> Result { } } } - -#[cfg(seccomp)] -fn maybe_with_seccomp( - name: &str, - syscalls: Option<&[ExtraAllowedSyscall]>, - f: impl FnOnce() -> Result + Send, -) -> Result { - use std::thread; - - use crate::seccomp::guest::get_seccomp_filter_for_host_function_worker_thread; - - // Use a scoped thread so that we can pass around references without having to clone them. - thread::scope(|s| { - thread::Builder::new() - .name(format!("Host Function Worker Thread for: {name:?}")) - .spawn_scoped(s, move || { - let seccomp_filter = get_seccomp_filter_for_host_function_worker_thread(syscalls)?; - seccomp_filter - .iter() - .try_for_each(|filter| seccompiler::apply_filter(filter))?; - - // We have a `catch_unwind` here because, if a disallowed syscall is issued, - // we handle it by panicking. This is to avoid returning execution to the - // offending host function—for two reasons: (1) if a host function is issuing - // disallowed syscalls, it could be unsafe to return to, and (2) returning - // execution after trapping the disallowed syscall can lead to UB (e.g., try - // running a host function that attempts to sleep without `SYS_clock_nanosleep`, - // you'll block the syscall but panic in the aftermath). - match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { - Ok(val) => val, - Err(err) => { - if let Some(crate::HyperlightError::DisallowedSyscall) = - err.downcast_ref::() - { - return Err(crate::HyperlightError::DisallowedSyscall); - } - - crate::log_then_return!("Host function {} panicked", name); - } - } - })? - .join() - .map_err(|_| new_error!("Error joining thread executing host function"))? - }) -} - -#[cfg(not(seccomp))] -fn maybe_with_seccomp( - _name: &str, - _syscalls: Option<&[ExtraAllowedSyscall]>, - f: impl FnOnce() -> Result + Send, -) -> Result { - // No seccomp, just call the function - f() -} diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index bbb23638b..b1fd45fa1 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -655,163 +655,6 @@ mod tests { assert_eq!(res, 0); } - #[test] - // TODO: Investigate why this test fails with an incorrect error when run alongside other tests - #[ignore] - #[cfg(target_os = "linux")] - fn test_violate_seccomp_filters() -> Result<()> { - fn make_get_pid_syscall() -> Result { - let pid = unsafe { libc::syscall(libc::SYS_getpid) }; - Ok(pid as u64) - } - - // First, run to make sure it fails. - { - let mut usbox = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), - None, - ) - .unwrap(); - - usbox.register("MakeGetpidSyscall", make_get_pid_syscall)?; - - let mut sbox: MultiUseSandbox = usbox.evolve()?; - - let res: Result = sbox.call("ViolateSeccompFilters", ()); - - #[cfg(seccomp)] - match res { - Ok(_) => panic!("Expected to fail due to seccomp violation"), - Err(e) => match e { - HyperlightError::GuestError(t, msg) - if t == ErrorCode::HostFunctionError - && msg.contains("Seccomp filter trapped on disallowed syscall") => {} - _ => panic!("Expected DisallowedSyscall error: {}", e), - }, - } - - #[cfg(not(seccomp))] - match res { - Ok(_) => (), - Err(e) => panic!("Expected to succeed without seccomp: {}", e), - } - } - - // Second, run with allowing `SYS_getpid` - #[cfg(seccomp)] - { - let mut usbox = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), - None, - ) - .unwrap(); - - usbox.register_with_extra_allowed_syscalls( - "MakeGetpidSyscall", - make_get_pid_syscall, - vec![libc::SYS_getpid], - )?; - // ^^^ note, we are allowing SYS_getpid - - let mut sbox: MultiUseSandbox = usbox.evolve()?; - - let res: Result = sbox.call("ViolateSeccompFilters", ()); - - match res { - Ok(_) => {} - Err(e) => panic!("Expected to succeed due to seccomp violation: {}", e), - } - } - - Ok(()) - } - - // We have a secomp specifically for `openat`, but we don't want to crash on `openat`, but rather make sure `openat` returns `EACCES` - #[test] - #[cfg(target_os = "linux")] - fn violate_seccomp_filters_openat() -> Result<()> { - // Hostcall to call `openat`. - fn make_openat_syscall() -> Result { - use std::ffi::CString; - - let path = CString::new("/proc/sys/vm/overcommit_memory").unwrap(); - - let fd_or_err = unsafe { - libc::syscall( - libc::SYS_openat, - libc::AT_FDCWD, - path.as_ptr(), - libc::O_RDONLY, - ) - }; - - if fd_or_err == -1 { - Ok((-std::io::Error::last_os_error().raw_os_error().unwrap()).into()) - } else { - Ok(fd_or_err) - } - } - { - // First make sure a regular call to `openat` on /proc/sys/vm/overcommit_memory succeeds - let ret = make_openat_syscall()?; - assert!( - ret >= 0, - "Expected openat syscall to succeed, got: {:?}", - ret - ); - - let mut ubox = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), - None, - ) - .unwrap(); - ubox.register("Openat_Hostfunc", make_openat_syscall)?; - - let mut sbox = ubox.evolve().unwrap(); - let host_func_result = sbox - .call::( - "CallGivenParamlessHostFuncThatReturnsI64", - "Openat_Hostfunc".to_string(), - ) - .expect("Expected to call host function that returns i64"); - - if cfg!(seccomp) { - // If seccomp is enabled, we expect the syscall to return EACCES, as setup by our seccomp filter - assert_eq!(host_func_result, -libc::EACCES as i64); - } else { - // If seccomp is not enabled, we expect the syscall to succeed - assert!(host_func_result >= 0); - } - } - - #[cfg(seccomp)] - { - // Now let's make sure if we register the `openat` syscall as an extra allowed syscall, it will succeed - let mut ubox = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), - None, - ) - .unwrap(); - ubox.register_with_extra_allowed_syscalls( - "Openat_Hostfunc", - make_openat_syscall, - [libc::SYS_openat], - )?; - let mut sbox = ubox.evolve().unwrap(); - let host_func_result: i64 = sbox - .call::( - "CallGivenParamlessHostFuncThatReturnsI64", - "Openat_Hostfunc".to_string(), - ) - .expect("Expected to call host function that returns i64"); - - // should pass regardless of seccomp feature - assert!(host_func_result >= 0); - } - - Ok(()) - } - #[test] fn test_trigger_exception_on_guest() { let usbox = UninitializedSandbox::new( diff --git a/src/hyperlight_host/src/sandbox/uninitialized.rs b/src/hyperlight_host/src/sandbox/uninitialized.rs index c4b08da2f..fb09572a9 100644 --- a/src/hyperlight_host/src/sandbox/uninitialized.rs +++ b/src/hyperlight_host/src/sandbox/uninitialized.rs @@ -35,21 +35,6 @@ use crate::mem::shared_mem::ExclusiveSharedMemory; use crate::sandbox::SandboxConfiguration; use crate::{MultiUseSandbox, Result, new_error}; -#[cfg(seccomp)] -const EXTRA_ALLOWED_SYSCALLS_FOR_WRITER_FUNC: &[super::ExtraAllowedSyscall] = &[ - // Fuzzing fails without `mmap` being an allowed syscall on our seccomp filter. - // All fuzzing does is call `PrintOutput` (which calls `HostPrint` ). Thing is, `println!` - // is designed to be thread-safe in Rust and the std lib ensures this by using - // buffered I/O, which I think relies on `mmap`. This gets surfaced in fuzzing with an - // OOM error, which I think is happening because `println!` is not being able to allocate - // more memory for its buffers for the fuzzer's huge inputs. - libc::SYS_mmap, - libc::SYS_brk, - libc::SYS_mprotect, - #[cfg(mshv)] - libc::SYS_close, -]; - #[cfg(any(crashdump, gdb))] #[derive(Clone, Debug, Default)] pub(crate) struct SandboxRuntimeConfig { @@ -304,25 +289,7 @@ impl UninitializedSandbox { name: impl AsRef, host_func: impl Into>, ) -> Result<()> { - register_host_function(host_func, self, name.as_ref(), None) - } - - /// Registers a host function with additional allowed syscalls during execution. - /// - /// Unlike [`register`](Self::register), this variant allows specifying extra syscalls - /// that will be permitted when the function handler runs. - #[cfg(seccomp)] - pub fn register_with_extra_allowed_syscalls< - Args: ParameterTuple, - Output: SupportedReturnType, - >( - &mut self, - name: impl AsRef, - host_func: impl Into>, - extra_allowed_syscalls: impl IntoIterator, - ) -> Result<()> { - let extra_allowed_syscalls: Vec<_> = extra_allowed_syscalls.into_iter().collect(); - register_host_function(host_func, self, name.as_ref(), Some(extra_allowed_syscalls)) + register_host_function(host_func, self, name.as_ref()) } /// Registers the special "HostPrint" function for guest printing. @@ -334,40 +301,7 @@ impl UninitializedSandbox { &mut self, print_func: impl Into>, ) -> Result<()> { - #[cfg(not(seccomp))] - self.register("HostPrint", print_func)?; - - #[cfg(seccomp)] - self.register_with_extra_allowed_syscalls( - "HostPrint", - print_func, - EXTRA_ALLOWED_SYSCALLS_FOR_WRITER_FUNC.iter().copied(), - )?; - - Ok(()) - } - - /// Registers the "HostPrint" function with additional allowed syscalls. - /// - /// Like [`register_print`](Self::register_print), but allows specifying extra syscalls - /// that will be permitted during function execution. - #[cfg(seccomp)] - pub fn register_print_with_extra_allowed_syscalls( - &mut self, - print_func: impl Into>, - extra_allowed_syscalls: impl IntoIterator, - ) -> Result<()> { - #[cfg(seccomp)] - self.register_with_extra_allowed_syscalls( - "HostPrint", - print_func, - EXTRA_ALLOWED_SYSCALLS_FOR_WRITER_FUNC - .iter() - .copied() - .chain(extra_allowed_syscalls), - )?; - - Ok(()) + self.register("HostPrint", print_func) } } // Check to see if the current version of Windows is supported diff --git a/src/hyperlight_host/src/seccomp/guest.rs b/src/hyperlight_host/src/seccomp/guest.rs deleted file mode 100644 index c55d40686..000000000 --- a/src/hyperlight_host/src/seccomp/guest.rs +++ /dev/null @@ -1,159 +0,0 @@ -/* -Copyright 2025 The Hyperlight Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -use seccompiler::SeccompCmpOp::Eq; -use seccompiler::{ - BpfProgram, SeccompAction, SeccompCmpArgLen as ArgLen, SeccompCondition as Cond, SeccompFilter, - SeccompRule, TargetArch, -}; - -use crate::sandbox::ExtraAllowedSyscall; -use crate::{Result, and, or}; - -fn syscalls_allowlist() -> Result)>> { - Ok(vec![ - // SYS_signalstack, SYS_munmap, SYS_rt_sigprocmask, SYS_madvise, and SYS_exit - // are minimally required syscalls to be able to setup our seccomp filter. - (libc::SYS_sigaltstack, vec![]), - (libc::SYS_munmap, vec![]), - (libc::SYS_rt_sigprocmask, vec![]), - (libc::SYS_madvise, vec![]), - (libc::SYS_exit, vec![]), - // SYS_rt_sigaction, SYS_write, and SYS_rt_sigreturn are required for the - // signal handler inside the host function worker thread. - (libc::SYS_rt_sigaction, vec![]), - ( - libc::SYS_write, - or![ - and![Cond::new(0, ArgLen::Dword, Eq, 1)?], // stdout - and![Cond::new(0, ArgLen::Dword, Eq, 2)?], // stderr - ], - ), - (libc::SYS_rt_sigreturn, vec![]), - // Note: This `ioctl` is used to get information about the terminal. - // I believe it is used to get terminal information by our default writer function. - // That said, I am registering it here instead of in the function specifically - // because we don't currently support registering parameterized syscalls. - ( - libc::SYS_ioctl, - or![and![Cond::new( - 1, - ArgLen::Dword, - Eq, - #[cfg(all( - target_arch = "x86_64", - target_vendor = "unknown", - target_os = "linux", - target_env = "musl" - ))] - libc::TCGETS.try_into()?, - #[cfg(not(all( - target_arch = "x86_64", - target_vendor = "unknown", - target_os = "linux", - target_env = "musl" - )))] - libc::TCGETS, - )?]], - ), - // `futex` is needed for some tests that run in parallel (`simple_test_parallel`, - // and `callback_test_parallel`). - (libc::SYS_futex, vec![]), - // `sched_yield` is needed for many synchronization primitives that may be invoked - // on the host function worker thread - (libc::SYS_sched_yield, vec![]), - // `mprotect` is needed by malloc during memory allocation - (libc::SYS_mprotect, vec![]), - // `openat` is marked allowed here because it may be called by `libc::free()` - // since it will try to open /proc/sys/vm/overcommit_memory (https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/malloc-sysdep.h;h=778d8971d53e284397c3a5dcdd923e93be5e4731;hb=HEAD) - // We have another more restrictive filter for it below so it will return EACCES instead of trap, in which case libc will use the default value - (libc::SYS_openat, vec![]), - ]) -} - -/// Creates two `BpfProgram`s for a `SeccompFilter` over specific syscalls/`SeccompRule`s -/// intended to be applied on host function threads. -/// -/// Note: This does not provide coverage over the Hyperlight host, which is why we don't need -/// `SeccompRules` for operations we definitely perform but are outside the handler thread -/// (e.g., `KVM_SET_USER_MEMORY_REGION`, `KVM_GET_API_VERSION`, `KVM_CREATE_VM`, -/// or `KVM_CREATE_VCPU`). -pub(crate) fn get_seccomp_filter_for_host_function_worker_thread( - extra_allowed_syscalls: Option<&[ExtraAllowedSyscall]>, -) -> Result> { - let mut allowed_syscalls = syscalls_allowlist()?; - - if let Some(extra_allowed_syscalls) = extra_allowed_syscalls { - allowed_syscalls.extend( - extra_allowed_syscalls - .iter() - .copied() - .map(|syscall| (syscall, vec![])), - ); - - // Remove duplicates - allowed_syscalls.sort_by(|a, b| a.0.cmp(&b.0)); - allowed_syscalls.dedup(); - } - - let arch: TargetArch = std::env::consts::ARCH.try_into()?; - - // Allowlist filter that traps on unknown syscalls - let allowlist = SeccompFilter::new( - allowed_syscalls.into_iter().collect(), - SeccompAction::Trap, - SeccompAction::Allow, - arch, - )? - .try_into()?; - - // If `openat` is an explicitly allowed syscall, we shouldn't return the filter that forces it to return EACCES. - if let Some(extra_syscalls) = extra_allowed_syscalls - && extra_syscalls.contains(&libc::SYS_openat) - { - return Ok(vec![allowlist]); - } - - // Otherwise, we return both filters. - - // Filter that forces `openat` to return EACCES - let errno_on_openat = SeccompFilter::new( - [(libc::SYS_openat, vec![])].into_iter().collect(), - SeccompAction::Allow, - SeccompAction::Errno(libc::EACCES.try_into()?), - arch, - )? - .try_into()?; - - // Note: the order of the 2 filters are important. If we applied the strict filter first, - // we wouldn't be allowed to setup the second filter (would be trapped since the syscalls to setup seccomp are not allowed). - // However, from an seccomp filter perspective, the order of the filters is not important: - // - // If multiple filters exist, they are all executed, in reverse order - // of their addition to the filter tree—that is, the most recently - // installed filter is executed first. (Note that all filters will - // be called even if one of the earlier filters returns - // SECCOMP_RET_KILL. This is done to simplify the kernel code and to - // provide a tiny speed-up in the execution of sets of filters by - // avoiding a check for this uncommon case.) The return value for - // the evaluation of a given system call is the first-seen action - // value of highest precedence (along with its accompanying data) - // returned by execution of all of the filters. - // - // (https://man7.org/linux/man-pages/man2/seccomp.2.html). - // - Ok(vec![errno_on_openat, allowlist]) -} diff --git a/src/hyperlight_host/src/seccomp/mod.rs b/src/hyperlight_host/src/seccomp/mod.rs deleted file mode 100644 index 9edce3ed2..000000000 --- a/src/hyperlight_host/src/seccomp/mod.rs +++ /dev/null @@ -1,39 +0,0 @@ -/* -Copyright 2025 The Hyperlight Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// For more information on seccomp and its implementation in Hyperlight, -// refer to: https://github.com/hyperlight-dev/hyperlight/blob/dev/docs/seccomp.md - -/// This module defines all seccomp filters (i.e., used for blockage of non-specified syscalls) -/// needed for execution of guest code within Hyperlight through a syscalls allow-list. -pub(crate) mod guest; - -// The credit on the creation of the macros below goes to the cloud-hypervisor team -// (https://github.com/cloud-hypervisor/cloud-hypervisor/blob/main/vmm/src/seccomp_filters.rs) - -/// Shorthand for chaining `SeccompCondition`s with the `and` operator in a `SeccompRule`. -/// The rule will take the `Allow` action if _all_ the conditions are true. -#[macro_export] -macro_rules! and { - ($($x:expr),*) => (SeccompRule::new(vec![$($x),*]).unwrap()) -} - -/// Shorthand for chaining `SeccompRule`s with the `or` operator in a `SeccompFilter`. -#[macro_export] -macro_rules! or { - ($($x:expr,)*) => (vec![$($x),*]); - ($($x:expr),*) => (vec![$($x),*]) -} diff --git a/src/hyperlight_host/src/signal_handlers/mod.rs b/src/hyperlight_host/src/signal_handlers/mod.rs index 43a58977b..71bba7326 100644 --- a/src/hyperlight_host/src/signal_handlers/mod.rs +++ b/src/hyperlight_host/src/signal_handlers/mod.rs @@ -18,43 +18,12 @@ use libc::c_int; use crate::sandbox::SandboxConfiguration; -#[cfg(seccomp)] -pub mod sigsys_signal_handler; - pub(crate) fn setup_signal_handlers(config: &SandboxConfiguration) -> crate::Result<()> { // This is unsafe because signal handlers only allow a very restrictive set of // functions (i.e., async-signal-safe functions) to be executed inside them. // Anything that performs memory allocations, locks, and others are non-async-signal-safe. // Hyperlight signal handlers are all designed to be async-signal-safe, so this function // should be safe to call. - #[cfg(seccomp)] - { - use std::sync::Once; - - vmm_sys_util::signal::register_signal_handler( - libc::SIGSYS, - sigsys_signal_handler::handle_sigsys, - ) - .map_err(crate::HyperlightError::VmmSysError)?; - - static PANIC_HOOK_INIT: Once = Once::new(); - PANIC_HOOK_INIT.call_once(|| { - let original_hook = std::panic::take_hook(); - // Set a custom panic hook that checks for "DisallowedSyscall" - std::panic::set_hook(Box::new(move |panic_info| { - // Check if the panic payload matches "DisallowedSyscall" - if let Some(crate::HyperlightError::DisallowedSyscall) = panic_info - .payload() - .downcast_ref::( - ) { - // Do nothing to avoid superfluous syscalls - return; - } - // If not "DisallowedSyscall", use the original hook - original_hook(panic_info); - })); - }); - } vmm_sys_util::signal::register_signal_handler( libc::SIGRTMIN() + config.get_interrupt_vcpu_sigrtmin_offset() as c_int, vm_kill_signal, diff --git a/src/hyperlight_host/src/signal_handlers/sigsys_signal_handler.rs b/src/hyperlight_host/src/signal_handlers/sigsys_signal_handler.rs index 0202873f5..dc0572d55 100644 --- a/src/hyperlight_host/src/signal_handlers/sigsys_signal_handler.rs +++ b/src/hyperlight_host/src/signal_handlers/sigsys_signal_handler.rs @@ -14,93 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -#[cfg(seccomp)] -pub(super) extern "C" fn handle_sigsys( - signal: i32, - info: *mut libc::siginfo_t, - context: *mut libc::c_void, -) { - #[cfg(target_arch = "x86_64")] - { - unsafe { - // si_code contains the reason for the SIGSYS signal. - // SYS_SECCOMP is 1 as per: - // https://github.com/torvalds/linux/blob/81983758430957d9a5cb3333fe324fd70cf63e7e/include/uapi/asm-generic/siginfo.h#L301C9-L301C21 - const SYS_SECCOMP: libc::c_int = 1; - // Sanity checks to make sure SIGSYS was triggered by a BPF filter. - // If something else triggered a SIGSYS (i.e., kill()), we do nothing. - // Inspired by Chromium's sandbox: - // https://chromium.googlesource.com/chromium/chromium/+/master/sandbox/linux/seccomp-bpf/sandbox_bpf.cc#572 - if signal != libc::SIGSYS - || (*info).si_code != SYS_SECCOMP - || context.is_null() - || (*info).si_errno < 0 - { - let err_msg = - b"[ERROR][HYPERLIGHT] SIGSYS triggered by something other than a BPF filter\n"; - libc::write( - libc::STDERR_FILENO, - err_msg.as_ptr() as *const _, - err_msg.len(), - ); - return; - } - - let err_msg = b"[ERROR][HYPERLIGHT] Handling disallowed syscall\n"; - libc::write( - libc::STDERR_FILENO, - err_msg.as_ptr() as *const _, - err_msg.len(), - ); - - // We get the syscall number by accessing a particular offset in the `siginfo_t` struct. - // This only works because this is handling a SIGSYS signal (i.e., the `siginfo_t` struct - // is implemented as a union in the kernel: - // https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/siginfo.h). - // Note: This is not necessarily platform-agnostic, so we might want to be more careful here - // in the future. - const SI_OFF_SYSCALL: isize = 6; - let syscall = *(info as *const i32).offset(SI_OFF_SYSCALL) as usize; - let syscall_bytes = raw_format(b"[ERROR][HYPERLIGHT] Disallowed Syscall: ", syscall); - - // `write` as per https://man7.org/linux/man-pages/man7/signal-safety.7.html - // is async-signal-safe. - libc::write( - libc::STDERR_FILENO, - syscall_bytes.as_ptr() as *const _, - syscall_bytes.len(), - ); - - // Note: This is not necessarily platform-agnostic, so we might want to be more careful here - // in the future. - let ucontext = context as *mut libc::ucontext_t; - let mcontext = &mut (*ucontext).uc_mcontext; - - if syscall == libc::SYS_ioctl as usize { - let ioctl_param = mcontext.gregs[libc::REG_EBRACE as usize] as usize; - let ioctl_param_bytes = - raw_format(b"[ERROR][HYPERLIGHT] IOCTL Param: ", ioctl_param); - libc::write( - libc::STDERR_FILENO, - ioctl_param_bytes.as_ptr() as *const _, - ioctl_param_bytes.len(), - ); - } - - // We don't want to return execution to the offending host function, so - // we alter the RIP register to point to a function that will panic out of - // the host function call. - mcontext.gregs[libc::REG_RIP as usize] = - after_syscall_violation as usize as libc::greg_t; - } - } - - #[cfg(not(target_arch = "x86_64"))] - { - compile_error!("Unsupported architecture for seccomp feature"); - } -} - extern "C-unwind" fn after_syscall_violation() { #[allow(clippy::panic)] std::panic::panic_any(crate::HyperlightError::DisallowedSyscall); diff --git a/src/hyperlight_host/tests/integration_test.rs b/src/hyperlight_host/tests/integration_test.rs index 0869a9d5a..b252aa947 100644 --- a/src/hyperlight_host/tests/integration_test.rs +++ b/src/hyperlight_host/tests/integration_test.rs @@ -49,14 +49,8 @@ fn interrupt_host_call() { Ok(()) }; - #[cfg(any(target_os = "windows", not(seccomp)))] usbox.register("Spin", spin).unwrap(); - #[cfg(seccomp)] - usbox - .register_with_extra_allowed_syscalls("Spin", spin, vec![libc::SYS_clock_nanosleep]) - .unwrap(); - let mut sandbox: MultiUseSandbox = usbox.evolve().unwrap(); let interrupt_handle = sandbox.interrupt_handle(); assert!(!interrupt_handle.dropped()); // not yet dropped diff --git a/src/tests/rust_guests/simpleguest/src/main.rs b/src/tests/rust_guests/simpleguest/src/main.rs index 2c23368e6..eda988f33 100644 --- a/src/tests/rust_guests/simpleguest/src/main.rs +++ b/src/tests/rust_guests/simpleguest/src/main.rs @@ -720,19 +720,6 @@ fn twenty_four_k_in_eight_k_out(function_call: &FunctionCall) -> Result> } } -fn violate_seccomp_filters(function_call: &FunctionCall) -> Result> { - if function_call.parameters.is_none() { - let res = call_host_function::("MakeGetpidSyscall", None, ReturnType::ULong)?; - - Ok(get_flatbuffer_result(res)) - } else { - Err(HyperlightGuestError::new( - ErrorCode::GuestFunctionParameterTypeMismatch, - "Invalid parameters passed to violate_seccomp_filters".to_string(), - )) - } -} - fn call_given_paramless_hostfunc_that_returns_i64(function_call: &FunctionCall) -> Result> { if let ParameterValue::String(hostfuncname) = function_call.parameters.clone().unwrap()[0].clone() @@ -1416,14 +1403,6 @@ pub extern "C" fn hyperlight_main() { ); register_function(add_to_static_and_fail_def); - let violate_seccomp_filters_def = GuestFunctionDefinition::new( - "ViolateSeccompFilters".to_string(), - Vec::new(), - ReturnType::ULong, - violate_seccomp_filters as usize, - ); - register_function(violate_seccomp_filters_def); - let echo_float_def = GuestFunctionDefinition::new( "EchoFloat".to_string(), Vec::from(&[ParameterType::Float]), diff --git a/src/tests/rust_guests/witguest/Cargo.lock b/src/tests/rust_guests/witguest/Cargo.lock index 1cf11ca57..6172a003c 100644 --- a/src/tests/rust_guests/witguest/Cargo.lock +++ b/src/tests/rust_guests/witguest/Cargo.lock @@ -396,9 +396,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.101" +version = "1.0.102" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89ae43fd86e4158d6db51ad8e2b80f313af9cc74f5c0e03ccb87de09998732de" +checksum = "8e0f6df8eaa422d97d72edcd152e1451618fed47fabbdbd5a8864167b1d4aff7" dependencies = [ "unicode-ident", ]