Fix rm rf v2

[jsynacek]

No description provided.

[martinpitt]

As discussed on IRC, I'm in favor of using path_is_safe() everywhere for consistency, and relaxing it a bit. In particular, double slashes (/foo//bar) commonly happen when unit files are autogenerated by scripts (think of path="$base/$resource" where $base itself has a trailing '/'), and these are perfectly safe; same argument for /./. I don't see why we should have a different behaviour for tmpfiles than for e. g. RequiresMountsFor= and other places where we use path_is_safe().

So in general I agree with this, but I ask you to split this into two commits. Thanks!

[jsynacek]

Ok, waiting for Zbyszek before splitting the commit.

[martinpitt]

@jsynacek semaphore fails on FAIL: test-unit-name, that should reproduce locally with make check.

[jsynacek]

v2.

[martinpitt]

Thanks for splitting! LGTM now, but still would like to get a second opinion from @keszybz or @poettering .

[keszybz]

I think @martinpitt's point about autogenerated paths is very valid. We shouldn't disallow things unless there's a clear reason. So I think the restrictions which are left after patch 1/2 are OK.

It certainly doesn't help it isn't defined anywhere what path_is_safe means and for what it is "safe". It seems that every call point makes some implicit assumptions... Going over the callers, allowing // and /./ seems safe. Some callers call path_kill_slashes anyway before calling path_is_safe, which makes the check for // moot. Others check that path_is_absolute, so the check for ./ is moot.

2/2 looks OK too.

[evverx]

Does anybody know what precisely path_is_safe checks? Previously, the condition path_is_safe == true guaranteed that path is normalised. What does it mean now? I still can remove everything with /etc/..////.

[keszybz]

Well, /../ is still forbidden, so you can't.

See my comment — I don't think anyone knows what it's supposed to mean exactly. Do any of the callers actually care if the path is normalized?

[evverx]

Well, /../ is still forbidden, so you can't.

Right. Sorry.

Do any of the callers actually care if the path is normalized?

Hm, I don't know. If nothing cares why do we need to call path_is_safe at all?

[evverx]

I've just updated test-path-util.c

diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c
index 22df20a..ad30794 100644
--- a/src/test/test-path-util.c
+++ b/src/test/test-path-util.c
@@ -38,6 +38,12 @@
                 assert_se(path_equal(b, a) == !result);   \
         }

+static void test_path_is_safe(void) {
+        assert_se(path_is_safe("///") == true);
+        assert_se(path_is_safe("/etc/..////") == false);
+        assert_se(path_is_safe("/etc/.////") == true);
+}
+
 static void test_path(void) {
         _cleanup_close_ int fd = -1;

@@ -563,6 +569,7 @@ int main(int argc, char **argv) {
         test_file_in_same_dir();
         test_filename_is_valid();
         test_hidden_or_backup_file();
+        test_path_is_safe();

         test_systemd_installation_has_version(argv[1]); /* NULL is OK */

It looks strange. Sorry, but I think path_is_safe doesn't make sense now. I don't understand why /etc/./// is safer than /etc/..///.

[keszybz]

The way I understand it, repeated slashes and /./ as a component are noise, but harmless. But /../ allows one to go "outside", and is a problem.

[evverx]

repeated slashes and /./ as a component are noise, but harmless.

It depends on a caller. rm_rf shouldn't allow ///. This is why rm_rf calls path_equal(path, "/").

But /../ allows one to go "outside", and is a problem.

Well, it depends on a caller too. Going outside is not always a problem.

[evverx]

Hm, rm_rf shouldn't allow ///./// either. But

+        assert_se(path_is_safe("///.//") == true);
+        assert_se(path_equal("///.//", "/") == false);

passes.

I've already asked #5653 (comment), but why don't we try to normalise path?

[evverx]

is_path_safe is not sufficient because rm_rf("//.//", REMOVE_ROOT|REMOVE_PHYSICAL); destroys everything. I think we shouldn't relax path_is_safe. Instead, we should normalise path somewhere in the code and path_is_safe should check that path is normalised (or, maybe, path_is_safe should normalise path by itself).

[martinpitt]

I don't understand why /// or //.// should not be allowed -- these are just aliases for the root file system, and if you rm those or / directly makes no difference -- relying on some obscure pattern heuristic is not the point here, if you try and remove the root file system you just get what you are asking for..

[evverx]

if you try and remove the root file system you just get what you are asking for

@martinpitt , sorry, I am a bit confused. What is the point of this PR and #5653?

[martinpitt]

The point, as far as I understood it, was to avoid path traversal with an implied ..; particularly this happened with something like /etc/.* which looks innocuous at first sight (as it just looks like "remove all hidden files"), but is disastrous as .* also expands to ...

[poettering]

uh? why this change? as the comment says, we check here not only for things that are actively dangerous, but also confusing. I am not seeing why "/./" would ever be a good thing to permit here...

[poettering]

I really don't understand why relaxing path_is_safe() would be a good idea. As the comments in the function clarify, we check for "confusing" paths here too. The idea really is that we consider anything "unsafe" that is not a properly build, normalized path. Or to say this differently: it's supposed to detect paths that are the result of "dirty" path concatenations...

[martinpitt]

I'm mostly concerned about backwards compatibility with existing tmpfiles.d snippets here. As explained above double slashes are a frequent result of producing paths in code robustly, and I see no justification why these should be not allowed -- there is no case where they would be unsafe.

[poettering]

I'm mostly concerned about backwards compatibility with existing tmpfiles.d snippets here. As explained above double slashes are a frequent result of producing paths in code robustly, and I see no justification why these should be not allowed -- there is no case where they would be unsafe.

I can agree to that, but my suggestion would be to first call path_kill_slashes() on all parsed strings to remove duplicate slashes.

[martinpitt]

Dropping the first patch (changing path_is_safe() and instead changing the second to call path_kill_slashes() first sounds good to me too.

However, long-term I still wonder why path_is_safe() should be so picky about it if we basically always call path_kill_slashes() beforehand anyway..

[boucman]

maybe rename path_is_safe to something like path_is_readable ? since that seems to be the real point of that function

[zackw]

if you try and remove the root file system you just get what you are asking for.

Note that rm (the utility) will refuse to remove the root file system. See https://github.com/coreutils/coreutils/blob/e7a2580b96370da03c4d3553ccdf4ee66a14c6a4/src/remove.c#L453 Commentary there implies that this is a POSIX requirement.

[Rudd-O]

It seems to me that the original bug:

 R! /foo/.* - - - - -

is that whatever glob function is used to expand /foo/.* is incorrectly matching on the dot dot directory entry, when it should not be doing that in the first place.

Agree that this particular PR does help, but the real bug has not been fixed.

[Rudd-O]

FWIW

[user@engineering ansible]$ python
Python 2.7.13 (default, Jan 12 2017, 17:59:37) 
[GCC 6.3.1 20161221 (Red Hat 6.3.1-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import glob
>>> glob.glob(".*")
[]
>>> glob.glob("/tmp/.*")
['/tmp/.esd-1000', '/tmp/.X0-lock', '/tmp/.Test-unix', '/tmp/.font-unix', '/tmp/.XIM-unix', '/tmp/.ICE-unix', '/tmp/.X11-unix']
>>> 

[Saruspete]

Agreed with @Rudd-O , the only bug to be fixed is in the glob & recursion. You only have to skip the "." and ".." entries.

Do not add restrictions on valid use-cases, like "//" or "/abs/path/to/bin/../var/tmp".
the "./" can also be a valid case for entries generated with tools like "find".

[keszybz]

Yeah, the approach of filter out . and .. from any glob seems like the right thing to do. Literal paths with . and .. should be accepted.

We should also refuse to remove / (under whatever name), just like rm does. There is no circumstance where this would be useful.

(I checked "×", not because I think there's anything wrong with the code, but because a different approach seems warranted.)

[Rudd-O]

Correct. The initial fix that went in, is incorrect, and this fix, is also superstitious. The right fix is to (a) comply with POSIX rm, (b) prevent globbing from snarfing dot and dot dot, as it ought to be in this scenario. Even Python glob gets it right. We can get it right too.

…

On April 20, 2017 5:53:11 PM GMT+02:00, Adrien Mahieux ***@***.***> wrote: Agreed with @Rudd-O , the only bug to be fixed is in the glob & recursion. You only have to skip the "." and ".." entries. Do not add restrictions on valid use-cases, like "//" or "/abs/path/to/bin/../var/tmp". the "./" can also be a valid case for entries generated with tools like "find". -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #5655 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[pmaddams]

Even Python glob gets it right

LOL

From OpenBSD libc glob.c:

 * Copyright (c) 1989, 1993
 *	The Regents of the University of California.  All rights reserved.
 *
 * This code is derived from software contributed to Berkeley by
 * Guido van Rossum.

[Rudd-O]

And now we know why :-D

…

On April 23, 2017 5:48:11 PM GMT+02:00, Pavan Maddamsetti ***@***.***> wrote: > Even Python glob gets it right LOL From OpenBSD libc glob.c: ``` * Copyright (c) 1989, 1993 * The Regents of the University of California. All rights reserved. * * This code is derived from software contributed to Berkeley by * Guido van Rossum. ``` -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #5655 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[poettering]

@Rudd-O well, we use glibc glob(), which will expand to ".." if needed, and so will bash, as you can easily see:

echo ./.*
./. ./.. ./.deps ./.dirstamp ./.libs

Moreover, coreutils "rm" command line tool doesn't actually do any globbing, it's bash which does that and just passes the resolved strins to "rm". It's "rm" then which filters stuff ending in some specific suffixes, regardless whether those suffixes where the result of globbing or not.

I must say I find the "rm" logic pretty weird, as it checks for suffixes only, but globbing can be anywhere in the path...

That all said, I think it would be wise for us, if we'd add a wrapper for glibc glob() that filters out "." and ".." from any expansion, and that we'd use in all our own cases. We can do this relatively nicely through the GLOB_ALTDIRFUNC logic.

[poettering]

I posted #5792 about that now, to keep track.

[Rudd-O]

@poettering if you specify a dot star after a slash, yes, then glob() will match hidden dirs. But you have to specify the dot before the slash. The original bug, according to the reproducer posted there, was about ...../* matching dot dot, which is not correct, not even by glibc glob().

EDIT: no, Lennart is right and I am wrong, I misremembered the reproducer. /tmp/.* is a legit glob expression that will match dot dot.