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

chown+chgrp+chmod: Fix handling of preserve root flag and error messages #6042

Merged
merged 3 commits into from
Mar 9, 2024

Conversation

BenWiederhake
Copy link
Collaborator

This makes the GNU preserve-root.log test green.

In particular:

  • We used to have the wrong logic to detect whether or not to issue a warning. The root (punt not intended) cause was that Path::is_dir() completely ignores whether a given argument has a trailing slash or not, but that is the criterion that GNU (and many other tools) apparently uses to decide whether the path points "at" or "into" a directory. (See below for demonstration.)
  • chown and chgrp also didn't correctly output an indication why the warning was being given
  • And finally, chmod simply forgot to output its own name in one place.

Additional notes:

  • This makes the uucore function resolve_relative_path superfluous. This function is by necessity ill-defined: Depending on the context, .. is either the logical parent directory, sometimes the physical parent directory. This function can only work for the latter case, in which case Path::canonicalize is often a better approach.
  • While rewriting, I noticed and documented an unavoidable TOCTOU bug. (If root runs chown -R -L and the attacker can quickly inject a directory and replace it with a symlink to /, then --preserve-root does not trigger. However, in this scenario the attacker could probably just as well create a handful of symlinks, one for each directory in /, for the same effect. So this is not a vulnerability, just an unavoidable bug.)
Path::is_dir demo
fn main() {
    for name in "/tmp /tmp/ /xxx /xxx/".split(" ") {
        let buf: &std::path::Path = name.as_ref();
        println!("{:?}.is_dir() = {}", buf.as_os_str(), buf.is_dir());
    }
}

produces

"/tmp".is_dir() = true
"/tmp/".is_dir() = true
"/xxx".is_dir() = false
"/xxx/".is_dir() = false

Copy link

github-actions bot commented Mar 3, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/chown/preserve-root is no longer failing!
Congrats! The gnu test tests/mv/backup-dir is no longer failing!

@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Uglified the code away from OsStr::ends_with(), because that is Rust 1.74.0, and MSRV is Rust 1.70.0.
  • I don't understand why mv/backup-dir is doing better now. I touched none of these tools O.o

Copy link

github-actions bot commented Mar 3, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/chown/preserve-root is no longer failing!
Congrats! The gnu test tests/mv/backup-dir is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, please check that I understand correctly. Here's what I got from this:

We need to know whether symlinks will be dereferenced on the path to know how to check for root. Dereferencing happens if either the setting for dereferencing is passed or if the / is at the end, so that we need to enter the directory and to do that need to dereference.

Does that make sense? If so, could you add that last part in a comment somewhere. It took me a while to understand :)

I also wonder if these path checks are the right way to do it. Maybe there's some other way to check this? It all feels inherently finnicky.

src/uu/chmod/src/chmod.rs Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Outdated Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Outdated Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Outdated Show resolved Hide resolved
src/uucore/src/lib/features/perms.rs Show resolved Hide resolved
One of the GNU tests checks for the exact error message.
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • rebased on current main
  • Converted the big introduction in fn is_root to a docstring
  • In it, mentioned that this is specific to chown and chgrp
  • Simplified the to_str/match/print handling in case the check triggers
  • Renamed the argument to would_traverse_symlink. All as suggested by @tertsdiepraam

@BenWiederhake
Copy link
Collaborator Author

Nope, wait, I got something wrong with the quoting.

…message

This is explicitly tested in the GNU tests.
This function is by necessity ill-defined: Depending on the context,
'..' is either the logical parent directory, sometimes the physical
parent directory. This function can only work for the latter case,
in which case `Path::canonicalize` is often a better approach.
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Fixed the double-quoting issue (I used "blah '{}' blah", pathbuf.quote(), which results in the quotes appearing twice. Doink!)

Copy link

github-actions bot commented Mar 7, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/chown/preserve-root is no longer failing!

@sylvestre
Copy link
Contributor

well done :)

@sylvestre sylvestre merged commit 6c29ed0 into uutils:main Mar 9, 2024
62 checks passed
@BenWiederhake BenWiederhake deleted the dev-chown-preserve-root branch March 11, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants