Zip Decompress Parent Path Injection

[stze]

By manipulation of the Zip input file header, the contents of the zip archive can be written to an arbitrary parent path of the user.

Expected behavior

Throw an exception if filename contains a parent directory reference. isValidPath() (ZipCommon.cpp) should check if the filename contains a tilde character.

Actual behavior

By inserting a tilde-slash (~/) in the filename area of the zip header, files can be written to the user's home directory.

Steps to reproduce the problem

Use the sample-unzip samle application as follows:

$ ./sample-unzip -f vuln.zip SOME_OUT_DIR

vuln.zip contains a file foo. foo includes the string bar

vuln.zip hexdump:

00000000  50 4b 03 04 0a 00 00 00  00 00 bb 91 5f 4b e9 b3  |PK.........._K..|
00000010  a2 04 04 00 00 00 04 00  00 00 03 00 1c 00 7e 2f  |..............~/|
00000020  6f 55 54 09 00 03 52 af  f8 59 4d af f8 59 75 78  |oUT...R..YM..Yux|
00000030  0b 00 01 04 e8 03 00 00  04 e8 03 00 00 62 61 72  |.............bar|
00000040  0a 50 4b 01 02 1e 03 0a  00 00 00 00 00 bb 91 5f  |.PK............_|
00000050  4b e9 b3 a2 04 04 00 00  00 04 00 00 00 03 00 18  |K...............|
00000060  00 00 00 00 00 01 00 00  00 b4 81 00 00 00 00 66  |...............f|
00000070  6f 6f 55 54 05 00 03 52  af f8 59 75 78 0b 00 01  |ooUT...R..Yux...|
00000080  04 e8 03 00 00 04 e8 03  00 00 50 4b 05 06 00 00  |..........PK....|
00000090  00 00 01 00 01 00 49 00  00 00 41 00 00 00 00 00  |......I...A.....|
000000a0

After executing the program, a file o with the content bar is written in the home of the user.

~/o

(o is just an example name)

POCO version

9288e89

Compiler and version

clang version 4.0.1 (tags/RELEASE_401/final)

Operating system and version

4.13.9-300.fc27.x86_64 #1 SMP Mon Oct 23 13:41:58 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Security implications

Due to the current behavior of the Zip Decompress mechanism it is possible to write files in parent arbitrary user directories. For example, a manipulated .bashrc could be inserted into the user's home.

Cheers
-Stephan Zeisberg

[aleks-f]

Thanks @stze.
@obiltschnig, I'm looking into this - it also means that an absolute path can be sneaked in surreptitiously?

[obiltschnig]

Hopefully not, I did not write that code, though.

From a quick look I cannot rule that out. I'd change ZipCommon::isValidPath() to also check for paths starting with '/' or '' and reject them. The relevant code calls the two-path constructor of Poco::Path which resolves the second against the first. If the second path (the one from the Zip) is absolute, it will replace the first, so this is a potential issue.

[aleks-f]

ZipCommon::isValidPath() to also check for paths starting with '/' or ''

It is not. Also not checking for eg. "/." or ".".

_flattenDirs flag will cause directory to be stripped off.

I will sort this out and send pull to develop. Seems logical, but just to confirm: is it a valid assumption that all the files from a zip archive should always be decompressed somewhere under the same single directory parent?

/cc @stze

[obiltschnig]

OK, verified that absolute paths are an issue. Fix should be simple, though:

bool ZipCommon::isValidPath(const std::string& path)
{
	if (path == "..")
		return false;
	if (path.compare(0, 3, "../") == 0)
		return false;
	if (path.compare(0, 3, "..\\") == 0)
		return false;
	if (path.find("/..") != std::string::npos)
		return false;
	if (path.find("\\..") != std::string::npos)
		return false;
	if (path.size() > 0 && (path[0] == '/' || path[0] == '\\' || path[0] == '~'))
		return false;
	return true;
}

[obiltschnig]

Yes, Zip files should not contain absolute paths. Info-Zip unzip will strip off leading '/' and print a warning.

[aleks-f]

plus, why eg. "../", "/.." - should be enough to ban any ".." occurrence, right?

[obiltschnig]

Technically, a filename could contain ".." in the middle. So you'll have to check for ".." in combination with trailing or leading slash.

[aleks-f]

yes, i just checked, multiple dots are valid in file name. also, obviously, single dots

[obiltschnig]

While we're at it: technically, a file name could also start with two dots, so something like /..foo would be valid. So maybe isValidPath() could be a bit more accurate.

[obiltschnig]

So, illegal entries (with regards to ..) would be:

• ../ or ..\ at beginning
• /../, /..\, \../ or \..\ anywhere in path
• /.. or \.. at end
• and .. as entire path

[aleks-f]

seems simpler (and more efficient) to pass only directory (without file name) to isValidPath() and ban anywhere:

• absolute path
• .
• ..
• ~/

[obiltschnig]

But then you'd have to extract the directory part first. Zip file entries always contain full paths. So I'd keep it as it is and do the proper tests.

[obiltschnig]

And directory names could also contain ".." or even start with "..". So, no shortcuts here ;-)

[aleks-f]

yes, true

[obiltschnig]

You can probably skip the check for "/.." at end.

[obiltschnig]

So this should basically do it:

bool ZipCommon::isValidPath(const std::string& path)
{
	if (path == "..")
		return false;
	if (path.compare(0, 3, "../") == 0)
		return false;
	if (path.compare(0, 3, "..\\") == 0)
		return false;
	if (path.find("/../") != std::string::npos)
		return false;
	if (path.find("\\..\\") != std::string::npos)
		return false;
	if (path.find("/..\\") != std::string::npos)
		return false;
	if (path.find("\\../") != std::string::npos)
		return false;
	if (path.size() > 0 && (path[0] == '/' || path[0] == '\\' || path[0] == '~'))
		return false;
	return true;
}

A '.' in the path is not a problem.

[obiltschnig]

One more thing, in Decompress::handleZipEntry(), at the two places where a Poco::Path is constructed from the (valid) entry name, we should again check that the path is relative. Otherwise, it may be possible to sneak something like c:\windows\system32 in, which we don't catch in isValidPath().

[aleks-f]

There is also validZipEntryFileName(). I'm thinking maybe best to create a ZipEntryFileName class with all the proper checks consolidated in one place and never use std::string or Poco::Path to hold an entry file name (except, of course, receiving them from the user side and then immediately wrapping them for internal use)?

[obiltschnig]

I'd keep it simple. verifyZipEntryFileName() calls isValidPath() anyway.

[obiltschnig]

... and keeping changes small will save us both work ;-)

[aleks-f]

@obiltschnig fix and test

the rest of changes are cleaning up temp test files and eliminate unused warnings

[aleks-f]

on second thought, this is probably redundant because of this