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

Reply via email to