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.
>
> 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 unsafe_symlink_target(const char *target, const char *linkname)
{
	const char *dot;
	unsigned up_count;

	if (target[0] == '/') {
		const char *var;
 unsafe:
		var = getenv("EXTRACT_UNSAFE_SYMLINKS");
		if (var) {
			if (LONE_CHAR(var, '1'))
				return 0; /* pretend it's safe */
			return 1; /* "UNSAFE!" */
		}
		bb_error_msg("skipping unsafe symlink to '%s' in archive,"
			" set %s=1 to extract",
			target,
			"EXTRACT_UNSAFE_SYMLINKS"
		);
		/* Prevent further messages */
		setenv("EXTRACT_UNSAFE_SYMLINKS", "0", 0);
		return 1; /* "UNSAFE!" */
	}

	/*
	 * 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,
	 * "../" is not ok,
	 * "../../dir" 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).
	 *
	 * 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.
	 */
	up_count = 0;
	while (is_prefixed_with(target, "../")) {
		up_count++;
		target += 3;
		while (*target == '/') target++;
	}
	if (up_count != 0) {
		/* Skipped UP_COUNT ".." leading components in target name */

		/* Verify that linkname is at least that deep in the unpacked tree.
		 * We do not verify that linkname isn't starting with '/', has no ".."
		 * components and such - any unpacked filename needs to be sanitized
		 * against that, not only symlinks. It's caller's responsibility
		 * (usually caller uses strip_unsafe_prefix()).
		 * However, "." components _must be_ checked and rejected.
		 */
		unsigned n = up_count;
		do {
			if (linkname[0] == '.' && linkname[1] == '/')
				return 1; /* unsafe */
			linkname = strchr(linkname, '/');
			if (!linkname)
				return 1; /* unsafe */
			do linkname++; while (*linkname == '/');
			n--;
		} while (n != 0);
		/* Don't be tricked by "path/to/" kind of name:
		 * trailing slash, there is no third component in this name!
		 */
		if (!linkname[0])
			return 1; /* unsafe */

		/* Now skip the same number of non-dot[dot]
		 * components in link target as it had ".." 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);
	}

	/* None of the remaining components are allowed to be ".." */
	dot = target;
	for (;;) {
		dot = strchr(dot, '.');
		if (!dot)
			return 0; /* safe target */

		/* Is it a path component starting with ".."? */
		if ((dot[1] == '.')
		 && (dot == target || dot[-1] == '/')
		    /* Is it exactly ".."? */
		 && (dot[2] == '/' || dot[2] == '\0')
		) {
			goto unsafe;
		}
		/* NB: it can even be trailing ".", should only add 1 */
		dot += 1;
	}
}
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to