Commit 5c33499f authored by Nick Mathewson's avatar Nick Mathewson 🦞
Browse files

Loosen checking for readable files within target directories. 

If the target directory itself is unreadable by untrusted users,
then its contents can't be read[*] by them regardless of their
permissions.  If the target directory _is_ readable, then _it_ will
be rejected if we are forbidding readable objects.  (And if we
aren't we don't care if the contents are readable.)

A similar argument would apply to writable objects within an
unreadable target directory.  We're not making that argument, since
such contents are likelier to be a mistake.

[*] Unless they're hard-linked; see comments in "Limitations"
section.
parent dcc5a3cf

crates/fs-mistrust/src/imp.rs

+13 −10
Original line number Diff line number Diff line
@@ -182,18 +182,21 @@ 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 || 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.

        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.

            //

            // (We allow _content_ to be globally readable even if readable_okay

            // is false, since we check that the Final directory is itself

            // unreadable.  This is okay unless the content has hard links: see

            // the Limitations section of the crate-level documentation.)

            0o077

        } else {

            // If this is the target object and it may be readable, or if

            // this is _any parent directory_, then we typically forbid the

            // group-write and all-write bits.  (Those are the bits that

            // would allow non-trusted users to change the object, or change

            // things around in a directory.)

            // If this is the target object and it may be readable, or if this

            // is _any parent directory_ or any content, then we typically

            // forbid the group-write and all-write bits.  (Those are the bits

            // that would allow non-trusted users to change the object, or

            // change things around in a directory.)

            if meta.is_dir() && meta.mode() & STICKY_BIT != 0 && path_type == PathType::Intermediate

            {

                // This is an intermediate directory and this sticky bit is

crates/fs-mistrust/src/lib.rs

+17 −4
Original line number Diff line number Diff line
@@ -200,6 +200,17 @@ 
//!    * SELinux capabilities

//!    * POSIX (and other) ACLs.

//!

//! We use a somewhat inaccurate heuristic when we're checking the permissions

//! of items _inside_ a target directory (using [`Verifier::check_content`] or

//! [`CheckedDir`]): we continue to forbid untrusted-writeable directories and

//! files, but we still allow readable ones, even if we insisted that the target

//! directory itself was required to to be unreadable.  This is too permissive

//! in the case of readable objects with hard links: if there is a hard link to

//! the file somewhere else, then an untrusted user can read it.  It is also too

//! restrictive in the case of writeable objects _without_ hard links: if

//! untrusted users have no path to those objects, they can't actually write

//! them.

//!

//! On Windows, we accept all file permissions and owners.

//!

//! We don't check for mount-points and the privacy of filesystem devices
@@ -232,7 +243,6 @@ 


// POSSIBLY TODO:

//  - Cache information across runs.

//  - Add a way to recursively check the contents of a directory.



#![deny(missing_docs)]

#![warn(noop_method_call)]
@@ -907,7 +917,7 @@ mod test { 
        d.chmod("a", 0o700);

        d.chmod("a/b", 0o700);

        d.chmod("a/b/c", 0o755);

        d.chmod("a/b/c/d", 0o644);

        d.chmod("a/b/c/d", 0o666);



        let mut m = Mistrust::new();

        m.ignore_prefix(d.canonical_root()).unwrap();
@@ -915,15 +925,18 @@ mod test { 
        // A check should work...

        m.check_directory(d.path("a/b")).unwrap();



        // But we get errors if we check the contents.

        // But we get an error if we check the contents.

        let e = m

            .verifier()

            .all_errors()

            .check_content()

            .check(d.path("a/b"))

            .unwrap_err();

        assert_eq!(1, e.errors().count());



        assert_eq!(2, e.errors().count());

        // We only expect an error on the _writable_ contents: the _readable_

        // a/b/c is okay.

        assert_eq!(e.path().unwrap(), d.path("a/b/c/d"));

    }



    #[test]