diff --git a/Cargo.lock b/Cargo.lock index 40c1b5977..3508572fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1197,6 +1197,7 @@ dependencies = [ "libc", "tempfile", "thiserror", + "walkdir", ] [[package]] diff --git a/crates/fs-mistrust/Cargo.toml b/crates/fs-mistrust/Cargo.toml index 3fe84713f..3ed1b598f 100644 --- a/crates/fs-mistrust/Cargo.toml +++ b/crates/fs-mistrust/Cargo.toml @@ -10,8 +10,12 @@ keywords = ["fs", "file", "permissions", "ownership", "privacy"] categories = ["filesystem"] repository = "https://gitlab.torproject.org/tpo/core/arti.git/" +[features] +default = [ "walkdir" ] + [dependencies] thiserror = "1" +walkdir = { version = "2", optional = true } [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/crates/fs-mistrust/src/err.rs b/crates/fs-mistrust/src/err.rs index 8b8812cb7..f210fff1c 100644 --- a/crates/fs-mistrust/src/err.rs +++ b/crates/fs-mistrust/src/err.rs @@ -71,6 +71,17 @@ pub enum Error { /// We tried to create a directory, and encountered a failure in doing so. #[error("Problem creating directory")] CreatingDir(#[source] Arc), + + /// We found a problem while checking the contents of the directory. + #[error("Invalid directory content")] + Content(#[source] Box), + + /// We were unable to inspect the contents of the directory + /// + /// This error is only present when the `walkdir` feature is enabled. + #[cfg(feature = "walkdir")] + #[error("Unable to list directory")] + Listing(#[source] Arc), } impl Error { @@ -95,6 +106,8 @@ impl Error { Error::StepsExceeded => return None, Error::CurrentDirectory(_) => return None, Error::CreatingDir(_) => return None, + Error::Content(e) => return e.path(), + Error::Listing(e) => return e.path(), } .as_path(), ) diff --git a/crates/fs-mistrust/src/imp.rs b/crates/fs-mistrust/src/imp.rs index 0a21bd474..72c6455e7 100644 --- a/crates/fs-mistrust/src/imp.rs +++ b/crates/fs-mistrust/src/imp.rs @@ -25,6 +25,11 @@ use crate::{ #[cfg(target_family = "unix")] pub(crate) const STICKY_BIT: u32 = 0o1000; +/// Helper: Box an iterator of errors. +fn boxed<'a, I: Iterator + 'a>(iter: I) -> Box + 'a> { + Box::new(iter) +} + impl<'a> super::Verifier<'a> { /// Return an iterator of all the security problems with `path`. /// @@ -37,13 +42,6 @@ impl<'a> super::Verifier<'a> { // to the code. It's not urgent, since the allocations won't cost much // compared to the filesystem access. pub(crate) fn check_errors(&self, path: &Path) -> impl Iterator + '_ { - /// Helper: Box an iterator. - fn boxed<'a, I: Iterator + 'a>( - iter: I, - ) -> Box + 'a> { - Box::new(iter) - } - let rp = match ResolvePath::new(path) { Ok(rp) => rp, Err(e) => return boxed(vec![e].into_iter()), @@ -69,6 +67,41 @@ impl<'a> super::Verifier<'a> { ) } + /// If check_contents is set, return an iterator over all the errors in + /// elements _contained in this directory_. + #[cfg(feature = "walkdir")] + pub(crate) fn check_content_errors(&self, path: &Path) -> impl Iterator + '_ { + use std::sync::Arc; + + if !self.check_contents { + return boxed(std::iter::empty()); + } + + boxed( + walkdir::WalkDir::new(path) + .follow_links(false) + .min_depth(1) + .into_iter() + .flat_map(move |ent| match ent { + Err(err) => vec![Error::Listing(Arc::new(err))], + Ok(ent) => match ent.metadata() { + Ok(meta) => self + .check_one(ent.path(), PathType::Content, &meta) + .into_iter() + .map(|e| Error::Content(Box::new(e))) + .collect(), + Err(err) => vec![Error::Listing(Arc::new(err))], + }, + }), + ) + } + + /// Return an empty iterator. + #[cfg(not(feature = "walkdir"))] + pub(crate) fn check_content_errors(&self, _path: &Path) -> impl Iterator + '_ { + std::iter::empty() + } + /// Check a single `path` for conformance with this `Concrete` mistrust. /// /// `position` is the position of the path within the ancestors of the @@ -78,20 +111,16 @@ impl<'a> super::Verifier<'a> { fn check_one(&self, path: &Path, path_type: PathType, meta: &Metadata) -> Vec { let mut errors = Vec::new(); - if path_type == PathType::Symlink { - // There's nothing to check on a symlink; its permissions and - // ownership do not actually matter. - // - // TODO: Make sure that is correct. - return errors; - } - - // Make sure that the object is of the right type (file vs directory). - let want_type = if path_type == PathType::Final { - self.enforce_type - } else { - // We make sure that everything at a higher level is a directory. - Type::Dir + let want_type = match path_type { + PathType::Symlink => { + // There's nothing to check on a symlink encountered _while + // looking up the target_; its permissions and ownership do not + // actually matter. + return errors; + } + PathType::Intermediate => Type::Dir, + PathType::Final => self.enforce_type, + PathType::Content => Type::DirOrFile, }; if !want_type.matches(meta.file_type()) { @@ -110,9 +139,11 @@ impl<'a> super::Verifier<'a> { if uid != 0 && Some(uid) != self.mistrust.trust_uid { errors.push(Error::BadOwner(path.into(), uid)); } - let mut forbidden_bits = if !self.readable_okay && path_type == PathType::Final { - // If this is the target object, and it must not be readable, - // then we forbid it to be group-rwx and all-rwx. + let mut forbidden_bits = if !self.readable_okay + && (path_type == PathType::Final || path_type == PathType::Content) + { + // If this is the target or a content object, and it must not be + // readable, then we forbid it to be group-rwx and all-rwx. 0o077 } else { // If this is the target object and it may be readable, or if diff --git a/crates/fs-mistrust/src/lib.rs b/crates/fs-mistrust/src/lib.rs index 5a0f2a081..d4d441e7e 100644 --- a/crates/fs-mistrust/src/lib.rs +++ b/crates/fs-mistrust/src/lib.rs @@ -27,7 +27,11 @@ //! can be modified by an untrusted user. If there is a modifiable symlink in //! the middle of the path, or at any stage of the path resolution, somebody //! who can modify that symlink can change which file the path points to. -//! * TODO: explain the complications that hard links create. +//! * Even if you have checked a directory as being writeable only by a trusted +//! user, that doesn't mean that the objects _in_ that directory are only +//! writeable by trusted users. Those objects might be symlinks to some other +//! (more writeable) place on the file system; or they might be accessible +//! with hard links stored elsewhere on the file system. //! //! Different programs try to solve this problem in different ways, often with //! very little rationale. This crate tries to give a reasonable implementation @@ -205,6 +209,10 @@ pub struct Verifier<'a> { /// If the user called [`Verifier::require_file`] or /// [`Verifier::require_directory`], which did they call? enforce_type: Type, + + /// If true, we want to check all the contents of this directory as well as + /// the directory itself. Requires the `walkdir` feature. + check_contents: bool, } /// A type of object that we have been told to require. @@ -283,6 +291,7 @@ impl Mistrust { readable_okay: false, collect_multiple_errors: false, enforce_type: Type::DirOrFile, + check_contents: false, } } @@ -372,6 +381,20 @@ impl<'a> Verifier<'a> { self } + /// Configure this verifier so that, after checking the directory, check all + /// of its contents. + /// + /// Symlinks are not permitted; both files and directories are allowed. This + /// option implies `require_directory()`, since only a directory can have + /// contents. + /// + /// Requires that the `walkdir` feature is enabled. + #[cfg(feature = "walkdir")] + pub fn check_content(mut self) -> Self { + self.check_contents = true; + self.require_directory() + } + /// Check whether the file or directory at `path` conforms to the /// requirements of this `Verifier` and the [`Mistrust`] that created it. pub fn check>(self, path: P) -> Result<()> { @@ -380,7 +403,9 @@ impl<'a> Verifier<'a> { // This is the powerhouse of our verifier code: // // See the `imp` module for actual implementation logic. - let mut error_iterator = self.check_errors(path.as_ref()); + let mut error_iterator = self + .check_errors(path.as_ref()) + .chain(self.check_content_errors(path.as_ref())); // Collect either the first error, or all errors. let opt_error: Option = if self.collect_multiple_errors { @@ -392,9 +417,11 @@ impl<'a> Verifier<'a> { }; match opt_error { - Some(err) => Err(err), - None => Ok(()), + Some(err) => return Err(err), + None => {} } + + Ok(()) } /// Check whether `path` is a valid directory, and create it if it doesn't @@ -670,6 +697,33 @@ mod test { m.make_directory(d.path("a/b/c/d")).unwrap(); } + #[test] + fn check_contents() { + let d = Dir::new(); + d.dir("a/b/c"); + d.file("a/b/c/d"); + d.chmod("a", 0o700); + d.chmod("a/b", 0o700); + d.chmod("a/b/c", 0o755); + d.chmod("a/b/c/d", 0o644); + + let mut m = Mistrust::new(); + m.ignore_prefix(d.canonical_root()).unwrap(); + + // A check should work... + m.check_directory(d.path("a/b")).unwrap(); + + // But we get errors if we check the contents. + let e = m + .verifier() + .all_errors() + .check_content() + .check(d.path("a/b")) + .unwrap_err(); + + assert_eq!(2, e.errors().count()); + } + // TODO: Write far more tests. // * Can there be a test for a failed readlink()? I can't see an easy way // to provoke that without trying to make a time-of-check/time-of-use race diff --git a/crates/fs-mistrust/src/walk.rs b/crates/fs-mistrust/src/walk.rs index 5e339e370..6b89212f9 100644 --- a/crates/fs-mistrust/src/walk.rs +++ b/crates/fs-mistrust/src/walk.rs @@ -11,7 +11,7 @@ use std::{ sync::Arc, }; -/// The type of a single path from `ResolvePath`. +/// The type of a single path inspected by [`Verifier`](crate::Verifier). #[derive(Debug, Copy, Clone, Eq, PartialEq)] #[allow(clippy::exhaustive_enums)] pub(crate) enum PathType { @@ -22,6 +22,8 @@ pub(crate) enum PathType { Intermediate, /// This is a symbolic link. Symlink, + /// This is a file _inside_ the target directory. + Content, } /// An iterator to resolve and canonicalize a filename, imitating the actual