[Bug 10651] tar: check for unsafe symlinks is overly strict

[bugzilla at busybox.net]

https://bugs.busybox.net/show_bug.cgi?id=10651

--- Comment #7 from Denys Vlasenko <vda.linux at googlemail.com> ---
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?

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

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

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.

P.S. let's discuss it in the mailing list, other people may have good ideas.

-- 
You are receiving this mail because:
You are on the CC list for the bug.