On Mon, Feb 19, 2018 at 9:07 PM, Denys Vlasenko
<vda.li...@googlemail.com> wrote:
> 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 :(
commit a84db18fc71d09e801df0ebca048d82e90b32c6a
Author: Denys Vlasenko <vda.li...@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 code.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
