Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Landlock isolation support #816

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

n0toose
Copy link
Member

@n0toose n0toose commented Nov 30, 2024

Leaving this here for now, the most important design problem with this is how Landlock does not allow whitelisting files if they are not created. (So, we'd have to force the user to use a directory for that instead.)

We also need to avoid "parsing" the same --file-mapping inputs twice, as well as not use OnceLock for enforcing the whitelist when the kernel is actually being loaded. UhyveVm::new is called in a new thread because Landlock enforces the restrictions for the entire process and its children. We're not testing if the sandbox is applied correctly yet.

See: https://docs.kernel.org/userspace-api/landlock.html

Fixes #766

@n0toose n0toose changed the title introduce Landlock isolation support Introduce Landlock isolation support Nov 30, 2024
@n0toose n0toose marked this pull request as draft November 30, 2024 22:24
@n0toose
Copy link
Member Author

n0toose commented Dec 1, 2024

Depends on #814.

@n0toose
Copy link
Member Author

n0toose commented Dec 1, 2024

We need to move the split_guest_and_host functionality that is used during the initialization of UhyveFileMap::new away from that function for two reasons:

@n0toose
Copy link
Member Author

n0toose commented Dec 17, 2024

TODO: Investigate landlock_path_beneath_attr

@n0toose
Copy link
Member Author

n0toose commented Dec 18, 2024

uhyvefilemap_test works. The problem is that you can't run UhyveVm::run multiple times, as the Landlock restrictions have already been applied process-wide once. Additionally, the temporary directory gets dropped after the first instance of UhyveVm::run, so trying to re-enforce Landlock causes an error as the temporary directory has been dropped.

My approach to the problem of "not being able to whitelist files that don't exist yet" is just whitelisting the parent directory and letting UhyveFileMap contain file operations to that one specific file only (in the whitelisted directory). We do that by iterating over the file's parents and establishing whether they exist, once - but this could be made configurable, and it should be fine if we disclose to the user that whitelisting directories is safer.

However, whitelisting a directory is not always practical, because we can't map the entirety of /root to anything yet, as the /root component gets popped by Uhyve before a hypercall is executed (see fs-test.rs and create_file.rs for a workaround), see: https://github.com/hermit-os/kernel/blob/12d4854554f8ec48fa66a8d282ebcf46b7bf9499/src/fs/mod.rs#L160

@n0toose
Copy link
Member Author

n0toose commented Dec 18, 2024

The change incorporates some changes from #844.

@n0toose
Copy link
Member Author

n0toose commented Jan 2, 2025

hermit-os/kernel#1529 is now a hard requirement for this change. CC: @mkroening

@n0toose
Copy link
Member Author

n0toose commented Jan 2, 2025

no idea why fs-related integration tests work locally but fail in the CI, will investigate later

@n0toose n0toose marked this pull request as ready for review January 3, 2025 13:05
@n0toose
Copy link
Member Author

n0toose commented Jan 3, 2025

  • This should work now as soon as feat(uhyve): use absolute paths kernel#1529 (review) is merged and a release on the Hermit kernel's side is made.

  • On the contrary, temporary files will break on main, as joining an absolute path to another PathBuf will replace the PathBuf's contents with the absolute path that is being joined. This change introduces a bunch of changes that fixes this. See:

    let host_path = self.tempdir.path().join(guest_path).into_os_string();

  • This is otherwise ready for review, will need some rebases before being mergeable.

n0toose added a commit to n0toose/uhyve that referenced this pull request Jan 5, 2025
Ported from hermit-os#816, fixes a regression introduced by
hermit-os/kernel#1529,
which modified the Hermit kernel so that it uses absolute paths instead
of relative ones.
@n0toose
Copy link
Member Author

n0toose commented Jan 5, 2025

This PR includes work that was split into separate PRs, #844 and #852, which should probably be merged first. This PR relies on hermit-os/kernel#1529. It includes some changes to our tests that reflect the changes made in hermit-os/kernel#1529.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File Isolation: Add support for Landlock
2 participants