[Bug-tar] possible fixes for CVE-2016-6321

[Antoine Beaupré]

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:

address@hidden: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
address@hidden: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