On Sun, Feb 18, 2018 at 9:35 PM, Harald van Dijk <har...@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.

I don't plan to defend against parallel unpacks. They are not realistic.

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

Right.

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

>  a/a/a/a
>
> Here, a/a/../../c doesn't result in c, it results in ../c, and causes ../c/a
> to be created.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to