Proper defense against symlink attacks in tar, unzip et al

[Harald van Dijk]

On 19/02/2018 17:15, Denys Vlasenko wrote:
> On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk <harald at gigawatt.nl> wrote:
>>>>> 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
>>>
>>>
>>> The link to "../../c" is not allowed - it fails criteria (b).
>>
>> You wrote "it can be achieved just by looking at names", but that's not
>> enough here: you have to know that a/a/a is actually b/a, so only one level
>> deep in the -C directory, to know that ../../c points outside the -C
>> directory.
> 
> Yes... but:
> 
>> Since the a/a -> ../b symlink may not even be in this archive,
>> the only way to determine that is by lstat().
> 
> I assume that tarball(s) are being unpacked into an empty directory.

Yes. To be explicit: the case of unpacking a single tarball into an 
empty directory was already handled by your earlier 
<https://git.busybox.net/busybox/commit/?id=b920a38dc0a87f5884444d4731a8b887b5e16018>. 
The point of 
<https://git.busybox.net/busybox/commit/?id=bc9bbeb2b81001e8731cd2ae501c8fccc8d87cc7> 
was to secure unpacking multiple tarballs into the same previously empty 
directory, so I assume that a tarball is being unpacked into a directory 
in which an earlier invocation of tar had potentially already created 
files, but no other tool.

> The case when someone already placed malicious symlinks there
> before unpacking would be a bug in whatever tool allowed _that_
> to happen.
 >
> My goal here is to not allow tar (and unzip) to create such symlinks.

a/a -> ../b is not a malicious symlink, because assuming a is a 
directory, a/a will not point outside the -C directory. That's all you 
can determine from looking at the names. This symlink would be allowed 
by your unsafe_symlink_target function.

a/a/a -> ../../c is not a malicious symlink by itself, because assuming 
a and a/a are directories, it does not point outside the -C directory. 
That's again all you can determine from looking at the names. Based on 
your explanation here, I had thought that you wanted this to be 
accepted, but reading your unsafe_symlink_target function, *this* is the 
symlink which would not be accepted.

However, consider this different example instead:

   cur -> .
   cur/dir -> ../dir
   cur/dir/file

These two symlinks are accepted and there is no indication, just looking 
at each in isolation, that anything is wrong. Still, the end result is 
that ../dir/file would be created, outside of the -C directory.

Cheers,
Harald van Dijk