Create sandbox for host file system access #783

n0toose · 2024-11-05T19:42:15Z

Add --mount parameter for "whitelisting" guest_paths and defining their respective filesystem paths on the host FS
Add UhyveFileMap structure
Add sandbox support to open() syscall

A few points that could be further worked are unit tests, handling more of the parsing using the clap library directly and performance optimizations.

Fixes #767

n0toose · 2024-11-05T19:45:35Z

Tasks that should be done:

Unit tests for UhyveFileMap structure. (Nevertheless, a review would be appreciated.)

Additional ideas:

Linux-only configuration option for storing any files that were not explicitly defined in /tmp.
Option that allows explicitly locks down filesystem access completely, instead of giving full host access if no --mount parameter exists.

codecov · 2024-11-05T19:45:35Z

Codecov Report

Attention: Patch coverage is 93.93939% with 10 lines in your changes missing coverage. Please review.

Project coverage is 69.37%. Comparing base (0cbbc3a) to head (2f5355f).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/isolation.rs	96.21%	5 Missing ⚠️
src/bin/uhyve.rs	0.00%	3 Missing ⚠️
src/hypercall.rs	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
+ Coverage   67.84%   69.37%   +1.52%     
==========================================
  Files          19       20       +1     
  Lines        2454     2609     +155     
==========================================
+ Hits         1665     1810     +145     
- Misses        789      799      +10

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

src/vm.rs

jounathaen · 2024-11-06T10:09:39Z

src/hypercall.rs

+		// In that case, converting a CStr to str will return a Utf8Error.
+		//
+		// See: https://nrc.github.io/big-book-ffi/reference/strings.html
+		let guest_path = unsafe { CStr::from_ptr(requested_path) }.to_str();


I'd much prefer a checked safe variant here

I don't see any better way of doing this in https://doc.rust-lang.org/std/ffi/struct.CStr.html

I presumed to_str() to be "good enough" given that the type is a Result(&str, Utf8Error) and basically relies on Rust itself to protect against malicious input, no?

src/hypercall.rs

src/isolation.rs

src/params.rs

jounathaen · 2024-11-06T10:22:56Z

src/vm.rs

@@ -149,6 +151,8 @@ impl<VCpuType: VirtualCPU> UhyveVm<VCpuType> {
 			"gdbstub is only supported with one CPU"
 		);

+		let file_map = params.mount.as_deref().and_then(UhyveFileMap::new);


Reads a bit more complicated than UhyveFileMap::new(&params.mount) if you ask me.

Reads a bit more complicated than UhyveFileMap::new(&params.mount) if you ask me.

Perhaps. But, if the specific parameter is missing, this helps us spare the function call as well as checking whether the parameter is none within UhyveFileMap::new itself, as it just straight up instantly returns None in that case with less lines of code.

n0toose · 2024-11-06T11:49:26Z

Addressing the first round of review comments in https://github.com/n0toose/uhyve/commits/sandbox-uhyvefilemap-review-round-1

Will rebase shortly and force-push to sandbox-uhyvefilemap.

* Add --mount parameter for "whitelisting" guest_paths and defining their respective filesystem paths on the host FS * Add UhyveFileMap structure * Add sandbox support to open() syscall A few points that could be further worked are unit tests, handling more of the parsing using the clap library directly and performance optimizations. Helped-by: Çağatay Yiğit Şahin <[email protected]> Helped-by: Jonathan Klimt <[email protected]>

Also removes the "Introduction" header, as the header takes more space than the text itself now. Originally introduced in e7869a9.

n0toose · 2024-11-06T12:35:23Z

src/isolation.rs

+		// Mind the order.
+		// TODO: Do this work using clap.
+		let host_path = OsString::from(partsiter.next().unwrap());
+		let guest_path = partsiter.next().unwrap().to_owned();


--file-map "" results in:

thread 'main' panicked at src/isolation.rs:44:43: called `Option::unwrap()` on a `None` value

This is "by design" for the time being, but suboptimal - there is definitely room for improvement here. Given that I'm targeting main, should this be improved later down the line or now?

