Bug 10651

Set EXTRACT_UNSAFE_SYMLINKS=1 for the tar command which extracts something with potentially unsafe symlinks.
Act of doing so serves as an explicit statement from the user: "Yes, I know what I'd doing. This tarball is trusted by me".

(In reply to Denys Vlasenko from comment #1)

Yes, I'm aware of that, and it's what I'm doing now. I see that as a workaround, not as a fix though: I do actually like the idea of having busybox block attempts to extract outside of the -C directory. It's just that the current check is too strict to be usable for me.

Ouch, this behavior change started bricking embedded systems I work with.  Similar to Harald, untarring a new root filesystem containing Busybox onto a target device now fails to unpack all the /sbin/ symlinks (for example the missing "pivot_root -> ../bin/busybox" link was the one that blew up my systems).

At this point it is simpler and more robust for me to switch to gnu tar rather than maintain EXTRACT_UNSAFE_SYMLINKS=1 in all the right places.

(In reply to Daniel Goertzen from comment #3)
> At this point it is simpler and more robust for me to switch to gnu tar rather than maintain EXTRACT_UNSAFE_SYMLINKS=1 in all the right places.

Well then, the following will be happening:
> MavProxyUser 2017-07-06 17:30:44 UTC
> 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.

It is inescapable fact of life that more secure systems are "less convenient", they would require more steps.

It is a balancing act somewhere between having (in this case) tar which will untar anything anywhere, and tar which requires some awful crypto-signed tarballs (this will be very safe, but basically unusable).

I am not a security nutjob. I _am_ trying to find a balance, not just bolt everything down to the point where it's securely useless.


With EXTRACT_UNSAFE_SYMLINKS=1 thing, I defend even against the case where attacker provides two tarballs, with 1st creating evil symlink, and another using it. GNU tar's solution of delayed symlink creation is defeated by that. 
 *and*
unlike a solution of requiring a special option, "tar --extract-unsafe-symlinks", this one does not wreak havoc for all your scripts with tar invocations: envvars are _inherited_. You can just set it in your login environment once, and bingo: all tar invocations will start acting as you want. 

Propose something better, or how to tweak this solution to be more usable.

Specifically, what we have here are symlinks in /sbin to /bin/busybox. Hilariously ad-hoc, but working for us busyboxers solution would be to allow creation of symlinks which end with "bin/busybox" or "/[s]bin/busybox" ;)

Will it help if EXTRACT_UNSAFE_SYMLINKS can be configurable at build time? You can build yourself a busybox binary which does not make this check, just by setting one CONFIG_ option, not with patching sources.

(In reply to Denys Vlasenko from comment #4)
> Propose something better, or how to tweak this solution to be more usable.

I did have two concrete proposals for something better in my initial comment:

1: Keep the current check, but modify it to allow all symlinks to files to be extracted.

OR:

2: Remove the current check, allow all symlinks to be extracted, but add a check that requires all path components during extraction to be real directories, not symlinks.

Option 1 should be trivial to implement, no less safe than the current implementation, and sufficient for the use cases mentioned in this report.

Option 2 is difficult to implement, but potentially safer since it also protects against symlinks created without the use of tar. An added benefit is that it would never error out when simply creating an archive from directory A and extracting it into directory B, regardless of what symlinks A might contain, so it would have fewer false positives.

(In reply to Denys Vlasenko from comment #5)
> Will it help if EXTRACT_UNSAFE_SYMLINKS can be configurable at build time?

Perhaps, but like I wrote earlier, I do like the idea of this check, so I would prefer to tweak the logic so that it works for more cases.

Bug 10651 - tar: check for unsafe symlinks is overly strict
https://bugs.busybox.net/show_bug.cgi?id=10651

| (In reply to Harald van Dijk from comment #6)
| 1: Keep the current check, but modify it to allow all symlinks to files to be extracted.

(1) What if symlink comes first in the tarball?

(2) Is it ok to extract symlink to e.g. "/etc/passwd"? For tar, it _should be ok_ since newly extracted files unlink target filename, then open it with O_CREAT|O_EXCL, thus /etc/passwd can't be overwritten thru this symlink - but it is still feels iffy. Basically, _tar_ will not compromise security - but it may unknowingly create a setup for subsequent writes to the "file" to break it.

| OR:
|
| 2: Remove the current check, allow all symlinks to be extracted, but add a check that requires all path components during extraction to be real directories, not symlinks.
| Option 2 is difficult to implement, but potentially safer since it also protects against symlinks created without the use of tar. An added benefit is that it would never error out when simply creating an archive from directory A and extracting it into directory B, regardless of what symlinks A might contain, so it would have fewer false positives.

Opt 2 is good. A slight downside is that there is no open flag in Linux which tells open() to fail if any path component is a symlink. O_NOFOLLOW only checks last component. We need to check each one. We will need to lstat() every path component in sequence.

Let's also brainstorm option 3:
Allow symlinks which
(a) start with one or more "../";
(b) never end up on a higher level of directory tree:
"../dir/.." is ok,
"../../usr/bin/.." is ok,
"../file" is not ok,
"../../dir/file" is not ok;
and (c) they also should not leave tarred tree:
symlink "sbin/mkswap" to "../bin/busybox" is ok, but
symlink "mkswap" to "../bin/busybox" is not ok (it hops "above" the tree).

This sounds a bit complicated, but it can be achieved just by looking at names, no multiple lstat() calls for every open() needed.

With only these symlinks allowed, we know that if we untar to an empty directory or to a formerly empty directory with another tarball untarred, any pathname we open, whilst it can contain components which are symlinks, they nevertheless can not allow new opens to "leave" the tree and create files elsewhere.

P.S. let's discuss it in the mailing list, other people may have good ideas.

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.

Do you guys want 1.28.2 build with this fix?

(In reply to Denys Vlasenko from comment #9)

Can't speak for others, but for me there's no urgency, I can just apply your commit until the next release. Thanks though :)

fixed in 1.28.3