FreeBSD RESOLVE_BENEATH support #296

valpackett · 2023-02-21T08:17:05Z

wheeeee! Overall the code is pretty simple but rustix::fs is getting a bit messier.

the support check I've implemented simply by probing AT_RESOLVE_BENEATH support because it seems the simplest, I don't want to copy version parsing code from the Android thing…
- we technically could also support older FreeBSD versions in capability mode by checking that, even if AT_RESOLVE_BENEATH is not a thing, openat(fd, "/") results in ENOTCAPABLE, but this would also require making the AT_RESOLVE_BENEATH flag conditional on the result, the detection state would be like an atomic enum Unknown | FullSupport | OldSandboxed | NoSupport and aghhhh old versions aren't worth it at all
linkat could be supported too, but it only sandboxes fd1, i.e. linkat(fd1, "../one", fd2, "two") is rejected, but linkat(fd1, "one", fd2, "../two") just does it, so via_parent::open_parent would need to be used on the target, and that's currently pub(super) along with the MaybeOwnedFd type and everything :/
- that escape wasn't getting caught by the tests when I tried to just use an implementation without doing that! I hope the test suite don't miss other possible escapes
yeah, openat(openat(AT_FDCWD, "/"), "..") does not return an error because we're already at the root and going .. doesn't actually escape anything, we're still at the root, haha

Waiting for: bytecodealliance/rustix#541

sunfishcode · 2023-02-23T15:30:17Z

wheeeee! Overall the code is pretty simple but rustix::fs is getting a bit messier.

This is cool, thanks for working on this!

* the support check I've implemented simply by probing `AT_RESOLVE_BENEATH` support because it seems the simplest, I don't want to copy version parsing code from the Android thing…

Entirely reasonable.

* `linkat` could be supported too, but it only sandboxes `fd1`, i.e. `linkat(fd1, "../one", fd2, "two")` is rejected, but `linkat(fd1, "one", fd2, "../two")` just does it, so `via_parent::open_parent` would need to be used on the target, and that's currently `pub(super)` along with the `MaybeOwnedFd` type and everything :/
  * that escape wasn't getting caught by the tests when I tried to just use an implementation without doing that! I hope the test suite don't miss other possible escapes

Good catch; we should add more tests here.

* yeah, `openat(openat(AT_FDCWD, "/"), "..")` does not return an error because we're already at the root and going `..` doesn't actually escape anything, we're still at the root, haha

O_RESOLVE_BENEATH is a new feature in FreeBSD; I wonder if it would be possible to report this as a bug and get this fixed. In any RESOLVE_BENEATH use case I can think of, the caller of open shouldn't know
, for compatibility with Linux RESOVLVE_BENEATH, which does fail on "..".

Waiting for: bytecodealliance/rustix#541

I'm behind on my PR reviews at the moment, but I'll get to it!

sunfishcode · 2023-02-26T09:45:48Z

I filed https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269780 to see if FreeBSD would change the behavior of .. in /.

sunfishcode · 2023-04-06T16:39:47Z

FreeBSD maintainers have not responded in a while in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269780, and in any case, the current release behavior of RESOLVE_BENEATH succeeds on /... My inclination is that cap-std should not pass this behavior on to its users, and that we can implement this by adding a #[cfg(target_os = "freebsd")] boolean field to Dir which records whether the directory could be root, and if it could be, we should use manually::open instead of open with RESOLVE_BENEATH. I can implement this is you'd like.

valpackett · 2023-04-06T21:45:54Z

Hmm, I'm not sure that rather harmless and uncommon edge case is worth the effort tbh, I'm curious though, how would that tracking work?

sunfishcode · 2023-04-10T20:16:13Z

I'm a little torn here. I agree that the /.. issue is pretty obscure. At the same time though, the concepts of "the root filesystem" and preventing applications from being aware of the configuration of the filesystem namespace outside of what they've been granted are fundamental to what cap-std is all about.

Thinking about this more though, adding a flag to Dir would probably get pretty cumbersome, especially with the current cap-std vs cap-primitives split. So what about the following:

