Prevent openat from trapping on seccomp thread, by making it return EACCES instead (hyperlight-dev#573)

* Prevent openat from trapping on seccomp thread, by making it return EACCES instead

Signed-off-by: Ludvig Liljenberg <[email protected]>

* Allow openat to be allowed if specified in extra_allowed_syscalls

Signed-off-by: Ludvig Liljenberg <[email protected]>

---------

Signed-off-by: Ludvig Liljenberg <[email protected]>

Author: ludfjig

src/hyperlight_host/src/sandbox/host_funcs.rs

@@ -162,7 +162,9 @@ fn maybe_with_seccomp<T: Send>(
             .name(format!("Host Function Worker Thread for: {name:?}"))
             .spawn(move |_| {
                 let seccomp_filter = get_seccomp_filter_for_host_function_worker_thread(syscalls)?;
-                seccompiler::apply_filter(&seccomp_filter)?;
+                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

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -458,6 +458,92 @@ mod tests {
         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<i64> {
+            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(Noop::default()).unwrap();
+            let host_func_result = sbox
+                .call_guest_function_by_name::<i64>(
+                    "CallGivenParamlessHostFuncThatReturnsI64",
+                    "Openat_Hostfunc".to_string(),
+                )
+                .expect("Expected to call host function that returns i64");
+
+            if cfg!(feature = "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(feature = "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(Noop::default()).unwrap();
+            let host_func_result = sbox
+                .call_guest_function_by_name::<i64>(
+                    "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(

src/hyperlight_host/src/seccomp/guest.rs

@@ -17,7 +17,7 @@ limitations under the License.
 use seccompiler::SeccompCmpOp::Eq;
 use seccompiler::{
     BpfProgram, SeccompAction, SeccompCmpArgLen as ArgLen, SeccompCondition as Cond, SeccompFilter,
-    SeccompRule,
+    SeccompRule, TargetArch,
 };
 
 use crate::sandbox::ExtraAllowedSyscall;
@@ -59,10 +59,14 @@ fn syscalls_allowlist() -> Result<Vec<(i64, Vec<SeccompRule>)>> {
         (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 a `BpfProgram` for a `SeccompFilter` over specific syscalls/`SeccompRule`s
+/// 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
@@ -71,7 +75,7 @@ fn syscalls_allowlist() -> Result<Vec<(i64, Vec<SeccompRule>)>> {
 /// or `KVM_CREATE_VCPU`).
 pub(crate) fn get_seccomp_filter_for_host_function_worker_thread(
     extra_allowed_syscalls: Option<&[ExtraAllowedSyscall]>,
-) -> Result<BpfProgram> {
+) -> Result<Vec<BpfProgram>> {
     let mut allowed_syscalls = syscalls_allowlist()?;
 
     if let Some(extra_allowed_syscalls) = extra_allowed_syscalls {
@@ -87,11 +91,50 @@ pub(crate) fn get_seccomp_filter_for_host_function_worker_thread(
         allowed_syscalls.dedup();
     }
 
-    Ok(SeccompFilter::new(
+    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,  // non-match syscall will kill the offending thread
-        SeccompAction::Allow, // match syscall will be allowed
-        std::env::consts::ARCH.try_into()?,
-    )
-    .and_then(|filter| filter.try_into())?)
+        SeccompAction::Trap,
+        SeccompAction::Allow,
+        arch,
+    )?
+    .try_into()?;
+
+    // If `openat` is an exclicitly allowed syscall, we shouldn't return the filter that forces it to return EACCES.
+    if let Some(extra_syscalls) = extra_allowed_syscalls {
+        if 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])
 }

src/tests/rust_guests/simpleguest/src/main.rs

@@ -700,6 +700,21 @@ fn violate_seccomp_filters(function_call: &FunctionCall) -> Result<Vec<u8>> {
     }
 }
 
+fn call_given_paramless_hostfunc_that_returns_i64(function_call: &FunctionCall) -> Result<Vec<u8>> {
+    if let ParameterValue::String(hostfuncname) =
+        function_call.parameters.clone().unwrap()[0].clone()
+    {
+        let res = call_host_function::<i64>(&hostfuncname, None, ReturnType::Long)?;
+
+        Ok(get_flatbuffer_result(res))
+    } else {
+        Err(HyperlightGuestError::new(
+            ErrorCode::GuestFunctionParameterTypeMismatch,
+            "Invalid parameters passed to test_rust_malloc".to_string(),
+        ))
+    }
+}
+
 fn add(function_call: &FunctionCall) -> Result<Vec<u8>> {
     if let (ParameterValue::Int(a), ParameterValue::Int(b)) = (
         function_call.parameters.clone().unwrap()[0].clone(),
@@ -1137,6 +1152,14 @@ pub extern "C" fn hyperlight_main() {
         large_parameters as usize,
     );
     register_function(large_parameters_def);
+
+    let call_given_hostfunc_def = GuestFunctionDefinition::new(
+        "CallGivenParamlessHostFuncThatReturnsI64".to_string(),
+        Vec::from(&[ParameterType::String]),
+        ReturnType::Long,
+        call_given_paramless_hostfunc_that_returns_i64 as usize,
+    );
+    register_function(call_given_hostfunc_def);
 }
 
 #[no_mangle]