Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

Do you guys want 1.28.2 build with this fix?

On Tue, Feb 20, 2018 at 4:09 PM, Denys Vlasenko
 wrote:
> On Mon, Feb 19, 2018 at 9:07 PM, Denys Vlasenko
>  wrote:
>> On Mon, Feb 19, 2018 at 8:52 PM, Harald van Dijk  wrote:
>>> On 19/02/2018 17:15, Denys Vlasenko wrote:
 On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk 
 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
>>> .
>>> The point of
>>> 
>>> 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 -> .
>

Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

On Mon, Feb 19, 2018 at 9:07 PM, Denys Vlasenko
 wrote:
> On Mon, Feb 19, 2018 at 8:52 PM, Harald van Dijk  wrote:
>> On 19/02/2018 17:15, Denys Vlasenko wrote:
>>> On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk 
>>> 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
>> .
>> The point of
>> 
>> 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

Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

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

Re: 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  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 
. 
The point of 
 
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
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: Proper defense against symlink attacks in tar, unzip et al

[Michael Conrad]

On 2/19/2018 11:15 AM, Denys Vlasenko wrote:

On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk  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.
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.


That might be considered a bug though.  There's lots of times a symlink 
points to a higher directory (or even starts with '/') and tar should be 
a reliable way to mirror a directory tree.


What if you do a test on the inode of the destination directory?  To 
determine whether it is valid, repeatedly append ".." and stat until you 
reach root or the unpack directory.  Then to speed it up, make a 
fixed-size LRU cache of directories you know are valid.  Each time you 
create a directory, add it to the cache, and then on average you 
probably only add one directory stat per unpacked file.  For more speed 
and less security, just change it to a cache of path names.


-Mike
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

On Mon, Feb 19, 2018 at 5:09 PM, Harald van Dijk  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.
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.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: Proper defense against symlink attacks in tar, unzip et al

[Harald van Dijk]

On 19/02/2018 15:47, Denys Vlasenko 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. Since the a/a -> ../b symlink may not even be in this 
archive, the only way to determine that is by lstat().


Cheers,
Harald van Dijk
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

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


Please review attached.

Should the logic also allow e.g. "sbin" -> "../bin" links?
How do we know this is not an attack?
/* vi: set sw=4 ts=4: */
/*
 * Licensed under GPLv2 or later, see file LICENSE in this source tree.
 */
#include "libbb.h"
#include "bb_archive.h"

/* To avoid a directory traversal attack via symlinks,
 * do not restore symlinks with ".." components
 * or symlinks starting with "/", unless a magic
 * envvar is set.
 *
 * For example, consider a .tar created via:
 *  $ tar cvf bug.tar anything.txt
 *  $ ln -s /tmp symlink
 *  $ tar --append -f bug.tar symlink
 *  $ rm symlink
 *  $ mkdir symlink
 *  $ tar --append -f bug.tar symlink/evil.py
 *
 * This will result in an archive that contains:
 *  $ tar --list -f bug.tar
 *  anything.txt
 *  symlink [-> /tmp]
 *  symlink/evil.py
 *
 * Untarring bug.tar would otherwise place evil.py in '/tmp'.
 *
 * We have additional logic which allows some forms of symlinks
 * with ".." components which do not leave unpacked tree,
 * since they are typical in tarballs of busyboxed filesystems
 * e.g. sbin-to-bin links: sbin/mkswap -> ../bin/busybox
 * and we definitely want that to work out-of-the-box.
 */
int FAST_FUNC

Re: Proper defense against symlink attacks in tar, unzip et al

[Denys Vlasenko]

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

Re: Proper defense against symlink attacks in tar, unzip et al

[Harald van Dijk]

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.


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.



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
  a/a/a/a

Here, a/a/../../c doesn't result in c, it results in ../c, and causes 
../c/a to be created.


Cheers,
Harald van Dijk

P.S.: I use a different e-mail address for bugzilla for easier e-mail 
filtering, removed that address from the reply list. :)

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox