Hi all, (Debian maintainers, Debian security teams and upstream bug mailing list in CC.)
I have added notes regarding the "/../" mismatch security issue in the security tracker here: https://security-tracker.debian.org/tracker/CVE-2016-6321 Basically, there's a proof of concept here: https://sintonen.fi/advisories/tar-poc.tar And a patch here: https://sintonen.fi/advisories/tar-extract-pathname-bypass.patch The issue has been resolved only on Gentoo so far and there is some controversy with upstream regarding whether this is a real vulnerability. From my point of view, it's a significant risk that is well outlined in the advisory, as there is a mismatch error (you wouldn't expect the etc/motd pattern to extract etc/passwd). But I understand why there could be a discussion about this. After reviewing the above patch myself, I also wonder if it is the right solution: it simply removes checks for /../ in paths and replaces that with a "deferred error" which translates to the familiar "Exiting with failure status due to previous errors" when tar exits. But the malicious paths are still extracted. Instead of giving the tree: . ./etc ./etc/shadow ...the patched version gives the following tree: . ./etc ./etc/motd ./etc/etc ./etc/etc/shadow That is hardly satisfactory: a tar file with "etc/motd/../../etc/shadow" would still overwrite the "./etc/shadow" file, I believe. I strongly recommend *not* applying the patch given in the original advisory as is: it just completely works around "/../" detection, and upstream added that for a good reason. I am not sure what the correct solution is. It seems to me this is a typical input mismatch problem: you ask for "etc/motd" and get "etc/shadow". ideally paths would be resolved before being matched, which would work around the issue when asking for a specific path, but not when extracting a whole archive... The advisory suggests entries with "/../" could just be skipped. There is no direct way to do this in the patched function: there is a mechanism to process empty strings ('return "."')... but then this could be abused to change the mode on "." when extracting. Yet it is semantically pretty weird: this is designed for empty filenames, not for skipping really. Otherwise, we could just completely crash with a FATAL_EROR instead of an ERROR: --- a/lib/paxnames.c +++ b/lib/paxnames.c @@ -18,6 +18,7 @@ #include <system.h> #include <hash.h> #include <paxlib.h> +#include <quotearg.h> /* Hash tables of strings. */ @@ -114,7 +115,15 @@ safer_name_suffix (char const *file_name for (p = file_name + prefix_len; *p; ) { if (p[0] == '.' && p[1] == '.' && (ISSLASH (p[2]) || !p[2])) - prefix_len = p + 2 - file_name; + { + static char const *const diagnostic[] = + { + N_("%s: Member name contains '..'"), + N_("%s: Hard link target contains '..'") + }; + FATAL_ERROR ((0, 0, _(diagnostic[link_target]), + quotearg_colon (file_name))); + } do { I would love to get more feedback on this patch. With the above, the POC yields: [1093]anarcat@angela:t$ ../debian/tar/bin/tar vfx ~/Downloads/tar-poc.tar etc/motd ../debian/tar/bin/tar: etc/motd/../etc/shadow: Member name contains '..' ../debian/tar/bin/tar: Error is not recoverable: exiting now [1094]anarcat@angela:t2$ find . Not so great, but not vulnerable anymore. :) Note that star skips such files "since 2003": http://lists.gnu.org/archive/html/bug-tar/2016-10/msg00013.html I couldn't figure out how to easily skip archive members reliably in tar, unfortunately. I found about skip_member(), but I'm not sure where to throw it in: it seems we could throw a loop in extract_archive(), but there aren't any safety checks in there so far: everything seems to be done by rewriting the path before extraction in tar, as far as I can see, so I am hesitant in trying to change that. A. -- The greatest crimes in the world are not committed by people breaking the rules but by people following the rules. It's people who follow orders that drop bombs and massacre villages. - Bansky
