Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

On Sun, Feb 18, 2018 at 9:35 PM, Harald van Dijk <harald at gigawatt.nl> wrote:
> 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.


Please review attached.

Should the logic also allow e.g. "sbin" -> "../bin" links?
How do we know this is not an attack?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unsafe_symlink_target.c
Type: text/x-csrc
Size: 4397 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20180219/301741cc/attachment.c>