In the beneath_supported function, before checking WORKING, try an open with start and ".." and O_RESOLVE_BENEATH, and if that succeeds (or maybe also if it fails with an error other than ENOTCAPABLE), then return false.

We could do something more complex if we care about the possibility of a chroot racing with this code from another thread, but chroot requires an effective uid of 0, and the idea of a multi-threaded program doing a chroot on one thread to a directory that another thread is doing cap-std calls on while also not tripping FreeBSD's kern.chroot_allow_open_directories check is extraordinarily obscure, so I'd be ok ignoring that for now.

That's admittedly not free; it is an extra system call. With a little cleverness, we could at least avoid doing it in the case where the provided path has exactly one component (unless it turns out to be a symlink), which is a pretty common case.

valpackett · 2023-04-11T07:49:25Z

chroot requires an effective uid of 0

Ha—security.bsd.unprivileged_chroot might become enabled by default in the future :) But yes, I'm sure the allow-open-directories check completely disallows this kind of behavior. Races shouldn't be possible at all, as the chroot() and the VFS lookup used by openat() synchronize by exclusively locking the struct pwd belonging to the whole process—either the directory will be opened first and the chroot will be denied, or chroot will happen first and the directory will be searched inside of the chroot.

Waaaaait.

*facepalm*

What are we even talking about. The flag for checking whether the Dir is the root is trivial, isn't it? We can only acquire a Dir of / by requesting that from ambient authority; preventing any other way of getting that is kind of the whole point. ~~I'll implement that :)~~ Riiight, it's more the whole "adding a flag to Dir would probably get pretty cumbersome" thing

valpackett · 2023-05-01T18:32:28Z

The change of the /.. behavior has landed in -STABLE and will be available in the next 13.x 🎉

valpackett · 2023-09-06T06:17:16Z

Rebased. Had to fix a couple unrelated CI errors…

sunfishcode

Cool, thanks for working on this! Overall this looks good.

Could you add a test for the openat(openat(AT_FDCWD, "/"), "..") behaior?

sunfishcode · 2023-09-19T13:14:02Z

cap-primitives/src/rustix/fs/oflags.rs

