Bug 8411

CVE-2011-5325 was assigned to this issue:

  http://openwall.com/lists/oss-security/2015/10/21/7

Created attachment 6191 [details]
Patch for busybox 1.22.0

Created attachment 6196 [details]
Patch for busybox 1.22.0 v2

Updated patch attached - I didn't spot I had removed a TODO comment; apologies.

Created attachment 6201 [details]
Patch for busybox 1.22.0 v3

Matching code style.

(In reply to comment #4)
> Created attachment 6201 [details]
> Patch for busybox 1.22.0 v3
> 
> Matching code style.

Hello Chris - the dot dot check is incorrect. A file can still be extracted into the parent directory:

$ ln -s .. foo
$ tar -cf dotdot.tar foo
$ rm foo
$ mkdir foo
$ touch foo/bar
$ tar -rf dotdot.tar foo/bar
$ rm -rf foo
$ stat -t ../bar
stat: cannot stat ‘../bar’: No such file or directory
$ busybox tar -xvf dotdot.tar
foo
foo/bar
$ stat -t ../bar
../bar 0 0 81b4 1000 1000 fc00 23736439 1 0 0 1446875494 1446875494 1446876258 0 4096

Created attachment 6206 [details]
Patch for busybox 1.22.0 v4

Oh, good catch.

Instead of matching ".." anywhere ("..foo" is totally valid after all!), I'm also now just matching on a literal ".."

(In reply to comment #6)
> Created attachment 6206 [details]
> Patch for busybox 1.22.0 v4
> 
> Oh, good catch.
> 
> Instead of matching ".." anywhere ("..foo" is totally valid after all!), I'm
> also now just matching on a literal ".."

Something odd happened with the patch that you attached. It is base64 encoded and, while it does contain some changes, does not contain the change to match a lateral ".." sequence.

Created attachment 6211 [details]
Patch for busybox 1.22.0 v5

That's.. odd. Let's try again.

(In reply to comment #6)
> Created attachment 6206 [details]
> Patch for busybox 1.22.0 v4
> 
> Oh, good catch.
> 
> Instead of matching ".." anywhere ("..foo" is totally valid after all!), I'm
> also now just matching on a literal ".."

Good point regarding "..foo". I guess that means that strstr(file_header->link_target, "../") will unintentionally match a target of "foo../bar".

> Good point regarding "..foo". I guess that means that
> strstr(file_header->link_target, "../") will unintentionally match a target of
> "foo../bar".

Doh. Although an aside, inadventent matching is "harmless" as it will end up as symlink just as intended.. but via the placeholder mechanism first. :)

Will update patch tomorrow (UTC+0) with a fresh head.

(In reply to comment #0)
> I took a quick look at how GNU tar handles such situations. If the symlink
> target is absolute or contains a ".." component, they create a regular file as
> a placeholder. After all other files have been extracted, the placeholder files
> are replaced with the originally intended symlinks.
> 
> (That is also how they handle hardlink extraction but I don't see any support
> for LNKTYPE files in busybox tar.)

I was wrong about hardlinks. They're supported in busybox's libarchive and
they're also vulnerable. From archival/libarchive/data_extract_all.c:

        /* Handle hard links separately
         * We encode hard links as regular files of size 0 with a symlink */

Created attachment 6216 [details]
Tar file containing two files (abs and rel) encoded as hardlinks of /tmp/foo

Here's a tar file that includes two files, abs and rel, that are encoded in such a way to match busybox libarchive's encoding of hardlinks (which seems to differ from what GNU tar uses).

Busybox tar will extract the two files and create them as hardlinks of /tmp/foo.

$ rm -f /tmp/foo
$ touch /tmp/foo
$ stat -c %h /tmp/foo
1
$ busybox tar -xvf hardlink.tar
abs
rel
$ stat -c %h /tmp/foo # should print "1"
3

Right, let's get this finished. What's the desired behaviour in this hardlink case? Bail out with an error?

(In reply to Chris Lamb (lamby) from comment #10)
> Patch for busybox 1.22.0 v5
There is an missing closing brackets in if condition.
>if ((!strncmp(file_header->link_target, "/", 1))
> || ((!strcmp(file_header->link_target, ".."))
> || (strstr(file_header->link_target, "../"))) {
should be
if ((!strncmp(file_header->link_target, "/", 1))
 || ((!strcmp(file_header->link_target, ".."))
 || (strstr(file_header->link_target, "../")))) {

Changes have not been included in release yet.
Last change of the patch was on "2015-11-09 23:29 UTC" and currently the latest release is "10 January 2017 -- BusyBox 1.26.2 (stable)". 

My question is, if the changes are not so necessary to be upstreamed?

Thanks for detailing this bug. I can now confirm that this has been exploited "in the wild" to root / jailbreak DJI Mavic, Spark, Inspire2, and Phantom 4 drone series. 

Exploit here for posterity
https://github.com/MAVProxyUser/P0VsRedHerring/blob/master/RedHerring.rb#L24

Thanks for pushing to get this patched. It has festered for a while.

Applied patch to git:

commit b920a38dc0a87f5884444d4731a8b887b5e16018
Author: Denys Vlasenko <vda.linux@googlemail.com>
Date:   Mon Jul 24 17:20:13 2017 +0200

    tar: postpone creation of symlinks with "suspicious" targets. Closes 8411

However, I'm not sure this is sufficient to stop the attacks, if the target system is careless enough to untar untrusted tarballs. What if attacker supplies two tarballs, one creates a symlink to "/etc", and another has a lone "symlink/passwd" file? Both tarballs are not "malicious" individually. tar program can't know that unpacking second tar is doing a wrong thing!

What else needs to be done?

Fixed in git:

commit bc9bbeb2b81001e8731cd2ae501c8fccc8d87cc7
Author: Denys Vlasenko <vda.linux@googlemail.com>
Date:   Thu Aug 10 11:52:42 2017 +0200

    libarchive: do not extract unsafe symlinks unless $EXTRACT_UNSAFE_SYMLINKS=1

And reverted to the "old" solution:

commit a84db18fc71d09e801df0ebca048d82e90b32c6a
Author: Denys Vlasenko <vda.linux@googlemail.com>
Date:   Tue Feb 20 15:57:45 2018 +0100

    tar,unzip: postpone creation of symlinks with "suspicious" targets

    This mostly reverts commit bc9bbeb2b81001e8731cd2ae501c8fccc8d87cc7
    "libarchive: do not extract unsafe symlinks unless $EXTRACT_UNSAFE_SYMLINKS=1"

    Users report that it is somewhat too restrictive. See
    https://bugs.busybox.net/show_bug.cgi?id=8411

    In particular, this interferes with unpacking of busybox-based
    filesystems with links like "sbin/applet" -> "../bin/busybox".

    The change is made smaller by deleting ARCHIVE_EXTRACT_QUIET flag -
    it is unused since 2010, and removing conditionals on it
    allows commonalizing some error message codes.