[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: Proper defense against symlink attacks in tar, unzip et al
From: Denys Vlasenko <vda.linux () googlemail ! com>
Date: 2018-03-09 15:55:50
Message-ID: CAK1hOcMooRe343+mm1U1Yb9RM5i6_vREKFBO-mh6+QoKWfAP3g () mail ! gmail ! com
[Download RAW message or body]
Do you guys want 1.28.2 build with this fix?
On Tue, Feb 20, 2018 at 4:09 PM, Denys Vlasenko
<vda.linux@googlemail.com> wrote:
> On Mon, Feb 19, 2018 at 9:07 PM, Denys Vlasenko
> <vda.linux@googlemail.com> wrote:
>> On Mon, Feb 19, 2018 at 8:52 PM, Harald van Dijk <harald@gigawatt.nl> wrote:
>>> On 19/02/2018 17:15, Denys Vlasenko wrote:
>>>> On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk <harald@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.linux@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic