On Mon, Feb 19, 2018 at 8:52 PM, Harald van Dijk <har...@gigawatt.nl> wrote:
> On 19/02/2018 17:15, Denys Vlasenko wrote:
>> On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk <har...@gigawatt.nl>
>>>>>> 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.

It is malicious and will not be allowed by the code I propose:
it has two ".." leading components but only one subsequent component.
Specifically, this part of code will reject it:

+               /* Now skip the same number of non-dot[dot]
+                * components in link target as it had leading ".." components
+                * - or bail out with error.
+                */
+               do {
+                       unsigned len = strchrnul(target, '/') - target;
+                       if (len == 0) {
+                               /* Examples: "../", "../../abc", "../../abc/" */
+                               return 1; /* unsafe */
+                       }
+                       if (len <= 2
+                        && (len < 2 || target[1] == '.')
+                        && target[0] == '.'
+                       ) {
+                               /* "." or ".." */
+                               return 1; /* unsafe */
+                       }
+                       target += len;
+                       while (*target == '/') target++;
+                       up_count--;
+               } while (up_count != 0);

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

Exactly.

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

Indeed :(
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to