@@ -13,16 +13,7 @@ pub(in super::super) fn compute_oflags(options: &OpenOptions) -> io::Result<OFla
        oflags |= OFlags::SYNC;
    }
    if options.dsync {
-        #[cfg(not(target_os = "freebsd"))]


What does FreeBSD 12 do with the DSYNC flag? Would it useful to continue to set SYNC on FreeBSD <= 12?

It would ignore it, and… well, useful but probably not worth trying to version-detect or anything for a release branch that should become EoL by the end of this year? (Or, this can wait, really not an important part of the PR for me)

Yeah, it's an admittedly minor detail, but would you mind splitting this out into a separate PR?

sunfishcode · 2023-09-19T14:54:52Z

tests/fs.rs

    let metadata0 = check!(tmpdir.metadata(filename));
-    assert_eq!(mask & metadata0.permissions().mode(), 0);


This test is ported from a corresponding test in Rust's standard library, so ideally we should leave the test as-is and change cap-std to pass it. Can you say more about what this change is for?

As mentioned in the commit message this was failing with EPERM on FreeBSD… at some point for me, anyway, but it seems to pass for me now. Maybe it was due to me screwing around with my in-development kernel patches, oops

If the change isn't needed, would you mind reverting it?

Yeah, of course, I only haven't pushed the removal due to waiting on the DSYNC decision :)

valpackett · 2023-09-19T15:30:31Z

Could you add a test for the openat(openat(AT_FDCWD, "/"), "..") behaior?

Well, that would currently fail the CI upon reenabling cirrus, as the changed behavior hasn't made it into a release yet.

valpackett · 2023-10-13T20:26:00Z

cleaned up now!

sunfishcode

Looks good, thanks for working on this!

(Available since 13.0)

These do not apply (anymore?)

sunfishcode · 2023-12-28T17:50:20Z

Sorry for being slow here! I've now rebased this on #338 which fixes the CI failure.

valpackett force-pushed the freebsd branch from c571a3a to e6873c3 Compare February 21, 2023 18:22

valpackett force-pushed the freebsd branch from e6873c3 to d0cc6c4 Compare March 30, 2023 02:59

valpackett marked this pull request as ready for review March 30, 2023 02:59

sunfishcode force-pushed the main branch from f2a2291 to 76ea8cb Compare April 12, 2023 18:10

valpackett force-pushed the freebsd branch 4 times, most recently from 6b6dfd2 to c711e66 Compare September 6, 2023 06:02

sunfishcode reviewed Sep 19, 2023

View reviewed changes

valpackett force-pushed the freebsd branch from 53d7f2c to e312746 Compare October 13, 2023 19:23

sunfishcode reviewed Nov 1, 2023

View reviewed changes

sunfishcode force-pushed the freebsd branch from e312746 to b82832d Compare November 1, 2023 22:53

valpackett added 4 commits December 28, 2023 09:43

Update FreeBSD versions in Cirrus CI

d926d79

Enable O_PATH usage on FreeBSD

4930209

(Available since 13.0)

tests/fs_additional: fix incorrect FreeBSD specifics

9af71bd

These do not apply (anymore?)

Use AT_/O_RESOLVE_BENEATH on FreeBSD >= 13.0, fixes bytecodealliance#180

27ff2a8

sunfishcode force-pushed the freebsd branch from b82832d to 27ff2a8 Compare December 28, 2023 17:46

sunfishcode merged commit 5e32356 into bytecodealliance:main Dec 28, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeBSD RESOLVE_BENEATH support #296

FreeBSD RESOLVE_BENEATH support #296

valpackett commented Feb 21, 2023

sunfishcode commented Feb 23, 2023

sunfishcode commented Feb 26, 2023

sunfishcode commented Apr 6, 2023

valpackett commented Apr 6, 2023

sunfishcode commented Apr 10, 2023

valpackett commented Apr 11, 2023 •

edited

valpackett commented May 1, 2023

valpackett commented Sep 6, 2023

sunfishcode left a comment

sunfishcode Sep 19, 2023

valpackett Sep 19, 2023

sunfishcode Oct 11, 2023

sunfishcode Sep 19, 2023

valpackett Sep 19, 2023

sunfishcode Oct 11, 2023

valpackett Oct 13, 2023

valpackett commented Sep 19, 2023

valpackett commented Oct 13, 2023

sunfishcode left a comment

sunfishcode commented Dec 28, 2023

		let metadata0 = check!(tmpdir.metadata(filename));
		assert_eq!(mask & metadata0.permissions().mode(), 0);

FreeBSD RESOLVE_BENEATH support #296

FreeBSD RESOLVE_BENEATH support #296

Conversation

valpackett commented Feb 21, 2023

sunfishcode commented Feb 23, 2023

sunfishcode commented Feb 26, 2023

sunfishcode commented Apr 6, 2023

valpackett commented Apr 6, 2023

sunfishcode commented Apr 10, 2023

valpackett commented Apr 11, 2023 • edited

valpackett commented May 1, 2023

valpackett commented Sep 6, 2023

sunfishcode left a comment

Choose a reason for hiding this comment

sunfishcode Sep 19, 2023

Choose a reason for hiding this comment

valpackett Sep 19, 2023

Choose a reason for hiding this comment

sunfishcode Oct 11, 2023

Choose a reason for hiding this comment

sunfishcode Sep 19, 2023

Choose a reason for hiding this comment

valpackett Sep 19, 2023

Choose a reason for hiding this comment

sunfishcode Oct 11, 2023

Choose a reason for hiding this comment

valpackett Oct 13, 2023

Choose a reason for hiding this comment

valpackett commented Sep 19, 2023

valpackett commented Oct 13, 2023

sunfishcode left a comment

Choose a reason for hiding this comment

sunfishcode commented Dec 28, 2023

valpackett commented Apr 11, 2023 •

edited