#147 Directory traversal through symlinks

[Jesse Kornblum]

As reported in Debian bug 774660, https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=774660, p7zip is vulnerable to a directory traversal bug. I have verified that version 9.38.1 is still vulnerable.

[my p7zip]

p7zip like 7zip can store absolute filenames or pathnames.
So they can write outside the current directory.

What do you want p7zip to do with this directory traversal bug ?

I think that if the user can write a file (for example in /tmp) then p7zip can write this file and according to the user's permission, it's ok, don't you think so ?

[Philippe Ombredanne]

@my p7zip you wrote:

p7zip like 7zip can store absolute filenames or pathnames.
So they can write outside the current directory.
==> this is NOT true with 7zip 9.38.
The directory traversal bug is due to symlink handling code added in the p7zip port only

[Jesse Kornblum]

I agree that p7zip can and should write to absolute paths.

My issue is that I can't determine that an archive will be writing to an absolute path before expanding it. (Perhaps there's an option I'm missing?) But looking at the archive described in the Debian bug report with technical information, there's no indication that it would be writing to /tmp:

$ 7z l -slt test.7z

There are two solutions which would work for me:

1. Adding a switch to show where files will be written or at least where symbolic links will point.

2. Adding a switch to prevent writing to absolute file paths; limit output to a specified directory. If the archive wants to write to /etc/passwd, but I specify writing to ./tmp, the program should write to ./tmp/etc/passwd.

[Philippe Ombredanne]

@jesse you wrote:

I agree that p7zip can and should write to absolute paths.
I rather disagree with this.

The contract is to extract all files in a given directory. Ill formed and dangerous archives trying to extract things outside of that extract dir should be by default either have their paths rewritten (the default 7zip behavior) or failing extraction.

To your point if absolute extraction were to be supported, it should be a flag AND an improved simple directory listing (as opposed to -slt) should list like ls -al both a path and the linkpath.

[Philippe Ombredanne]

Adding to this, which is a regression IMHO in 9.38.1, is that a zip with relative paths is now extracted outside of the directory where you asked extraction for. This was not the case with 9.20.
So combined with symlink traversal, a malign archive can be extracted in funky and dangerous locations in many ways

[Philippe Ombredanne]

FYI, this is has become an official CVE https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2015-1038 meaning that distros such as debian are patching tools that depend on it (file-roller) to remove p7zip deps. I guess it would be better if they could patch p7zip but downstream maintainers may not know how to.

There is also a bug at RedHat: https://bugzilla.redhat.com/show_bug.cgi?id=1179505

[Philippe Ombredanne]

convert_to_symlink() in CPP/Windows/FileDir.cpp is where things happen in 9.38.1
See here (NB: the comment is my own addition)
https://github.com/pombredanne/7zip-and-p7zip/blob/bug_pzip_147/CPP/Windows/FileDir.cpp#L330

The fix could come in a few shapes:

1. never convert symlink-extracted-as-a-file to a real symlink: this is the default windows behavior.
2. convert symlink but strip leading dir separator, dot and dotdot dirs from the link, eventually resulting in a broken link even if the linked-to file actually exists in the extracted payload
3. do the right thing but more complex which is to resolve the link path and ensure the link points inside the extraction directory. If yes, then create a link. If no, do either 1. or 2.

[Sworddragon]

I also think there should be a 3-way toggle option:

1. Allow extraction to any location.
2. Rewrite any paths that would effectively cross at least 1 level above the extraction target.
3. Ignore any paths that would effectively cross at least 1 level above the extraction target.

The default should be either 2. or 3. and if any path gets rewritten/ignored a hint with some details could be written to stderr too. Additionally an extended listing as suggested in a previous post would be helpful too.

[Ben Hutchings]

I believe the correct thing to do is to defer conversion of placeholder files into symlinks until after all files and directories have been created. This is roughly what GNU tar does, for example. I'm attaching a patch that implements this behaviour. This is against 9.20.1 (the version in Debian) but I can have a look at the current version too.

[jlec]

Ben, could you also provide a patch against 9.38.1 please?

[Ben Hutchings]

Here's a patch for 9.38.1. I tested both the 7z and Client7z front-ends with some simple cases.

[Jesse Kornblum]

I attempted to apply this patch to 9.38.1 today, but it doesn't compile. I'm on OS X Yosemite. Here are the commands I used:

$ patch -p1 -b < ../p7zip-9.38.1-CVE-2015-1038.patch
patching file CPP/7zip/UI/Agent/Agent.cpp
patching file CPP/7zip/UI/Client7z/Client7z.cpp
patching file CPP/7zip/UI/Common/ArchiveExtractCallback.cpp
patching file CPP/7zip/UI/Common/ArchiveExtractCallback.h
patching file CPP/7zip/UI/Common/Extract.cpp
patching file CPP/Windows/FileDir.cpp
patching file CPP/Windows/FileDir.h

$ make
mkdir -p bin
/Library/Developer/CommandLineTools/usr/bin/make -C CPP/7zip/Bundles/Alone all
c++ -c -I. -I../../../myWindows -I../../../ -I../../../include_windows -m64 -O -DENV_MACOSX -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_REENTRANT -DENV_UNIX -DBREAK_HANDLER -DUNICODE -D_UNICODE ../../../../CPP/7zip/Archive/7z/7zEncode.cpp
In file included from ../../../../CPP/7zip/Archive/7z/7zEncode.cpp:8:
In file included from ../../../../CPP/7zip/Archive/7z/../../Common/InOutTempBuffer.h:7:
../../../../CPP/7zip/Archive/7z/../../Common/../../Windows/FileDir.h:89:3: error:
unknown type name 'ino_t'
ino_t _ino;
^
1 error generated.
make[1]: *** [7zEncode.o] Error 1
make: *** [7za] Error 2

$ c++ -v
Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.5.0
Thread model: posix

[Ben Hutchings]

Sorry, it seems that on Linux the definition of ino_t is already included. For portability I think this is needed near the top of CPP/Windows/FileDir.h:

#ifdef ENV_UNIX
#include <sys/types.h>
#endif

[Jesse Kornblum]

Yes, that works, thank you!

Has anybody else tested this patch? What's the process for official testing and getting this patched?

[Philippe Ombredanne]

This is a good question. myspace does not seem to have a clear process for this...

[Sworddragon]

Can somebody create a test archive that does write a file to /tmp with the symlink bug and another file to /tmp too with p7zip's absolute path functionality? I tried both with p7zip 9.20.1 but had no success.

[Ben Hutchings]

The Debian bug report explains how to create such an archive. Did those instructions not work for you?

[Sworddragon]

I have tried the symlink bug as described there a few weeks ago and it hasn't worked. Yesterday I have made some tests with absolute paths but had no success on packing a file absolute with extracting it back to the absolute path but maybe I have just made something wrong.

[Sérgio M. Basto]

here is the patch for p7zip-15.09

[txtsd]

Why is this not in the official release yet?

[Sérgio M. Basto]

Is now released in p7zip_15.14 ? , the patch attach https://sourceforge.net/p/p7zip/bugs/_discuss/thread/17901103/fbdf/attachment/p7zip-15.09-CVE-2015-1038.patch , doesn't apply , but just half of the patch .

[my p7zip]

please try p7zip 15.14.