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

Tracking Issue for directory handles #120426

Open
1 of 7 tasks
the8472 opened this issue Jan 27, 2024 · 23 comments
Open
1 of 7 tasks

Tracking Issue for directory handles #120426

the8472 opened this issue Jan 27, 2024 · 23 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-fuchsia Operating system: Fuchsia T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@the8472
Copy link
Member

the8472 commented Jan 27, 2024

Feature gate: #![feature(dirfd)]

This is a tracking issue for directory handles. Such handles provide a stable reference to an underlying filesystem object (typically directories) that are less vulnerable to TOCTOU attacks and similar races. These security properties will be platform-dependent. Platforms that don't provide the necessary primitives will fall back to operations on absolute paths.

Additionally they may also provide performance benefits by avoiding repeated path lookups when performing many operations on a directory.

Sandboxing is a non-goal. If a platform supports upwards path traversal via .. or symlinks then directory handles will not prevent that. Providing O_BENEATH-style traversal is left to 3rd-party crates or future extensions.

Public API

impl Dir {
     pub fn open<P: AsRef<Path>>(&self, path: P) -> Result<File>
     /// This could be put on OpenOptions instead
     pub fn open_with<P: AsRef<Path>>(&self, path: P, options: &OpenOptions) -> Result<File>
     pub fn create_dir<P: AsRef<Path>>(&self, path: P) -> Result<()>
     pub fn rename<P: AsRef<Path>, Q: AsRef<Path>>(&self, from: P, to_dir: &Self, to: Q) -> Result<()>
     pub fn remove_file<P: AsRef<Path>>(&self, path: P) -> Result<()>
     pub fn remove_dir<P: AsRef<Path>>(&self, path: P) -> Result<()>
     pub fn symlink<P: AsRef<Path>, Q: AsRef<Path>>(&self, original: P, link: Q)
     
     /// ... more convenience methods
}

impl DirEntry {
    pub fn open(&self) -> Result<File>
    /// This could be put on OpenOptions instead
    pub fn open_with(&self, options: &OpenOptions) -> Result<File>
    pub fn remove_file(&self) -> Result<()>
    pub fn remove_dir(&self) -> Result<()>
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@the8472 the8472 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jan 27, 2024
@the8472
Copy link
Member Author

the8472 commented Jan 27, 2024

During the ACP a survey of platform support was requested to make sure that fallbacks won't be needed on Tier-1 platforms.

libc/syscall survey

For restricted conversion between *DIR and a file descriptor we need fdopendir + dirfd. For free conversion we need an alternative to readdir that operates on file descriptors.

Presence of *at functions was surveyed by looking at the libc crate, so this may be incomplete.

Tier 1

Windows

  • openat: NtCreateFile
  • readdir: GetFileInformationByHandleEx + FILE_FULL_DIR_INFO.
  • renameat:SetFileInformationByHandle + FILE_RENAME_INFORMATION
  • unlinkat: SetFileInformationByHandle + FILE_DISPOSITION_INFO_EX
  • mkdirat: NtCreateFile
  • symlinkat: DeviceIoControl + FSCTL_SET_REPARSE_POINT
  • fstatat: either NtQueryInformationByName (if available) or GetFileInformationByHandle and GetFileInformationByHandleEx + FILE_ATTRIBUTE_TAG_INFO

Linux

  • readdir: getdents
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

macos

  • readdir: getattrlistbulk
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

Tier 2

freebsd, illumos, netbsd, solaris

  • readdir: getdents
  • have fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

fuchsia

  • readdir: ReadDirents
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

ios

  • readdir: getattrlistbulk
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

wasm-emscripten

  • readdir: getdents
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

wasm-wasi

  • readdir: fd_readdir
  • has fdopendir, dirfd, openat, renameat, unlinkat, mkdirat, symlinkat, fstatat

redox

libc only lists renameat, unlinkat, symlinkat, fstatat

The redox syscall crate lists no *at calls at all?

uefi

Listed as no_std in the platform support documentation. Somehow has some pieces in std anyway. No libc support either.

Not checked

