Add functionality to inspect directory content permissions

Also, explain _why_ this is pretty important.

Cargo.lock

@@ -1197,6 +1197,7 @@ dependencies = [
  "libc",
  "tempfile",
  "thiserror",
+ "walkdir",
 ]
 
 [[package]]

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"

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<IoError>),
+
+    /// We found a problem while checking the contents of the directory.
+    #[error("Invalid directory content")]
+    Content(#[source] Box<Error>),
+
+    /// 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<walkdir::Error>),
 }
 
 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(),
         )

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<Item = Error> + 'a>(iter: I) -> Box<dyn Iterator<Item = Error> + '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<Item = Error> + '_ {
-        /// Helper: Box an iterator.
-        fn boxed<'a, I: Iterator<Item = Error> + 'a>(
-            iter: I,
-        ) -> Box<dyn Iterator<Item = Error> + '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<Item = Error> + '_ {
+        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<Item = Error> + '_ {
+        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<Error> {
         let mut errors = Vec::new();
 
-        if path_type == PathType::Symlink {
+        let want_type = match path_type {
-            // There's nothing to check on a symlink; its permissions and
+            PathType::Symlink => {
-            // ownership do not actually matter.
+                // There's nothing to check on a symlink encountered _while
-            //
+                // looking up the target_; its permissions and ownership do not
-            // TODO: Make sure that is correct.
+                // actually matter.
                 return errors;
             }
-
+            PathType::Intermediate => Type::Dir,
-        // Make sure that the object is of the right type (file vs directory).
+            PathType::Final => self.enforce_type,
-        let want_type = if path_type == PathType::Final {
+            PathType::Content => Type::DirOrFile,
-            self.enforce_type
-        } else {
-            // We make sure that everything at a higher level is a directory.
-            Type::Dir
         };
 
         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 {
+            let mut forbidden_bits = if !self.readable_okay
-                // If this is the target object, and it must not be readable,
+                && (path_type == PathType::Final || path_type == PathType::Content)
-                // then we forbid it to be group-rwx and all-rwx.
+            {
+                // 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

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<P: AsRef<Path>>(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<Error> = if self.collect_multiple_errors {
@@ -392,9 +417,11 @@ impl<'a> Verifier<'a> {
         };
 
         match opt_error {
-            Some(err) => Err(err),
+            Some(err) => return Err(err),
-            None => Ok(()),
+            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

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