Proper defense against symlink attacks in tar, unzip et al

Harald van Dijk harald at gigawatt.nl
Sun Feb 18 20:35:12 UTC 2018


On 18/02/2018 21:01, Denys Vlasenko wrote:
> 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?

Good point, dangling symlinks should also be allowed for this approach 
to work. Either the target is inside the -C directory, in which case it 
isn't a security issue, or the target is outside the -C directory, in 
which case no other item in the tarball can cause it to be created, so 
it still isn't a security issue.

> (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 -

Indeed, that's why I think it's okay.

> 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.

Yes, but that requires code execution in an untrustworthy location, 
which would in itself already be another security vulnerability IMO, not 
a busybox issue. If you want to protect against that in tar, you'll also 
need to prevent device nodes from being created, at the least.

> | 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.

In the bug report, I had suggested openat(..., O_NOFOLLOW) for each 
component. lstat() followed by open() can fail if the symlink is created 
after lstat() already returned its result. This may be exploitable if 
whatever mechanism can be used to get a device to extract tarballs 
doesn't block parallel requests.

And for either lstat() or openat(..., O_NOFOLLOW), for the common case 
where the path components of the next item in the tarball are the same 
or mostly the same, it isn't necessary to repeat all the checks each time.

> 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.

This wouldn't be safe, I think. Consider a tarball containing

   a/
   b/
   a/a -> ../b
   a/a/a -> ../../c
   a/a/a/a

Here, a/a/../../c doesn't result in c, it results in ../c, and causes 
../c/a to be created.

Cheers,
Harald van Dijk

P.S.: I use a different e-mail address for bugzilla for easier e-mail 
filtering, removed that address from the reply list. :)


More information about the busybox mailing list