diff --git a/crates/fs-mistrust/src/dir.rs b/crates/fs-mistrust/src/dir.rs index 02aff3b5f..f79962263 100644 --- a/crates/fs-mistrust/src/dir.rs +++ b/crates/fs-mistrust/src/dir.rs @@ -17,6 +17,19 @@ use std::os::unix::fs::OpenOptionsExt; /// The accessor functions will enforce that whatever security properties we /// checked on the the directory also apply to all of the members that we access /// within the directory. +/// +/// ## Limitations +/// +/// Having a `CheckedDir` means only that, at the time it was created, we were +/// confident that no _untrusted_ user could access it inappropriately. It is +/// still possible, after the `CheckedDir` is created, that a _trusted_ user can +/// alter its permissions, make its path point somewhere else, or so forth. +/// +/// If this kind of time-of-use/time-of-check issue is unacceptable, you may +/// wish to look at other solutions, possibly involving `openat()` or related +/// APIs. +/// +/// See also the crate-level [Limitations](crate#limitations) section. pub struct CheckedDir { /// The `Mistrust` object whose rules we apply to members of this directory. mistrust: Mistrust, diff --git a/crates/fs-mistrust/src/imp.rs b/crates/fs-mistrust/src/imp.rs index 53005f71d..462830ff3 100644 --- a/crates/fs-mistrust/src/imp.rs +++ b/crates/fs-mistrust/src/imp.rs @@ -71,6 +71,8 @@ impl<'a> super::Verifier<'a> { boxed( rp.filter(should_retain) // Finally, check the path for errors. + // + // See `check_one` below for a note on TOCTOU issues. .flat_map(move |r| match r { Ok((path, path_type, metadata)) => { self.check_one(path.as_path(), path_type, &metadata) @@ -115,12 +117,11 @@ impl<'a> super::Verifier<'a> { std::iter::empty() } - /// Check a single `path` for conformance with this `Concrete` mistrust. + /// Check a single `path` for conformance with this `Verifier`. /// - /// `position` is the position of the path within the ancestors of the - /// target path. If the `position` is 0, then it's the position of the - /// target path itself. If `position` is 1, it's the target's parent, and so - /// on. + /// Note that this result is only meaningful if all of the _ancestors_ of + /// this path have been checked. Otherwise, a non-trusted user could change + /// where this path points after it has been checked. #[must_use] pub(crate) fn check_one( &self, diff --git a/crates/fs-mistrust/src/lib.rs b/crates/fs-mistrust/src/lib.rs index 38e2ab992..bc1f3b462 100644 --- a/crates/fs-mistrust/src/lib.rs +++ b/crates/fs-mistrust/src/lib.rs @@ -1,10 +1,32 @@ -//! # `fs-mistrust`: make sure that files are really private. +//! # `fs-mistrust`: check whether file permissions are private. //! -//! This crates provides a set of functionality to check the permissions on -//! files and directories to ensure that they are effectively private—that is, -//! that they are only readable or writable by trusted[^1] users. +//! This crate provides a set of functionality to check the permissions on files +//! and directories to ensure that they are effectively private—that is, that +//! they are only readable or writable by trusted[^1] users. //! -//! That's trickier than it sounds: +//! This kind of check can protect your users' data against misconfigurations, +//! such as cases where they've accidentally made their home directory +//! world-writable, or where they're using a symlink stored in a directory owned +//! by another user. +//! +//! The checks in this crate try to guarantee that, after a path has been shown +//! to be private, no action by a _non-trusted user_ can make that path private. +//! It's still possible for a _trusted user_ to change a path after it has been +//! checked. Because of that, you may want to use other mechanisms if you are +//! concerned about time-of-check/time-of-use issues caused by _trusted_ users +//! altering the filesystem. +//! +//! Also see the [Limitations](#limitations) section below. +//! +//! [^1]: we define "trust" here in the computer-security sense of the word: a +//! user is "trusted" if they have the opportunity to break our security +//! guarantees. For example, `root` on a Unix environment is "trusted", +//! whether you actually trust them or not. +//! +//! ## What's so hard about checking permissions? +//! +//! Suppose that we want to know whether a given path can be read or modified by +//! an untrusted user. That's trickier than it sounds: //! //! * Even if the permissions on the file itself are correct, we also need to //! check the permissions on the directory holding it, since they might allow @@ -38,10 +60,6 @@ //! for file privacy checking and enforcement, along with clear justifications //! in its source for why it behaves that way. //! -//! [^1]: we define "trust" here in the computer-security sense of the word: a -//! user is "trusted" if they have the opportunity to break our security -//! guarantees. For example, `root` on a Unix environment is "trusted", -//! whether you actually trust them or not. //! //! ## What we actually do //! @@ -170,12 +188,20 @@ //! //! ## Limitations //! +//! As noted above, this crate only checks whether a path can be changed by +//! _non-trusted_ users. After the path has been checked, a _trusted_ user can +//! still change its permissions. (For example, the user could make their home +//! directory world-writable.) This crate does not try to defend against _that +//! kind_ of time-of-check/time-of-use issue. +//! //! We currently assume a fairly vanilla Unix environment: we'll tolerate other //! systems, but we don't actually look at the details of any of these: //! * Windows security (ACLs, SecurityDescriptors, etc) //! * SELinux capabilities //! * POSIX (and other) ACLs. //! +//! On Windows, we accept all file permissions and owners. +//! //! We don't check for mount-points and the privacy of filesystem devices //! themselves. (For example, we don't distinguish between our local //! administrator and the administrator of a remote filesystem. We also don't diff --git a/crates/fs-mistrust/src/walk.rs b/crates/fs-mistrust/src/walk.rs index 5e7f45e6c..7d528de93 100644 --- a/crates/fs-mistrust/src/walk.rs +++ b/crates/fs-mistrust/src/walk.rs @@ -54,22 +54,30 @@ pub(crate) enum PathType { /// /// # Implementation notes /// -/// Abstractly, at any given point, the directory that we're resolving looks like -/// `"resolved"/"remaining"`, where `resolved` is the part that we've already -/// looked at (in canonical form, with all symlinks resolved) and `remaining` is -/// the part that we're still trying to resolve. +/// Abstractly, at any given point, the directory that we're resolving looks +/// like `"resolved"/"remaining"`, where `resolved` is the part that we've +/// already looked at (in canonical form, with all symlinks resolved) and +/// `remaining` is the part that we're still trying to resolve. /// /// We represent `resolved` as a nice plain PathBuf, and `remaining` as a stack -/// of strings that we want to push on to the end of the path. We initialize the -/// algorithm with `resolved` empty and `remaining` seeded with the path we want -/// to resolve. Once there are no more parts to push, the path resolution is -/// done. +/// of strings that we want to push on to the end of the path. We initialize +/// the algorithm with `resolved` empty and `remaining` seeded with the path we +/// want to resolve. Once there are no more parts to push, the path resolution +/// is done. /// /// The following invariants apply whenever we are outside of the `next` /// function: /// * `resolved.join(remaining)` is an alias for our target path. /// * `resolved` is in canonical form. /// * Every ancestor of `resolved` is a key of `already_inspected`. +/// +/// # Limitations +/// +/// Because we're using `Path::metadata` rather than something that would use +/// `openat()` and `fstat()` under the hood, the permissions returned here are +/// potentially susceptible to TOCTOU issues. In this crate we address these +/// issues by checking each yielded path immediately to make sure that only +/// _trusted_ users can change it after it is checked. // // TODO: This code is potentially of use outside this crate. Maybe it should be // public?