Something that @cagatay-y brought up to me is that using a bunch of .unwrap()'s to make up for invalid inputs is not explicit enough and could result in future problems during a supposed refactoring.

More specifically, I think it is possible that if the function signature is changed to return an Option in the future, it is possible that a None caused by an error in parsing may get confused with the None variant that signifies unsandboxed filesystem access.

n0toose · 2024-11-07T10:14:59Z

TODO for this PR:

fmt::Debug for UhyveFileMap
unit test for the UhyveFileMap structure

jounathaen · 2024-11-12T13:34:33Z

src/bin/uhyve.rs

+	/// Paths that the kernel should be able to view, read or write.
+	///
+	/// Files and directories are separated using commas.
+	/// Desired mount paths must be explicitly defined after a colon.
+	///
+	/// Example: --file_map host_directory:/root/guest_directory,file.txt:/root/my_file.txt
+	#[arg(value_delimiter = ',')]
+	#[clap(long, env = "HERMIT_FILE_MAP")]
+	file_map: Option<Vec<String>>,


I think it would be a better interface, if we have the separate mounts as separate options. Like uhvye -v a.txt:b.txt -v c.txt:/bla/blub.txt

The thing with the separate options works by default. I removed the delimiter and the environment variable for now, and changed file_map into mount (while still referring to a file_map internally, leaving room for further extensions in the future).

As described in private (after a discussion with @mkroening), the middle ground that I found for this was to call the parameter --mount again without introducing a short. -v is already taken by --verbose (just like -m for --file-map, only -f isn't taken but that's a weird choice).

Introduces a test for UhyveFileMap. Some refactoring was done so as to keep the two tests, fs-test and uhyvefilemap, in separate files. Some filesystem-related functions were moved into common.rs, because: - we anticipate that they will be necessary for further filesystem-related tests - putting two test functions in a single test (e.g. fs-test) causes the second test to hang for some mysterious reason - more descriptive errors

- Rename --file_map to --mount - Temporarily remove short parameter - Temporarily remove environment variable - Don't split file_map params with commas - Change documentation, remove references to file_map variable

n0toose · 2024-11-13T11:02:33Z

Currently working on a proof-of-concept for temporary files tracked under this branch, so as to completely remove unfettered host file system access: https://github.com/n0toose/uhyve/tree/sandbox-uhyvefilemap-tempfile

I'll squash the work done and move it under sandbox-uhyvefilemap once it is done to keep the sandbox-uhyvefilemap tree clean.

n0toose · 2024-11-13T11:30:26Z

My approach so far has been to create a temporary directory and perhaps append the file path to UhyveFileMap, but, as I kind of suspected, this may be an awful idea.

I'm using the crate tempfile: https://docs.rs/tempfile/latest/tempfile/

tempfile will (almost) never fail to cleanup temporary resources. However TempDir and NamedTempFile will fail if their destructors don’t run.

Using a TempDir seems like a solid approach, because it also allows us to use the created path with e.g. Landlock (which can only whitelist existing paths and not append any new paths in retrospect) and reopen files. However, other processes have the ability to write to it, so there's not a considerable security benefit compared to whitelisting /tmp. Which brings me to trusting /tmp...

Security

In the presence of pathological temporary file cleaner, relying on file paths is unsafe because a temporary file cleaner could delete the temporary file which an attacker could then replace.

(Source: https://docs.rs/tempfile/latest/tempfile/#security)

The middle-ground solution seems to be allowing the kernel to write into a file, but not store it internally if it needs to be reused at a later point.

n0toose · 2024-11-13T12:42:38Z

I'll figure out how / if this can be sufficiently prevented: https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File

n0toose force-pushed the sandbox-uhyvefilemap branch from a8ff49a to 38e7e46 Compare November 5, 2024 19:52

n0toose commented Nov 5, 2024

View reviewed changes

src/vm.rs Outdated Show resolved Hide resolved

n0toose changed the title ~~feat(sandbox): Add UhyveFileMap structure and sandbox~~ Create sandbox for host file system access Nov 5, 2024