  • tier 3 targets
  • fortanix

@the8472 the8472 added the A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` label Jan 27, 2024
@jackpot51
Copy link
Contributor

Regarding redox, relibc is the right place to look for support. We should be able to support the same list of functions as Linux and the BSDs.

@zopsicle
Copy link
Contributor

zopsicle commented Jan 27, 2024

The ACP mentions “it may be possible to add the Dir methods directly to ReadDir instead”. This is problematic because ReadDir: !From<OwnedFd> (#56777). Being able to open a file with File::open, then check its file type using Metadata::file_type and convert it to a Dir object if it is a directory would be very useful IMO, for instance to hash a tree of files. That said it may be better to just address #56777 than to have the design of #120426 hinge on this.

@RalfJung
Copy link
Member

From a Miri perspective I'd love to see this, it would let us implement support for emulation of openat and similar functions in a cross-platform way. :)

portable, insecure openat emulation based on Paths

I am somewhat surprised to see this TODO item. In other areas Rust chose to not provide lower-quality fallback implementations, e.g. famously Rust atomics are always hardware atomics and never lock emulated. I can't find discussion of this point in the ACP either. What is the reasoning here for preferring an insecure emulation over a security guarantee stated in the API?

@the8472
Copy link
Member Author

the8472 commented Jan 28, 2024

Operating on a directory is a fairly basic operation, like opening a File. If we returned an error that basically says "we could do that, just the way you're opening it is unsupported on this system" then it would become less portable and likely only interesting for tools like sudo which won't support such platforms anyway.

Dir isn't just about security. It provides a handle to a directory that should keep working even if the directory gets renamed (granted, fallbacks don't provide this). It helps with performance. It allows replacing global process state (the current working directory) with local state. It can prevent non-malicious races.
So applications that aren't security-sensitive would still want to use Dir in their filesystem code.

Additionally, the platforms where openat isn't available are more likely to be single-user systems without privilege separation, on those fallbacks wouldn't be an issue anyway. Afaik UEFI has none of that.

That said, maybe we can add a fail_closed flag, impl Dir { const IS_RACEFREE: bool } or something like that for applications that need some extra assurances.

Also, this wouldn't be a runtime fallback but a platform-based fallback. All tier-1 platforms will use real handles and fail if they're not available. E.g. if something blocks openat via seccomp on linux I do not intend to try open.

@addisoncrump
Copy link

Also, this wouldn't be a runtime fallback but a platform-based fallback.

Can this be completed with a compiler warning for these platforms? Since this will be something included in the target type, I would imagine so.

@the8472
Copy link
Member Author

the8472 commented Sep 30, 2024

@rustbot ping fuchsia

Since it appears that functions being available in the libc crate is not a reliable indicatior that they actually work, is there a better overview of the available posix APIs? I'm interested in the file descriptor APIs listed in #120426 (comment)

@rustbot rustbot added the O-fuchsia Operating system: Fuchsia label Sep 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

Hey friends of Fuchsia! This issue could use some guidance on how this should be
resolved/implemented on Fuchsia. Could one of you weigh in?

cc @djkoloski @erickt @P1n3appl3 @tmandry

@erickt
Copy link
Contributor

erickt commented Sep 30, 2024

@the8472 how odd, we do support it at the Fuchsia layer, maybe we're not exposing it through fdio. We'll do some investigation and report back.

@erickt
Copy link
Contributor

erickt commented Oct 1, 2024

@the8472 got confirmation from our team that we do support dirfd on fuchsia, so this is just a bug we'll get fixed.

@cgwalters
Copy link
Contributor

Sandboxing is a non-goal. If a platform supports upwards path traversal via .. or symlinks then directory handles will not prevent that. Providing O_BENEATH-style traversal is left to 3rd-party crates or future extensions.

I guess that's a reasonable position, but I would also say that for a lot of the code that would be intentionally switching to these APIs, it often makes sense (when rewriting) to just go to such a sandboxed approach anyways.

I'm a bit surprised this tracking issue doesn't reference cap-std as prior art - I'm sure it's known but we definitely should think about it intentionally.

Especially because when considering things like platform support, there's a lot of prior art that landed there for some raw functionality.

A very important choice to make here is whether the directory is opened as O_PATH (on Linux) or not - cap-std does it this way and I think that's right, but it does have some implications.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jan 8, 2025

I'm a bit surprised this tracking issue doesn't reference cap-std as prior art

If you click through to the API Change Proposal (ACP) you'll see it's prominently mentioned. The tracking issue is just a summary of the proposed API as currently envisioned.

@the8472
Copy link
Member Author

the8472 commented Jan 8, 2025

A very important choice to make here is whether the directory is opened as O_PATH (on Linux) or not - cap-std does it this way and I think that's right, but it does have some implications.

I was thinking of opening them with O_RDONLY by default to get a free conversion to ReadDir and fall back to O_PATH when a directory isn't readable. Though that probably wouldn't be guaranteed anyway since this is rather platform-specific behavior. Some have O_PATH or O_EXEC, others don't.
What implications did you have in mind?

@dead-claudia
Copy link

dead-claudia commented Jan 27, 2025

uefi

Listed as no_std in the platform support documentation. Somehow has some pieces in std anyway. No libc support either.

#100499 is tracking adding std support for UEFI environments where applicable, with the ultimate goal of making it no longer #![no_std] anymore.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 28, 2025
Allow Rust to use a number of libc filesystem calls

This allows Rust on Fuchsia to use a number of function calls from libc:

* dirfd
* fdatasync
* flock with LOCK_EX, LOCK_SH, LOCK_NB, LOCK_UN
* fstatat

cc rust-lang#120426

try-job: dist-various-2
@erickt
Copy link
Contributor

erickt commented Jan 28, 2025

@the8472: lost track of this, but #136213 should fix fuchsia using dirfd and a few other libc filesystem functions.

Urgau added a commit to Urgau/rust that referenced this issue Feb 8, 2025
Allow Rust to use a number of libc filesystem calls

This allows Rust on Fuchsia to use a number of function calls from libc:

* dirfd
* fdatasync
* flock with LOCK_EX, LOCK_SH, LOCK_NB, LOCK_UN
* fstatat

cc rust-lang#120426

try-job: dist-various-2
Urgau added a commit to Urgau/rust that referenced this issue Feb 8, 2025
Allow Rust to use a number of libc filesystem calls

This allows Rust on Fuchsia to use a number of function calls from libc:

* dirfd
* fdatasync
* flock with LOCK_EX, LOCK_SH, LOCK_NB, LOCK_UN
* fstatat

cc rust-lang#120426

try-job: dist-various-2
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 9, 2025
Rollup merge of rust-lang#136213 - erickt:fs, r=Mark-Simulacrum

Allow Rust to use a number of libc filesystem calls

This allows Rust on Fuchsia to use a number of function calls from libc:

* dirfd
* fdatasync
* flock with LOCK_EX, LOCK_SH, LOCK_NB, LOCK_UN
* fstatat

cc rust-lang#120426

try-job: dist-various-2
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this issue Mar 11, 2025
Allow Rust to use a number of libc filesystem calls

This allows Rust on Fuchsia to use a number of function calls from libc:

* dirfd
* fdatasync
* flock with LOCK_EX, LOCK_SH, LOCK_NB, LOCK_UN
* fstatat

cc rust-lang#120426

try-job: dist-various-2
@Ayush1325
Copy link
Contributor

uefi

Listed as no_std in the platform support documentation. Somehow has some pieces in std anyway. No libc support either.

#100499 is tracking adding std support for UEFI environments where applicable, with the ultimate goal of making it no longer #![no_std] anymore.

It is listed with ?, which means work-in-progress std.

@RalfJung
Copy link
Member

Looks like Go is getting something similar, though their version does provide directory isolation: https://go.dev/blog/osroot.

@addisoncrump
Copy link

Looks like Go is getting something similar

Yes, this was deemed effectively necessary to mitigate certain attacks in the Go space, including those listed in the original issue. A few years ago, I raised some concerns in a security advisory discussion about potential hardening issues present in Rust (and subsequently downstream crates) as a result of not having this supported in std. I would link to this if I could find it, but GitHub does not have good search functionality here... 😅 I'll see if I can't find it.

There are of course alternatives in downstream crates, but it would be good to have support for this internally IMO.

@addisoncrump
Copy link

addisoncrump commented Mar 20, 2025

Ah, I found the conversation, but it wasn't on an advisory -- instead it was within an email chain. They suggested I open an RFC at the time but frankly I did not think that I had time to champion that process and simply neglected to do so 🙈

But yes, ReadDir and friends should be using openat/openat2 as this is a standing race condition (i.e., there is no open on DirEntry, you have to expand the path first and then open the full path). IMO, once this is stabilised, it would be pertinent to issue clippy warnings for open(entry.path()) patterns.

@RalfJung
Copy link
Member

RalfJung commented Mar 20, 2025

Solving those race conditions is a separate question from whether things should be isolated with the original root somehow, i.e. whether there should be protection against directory traversal attacks. Sometimes, of course, directory traversal is what one wants, and so far I think the plan for the new APIs is to fix the race conditions, but directory traversal containment is considered out-of-scope.

@ChrisDenton
Copy link
Member

There are of course alternatives in downstream crates, but it would be good to have support for this internally IMO.

Well the ACP has been accepted, it's just waiting on an implementation. Anyone can get started with that if they like. Even if it just implements open on one platform then that would be a good enough starting point in a nightly-only API.

@addisoncrump
Copy link

addisoncrump commented Mar 20, 2025

Solving those race conditions is a separate question from whether things should be isolated with the original root somehow

100%, and the traversal problem would already be handled by this change partially since the OpenOptions may include O_NOFOLLOW (AFAIK).

@addisoncrump
Copy link

Anyone can get started with that if they like.

Time to read the contributor's guide...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-fuchsia Operating system: Fuchsia T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests