feat(sandbox): add fd sandbox #857

n0toose · 2025-01-08T21:52:34Z

No description provided.

n0toose · 2025-01-08T21:54:34Z

this was slightly more complex than expected because of unlink, need to take a more careful look at the correctness, but this is a proof of concept

codecov · 2025-01-08T21:54:56Z

Codecov Report

Attention: Patch coverage is 55.66038% with 47 lines in your changes missing coverage. Please review.

Project coverage is 71.31%. Comparing base (da4b63e) to head (6bd9b26).

Files with missing lines	Patch %	Lines
src/isolation/filemap.rs	44.18%	24 Missing ⚠️
src/hypercall.rs	56.81%	19 Missing ⚠️
src/linux/x86_64/kvm_cpu.rs	78.94%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
- Coverage   71.86%   71.31%   -0.56%     
==========================================
  Files          23       23              
  Lines        3064     3134      +70     
==========================================
+ Hits         2202     2235      +33     
- Misses        862      899      +37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

n0toose · 2025-01-08T22:27:50Z

this definitely works with both temporary files and mapped ones (and mapped directories), but breaks with hello_c and rusty_demo, as rusty_demo seemingly directly writes to 2

n0toose · 2025-01-08T22:54:20Z

works now

jounathaen · 2025-01-09T11:01:14Z

Have you also considered the special file descriptors (1,2 & 3) that are opened on the hypervisor per default?

n0toose · 2025-01-09T11:05:00Z

Have you also considered the special file descriptors (1,2 & 3) that are opened on the hypervisor per default?

In my bruteforcing attempts to get this to work, I think I completely forgot about 3 existing.

1 is already being handled by the hypercall, have to investigate the other case scenarios

		if syswrite.fd == 1 {
			// fd 0 is stdout
			let bytes = unsafe {
				parent_vm
					.mem
					.slice_at(guest_phys_addr, syswrite.len)
					.map_err(|e| {
						io::Error::new(
							io::ErrorKind::InvalidInput,
							format!("invalid syswrite buffer: {e:?}"),
						)
					})?
			};

n0toose · 2025-01-09T11:06:56Z

I presume that I'll probably have to take note of stdin's security implications and see if I can handle that case.

n0toose · 2025-01-10T10:57:03Z

All checks pass. This change should probably be manually tested on macOS too, just to be sure. I will leave the commit squashing for later.

n0toose

Given that this change is somewhat complex, I left a few remarks addressing some of its behavior as well as specific points for review, so as to prevent churn / bugs.

n0toose · 2025-01-10T10:59:51Z

src/hypercall.rs

@@ -171,8 +190,7 @@ pub fn write<B: VirtualizationBackend>(
 		)
 		.unwrap();

-		if syswrite.fd == 1 {
-			// fd 0 is stdout
+		if syswrite.fd == 1 || syswrite.fd == 2 {


Not 1000% sure whether this is sane.

Looks fine, at least until we have separate handling for stderr

src/isolation/filemap.rs

src/hypercall.rs

n0toose · 2025-01-10T11:06:39Z

src/hypercall.rs

+pub fn close(sysclose: &mut CloseParams, file_map: &mut UhyveFileMap) {
+	if file_map.get_fd_presence(sysclose.fd.into_raw_fd()) {
+		unsafe { sysclose.ret = libc::close(sysclose.fd) }
+		if sysclose.ret >= 0 {


Is allowing close on the fds 0, 1, 2 violating isolation? Should we handle this in a different way?

I found this comment: https://stackoverflow.com/a/4973065/6551168
So it might be ok for the kernel to close them, but as uhyve still needs them, I think ignoring this close is the way to go.

src/macos/aarch64/vcpu.rs

This extends UhyveFileMap with a sandbox mapping file descriptors to maps, so as to prevent the opening of file descriptors that belong to Uhyve (or to that of other VMs, when Uhyve exposes such functionality to the user). There is no Linux equivalent of unlink that is capable of removing files using a file descriptor. We did not want to keep a file descriptor of an unlinked file available to the VM. Therefore, we used a BiMap that associate fds to guest paths and guest paths to fds (instead of a simple container containing fds).

If an unlink takes place while the file is open, this would make the subsequent close fail, as soon as the file object goes out of scope and gets dropped. We introduced a HashSet that temporarily caches such unlinked file descriptors.

n0toose · 2025-01-10T21:19:53Z

caught a bug that occurs when unlinking a file and then subsequently dropping it - unlinked files present in the map will cause an error later, as we'd have already removed the fd during an unlink. this keeps it in a HashSet until it is actually dropped - this only happens if the file is already present in the fdmap map<>fd association

n0toose requested a review from jounathaen January 8, 2025 21:52

n0toose marked this pull request as draft January 8, 2025 22:08

n0toose force-pushed the sandbox-fd branch 2 times, most recently from bf3f183 to 2bbae9b Compare January 8, 2025 22:14

n0toose force-pushed the sandbox-fd branch from e9968f6 to e7c408e Compare January 10, 2025 10:08

n0toose marked this pull request as ready for review January 10, 2025 10:56

n0toose commented Jan 10, 2025

View reviewed changes

jounathaen reviewed Jan 10, 2025

View reviewed changes

src/macos/aarch64/vcpu.rs Show resolved Hide resolved

n0toose force-pushed the sandbox-fd branch 3 times, most recently from 823ea51 to 5802165 Compare January 10, 2025 12:09

n0toose added 2 commits January 10, 2025 20:45

fix(hypercall): add syserr to write's fd checks

b1ae5cb

n0toose force-pushed the sandbox-fd branch from 286171f to b1ae5cb Compare January 10, 2025 19:46

fix(sandbox): fix close after unlink

6bd9b26

If an unlink takes place while the file is open, this would make the subsequent close fail, as soon as the file object goes out of scope and gets dropped. We introduced a HashSet that temporarily caches such unlinked file descriptors.

n0toose force-pushed the sandbox-fd branch from bc6bb53 to 6bd9b26 Compare January 10, 2025 21:15

n0toose added 2 commits January 13, 2025 17:42

fix wrong condition

76aa537

fix(fd): drop all non-closed fds when UhyveFileMap is dropped

f63e489

n0toose force-pushed the sandbox-fd branch from 532b5c1 to f63e489 Compare January 18, 2025 23:27

n0toose mentioned this pull request Feb 5, 2025

Make Uhyve aware of process-wide limits #897

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sandbox): add fd sandbox #857

feat(sandbox): add fd sandbox #857

n0toose commented Jan 8, 2025

n0toose commented Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

n0toose commented Jan 8, 2025

n0toose commented Jan 8, 2025

jounathaen commented Jan 9, 2025

n0toose commented Jan 9, 2025

n0toose commented Jan 9, 2025

n0toose commented Jan 10, 2025

n0toose left a comment

n0toose Jan 10, 2025

jounathaen Jan 10, 2025

n0toose Jan 10, 2025

jounathaen Jan 10, 2025

n0toose commented Jan 10, 2025

feat(sandbox): add fd sandbox #857

Are you sure you want to change the base?

feat(sandbox): add fd sandbox #857

Conversation

n0toose commented Jan 8, 2025

n0toose commented Jan 8, 2025

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

n0toose commented Jan 8, 2025

n0toose commented Jan 8, 2025

jounathaen commented Jan 9, 2025

n0toose commented Jan 9, 2025

n0toose commented Jan 9, 2025

n0toose commented Jan 10, 2025

n0toose left a comment

Choose a reason for hiding this comment

n0toose Jan 10, 2025

Choose a reason for hiding this comment

jounathaen Jan 10, 2025

Choose a reason for hiding this comment

n0toose Jan 10, 2025

Choose a reason for hiding this comment

jounathaen Jan 10, 2025

Choose a reason for hiding this comment

n0toose commented Jan 10, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading