From: =?utf-8?q?G=C3=BCnter_Obiltschnig?= <guenter.obiltschnig@appinf.com>
Date: Thu, 2 Nov 2017 09:30:27 +0100
Subject: merge zip entry absolute path vulnerability fix (#1968) from develop

---
 Zip/src/Decompress.cpp             |  4 +-
 Zip/src/ZipCommon.cpp              | 26 ++++++++++--
 Zip/testsuite/src/CompressTest.cpp | 22 +++++-----
 Zip/testsuite/src/ZipTest.cpp      | 86 ++++++++++++++++++++++++++++++++++----
 Zip/testsuite/src/ZipTest.h        |  6 ++-
 5 files changed, 116 insertions(+), 28 deletions(-)

diff --git a/Zip/src/Decompress.cpp b/Zip/src/Decompress.cpp
index e01f47e..d37b199 100644
--- a/Zip/src/Decompress.cpp
+++ b/Zip/src/Decompress.cpp
@@ -82,7 +82,7 @@ bool Decompress::handleZipEntry(std::istream& zipStream, const ZipLocalFileHeade
 		{
 			std::string dirName = hdr.getFileName();
 			if (!ZipCommon::isValidPath(dirName))
-				throw ZipException("Illegal entry name " + dirName + " containing parent directory reference");
+				throw ZipException("Illegal entry name", dirName);
 			Poco::Path dir(_outDir, dirName);
 			dir.makeDirectory();
 			Poco::File aFile(dir);
@@ -102,7 +102,7 @@ bool Decompress::handleZipEntry(std::istream& zipStream, const ZipLocalFileHeade
 		}
 
 		if (!ZipCommon::isValidPath(fileName))
-			throw ZipException("Illegal entry name " + fileName + " containing parent directory reference");
+			throw ZipException("Illegal entry name", fileName);
 
 		Poco::Path file(fileName);
 		file.makeFile();
diff --git a/Zip/src/ZipCommon.cpp b/Zip/src/ZipCommon.cpp
index fade822..8241258 100644
--- a/Zip/src/ZipCommon.cpp
+++ b/Zip/src/ZipCommon.cpp
@@ -15,6 +15,7 @@
 
 
 #include "Poco/Zip/ZipCommon.h"
+#include "Poco/Path.h"
 
 
 namespace Poco {
@@ -23,16 +24,33 @@ namespace Zip {
 
 bool ZipCommon::isValidPath(const std::string& path)
 {
+	try
+	{
+		if (Path(path, Path::PATH_UNIX).isAbsolute() || Path(path, Path::PATH_WINDOWS).isAbsolute())
+			return false;
+	}
+	catch (...)
+	{
+		return false;
+	}
+
 	if (path == "..")
 		return false;
-	if (path.compare(0, 3, "../") == 0)
+	if ((path.size() >= 3) && path.compare(0, 3, "../") == 0)
+		return false;
+	if ((path.size() >= 3) && path.compare(0, 3, "..\\") == 0)
 		return false;
-	if (path.compare(0, 3, "..\\") == 0)
+	if (path.find("/../") != std::string::npos)
 		return false;
-	if (path.find("/..") != std::string::npos)
+	if (path.find("\\..\\") != std::string::npos)
 		return false;
-	if (path.find("\\..") != std::string::npos)
+	if (path.find("/..\\") != std::string::npos)
 		return false;
+	if (path.find("\\../") != std::string::npos)
+		return false;
+	if ((path.size() >= 2) && path.compare(0, 2, "~/") == 0)
+		return false;
+
 	return true;
 }
 
diff --git a/Zip/testsuite/src/CompressTest.cpp b/Zip/testsuite/src/CompressTest.cpp
index ee8bf84..0d66721 100644
--- a/Zip/testsuite/src/CompressTest.cpp
+++ b/Zip/testsuite/src/CompressTest.cpp
@@ -36,7 +36,7 @@ CompressTest::~CompressTest()
 
 void CompressTest::testSingleFile()
 {
-	std::ofstream out("appinf.zip", std::ios::binary);
+	std::ofstream out(Poco::Path::temp() + "appinf.zip", std::ios::binary);
 	Poco::Path theFile(ZipTest::getTestFile("test.zip"));
 	Compress c(out, true);
 	c.addFile(theFile, theFile.getFileName());
@@ -46,10 +46,9 @@ void CompressTest::testSingleFile()
 
 void CompressTest::testDirectory()
 {
-	std::ofstream out("pocobin.zip", std::ios::binary);
+	std::ofstream out(Poco::Path::temp() + "pocobin.zip", std::ios::binary);
 	Poco::File aFile("some/");
-	if (aFile.exists())
-		aFile.remove(true);
+	if (aFile.exists()) aFile.remove(true);
 	Poco::File aDir("some/recursive/dir/");
 	aDir.createDirectories();
 	Poco::File aDir2("some/other/recursive/dir/");
@@ -65,19 +64,20 @@ void CompressTest::testDirectory()
 	Compress c(out, true);
 	c.addRecursive(theFile, ZipCommon::CL_MAXIMUM, false, theFile);
 	ZipArchive a(c.close());
+	Poco::File(aFile).remove(true);
 }
 
 
 void CompressTest::testManipulator()
 {
 	{
-		std::ofstream out("appinf.zip", std::ios::binary);
+		std::ofstream out(Poco::Path::temp() + "appinf.zip", std::ios::binary);
 		Poco::Path theFile(ZipTest::getTestFile("test.zip"));
 		Compress c(out, true);
 		c.addFile(theFile, theFile.getFileName());
 		ZipArchive a(c.close());
 	}
-	ZipManipulator zm("appinf.zip", true);
+	ZipManipulator zm(Poco::Path::temp() + "appinf.zip", true);
 	zm.renameFile("test.zip", "renamedtest.zip");
 	zm.addFile("doc/othertest.zip", ZipTest::getTestFile("test.zip"));
 	ZipArchive archive=zm.commit();
@@ -88,13 +88,13 @@ void CompressTest::testManipulator()
 void CompressTest::testManipulatorDel()
 {
 	{
-		std::ofstream out("appinf.zip", std::ios::binary);
+		std::ofstream out(Poco::Path::temp() + "appinf.zip", std::ios::binary);
 		Poco::Path theFile(ZipTest::getTestFile("test.zip"));
 		Compress c(out, true);
 		c.addFile(theFile, theFile.getFileName());
 		ZipArchive a(c.close());
 	}
-	ZipManipulator zm("appinf.zip", true);
+	ZipManipulator zm(Poco::Path::temp() + "appinf.zip", true);
 	zm.deleteFile("test.zip");
 	zm.addFile("doc/data.zip", ZipTest::getTestFile("data.zip"));
 	ZipArchive archive=zm.commit();
@@ -106,13 +106,13 @@ void CompressTest::testManipulatorDel()
 void CompressTest::testManipulatorReplace()
 {
 	{
-		std::ofstream out("appinf.zip", std::ios::binary);
+		std::ofstream out(Poco::Path::temp() + "appinf.zip", std::ios::binary);
 		Poco::Path theFile(ZipTest::getTestFile("test.zip"));
 		Compress c(out, true);
 		c.addFile(theFile, theFile.getFileName());
 		ZipArchive a(c.close());
 	}
-	ZipManipulator zm("appinf.zip", true);
+	ZipManipulator zm(Poco::Path::temp() + "appinf.zip", true);
 	zm.replaceFile("test.zip", ZipTest::getTestFile("doc.zip"));
 	
 	ZipArchive archive=zm.commit();
@@ -124,7 +124,7 @@ void CompressTest::testManipulatorReplace()
 void CompressTest::testSetZipComment()
 {
 	std::string comment("Testing...123...");
-	std::ofstream out("comment.zip", std::ios::binary);
+	std::ofstream out(Poco::Path::temp() + "comment.zip", std::ios::binary);
 	Poco::Path theFile(ZipTest::getTestFile("test.zip"));
 	Compress c(out, true);
 	c.addFile(theFile, theFile.getFileName());
diff --git a/Zip/testsuite/src/ZipTest.cpp b/Zip/testsuite/src/ZipTest.cpp
index 0b89bd9..42562f9 100644
--- a/Zip/testsuite/src/ZipTest.cpp
+++ b/Zip/testsuite/src/ZipTest.cpp
@@ -51,16 +51,16 @@ void ZipTest::testSkipSingleFile()
 	ZipLocalFileHeader hdr(inp, false, skip);
 	assert (ZipCommon::HS_FAT == hdr.getHostSystem());
 	int major = hdr.getMajorVersionNumber();
-	int minor = hdr.getMinorVersionNumber();
+	int POCO_UNUSED minor = hdr.getMinorVersionNumber();
 	assert (major <= 2);
 	std::size_t hdrSize = hdr.getHeaderSize();
 	assert (hdrSize > 30);
-	ZipCommon::CompressionMethod cm = hdr.getCompressionMethod();
+	ZipCommon::CompressionMethod POCO_UNUSED cm = hdr.getCompressionMethod();
 	assert (!hdr.isEncrypted());
 	Poco::DateTime aDate = hdr.lastModifiedAt();
-	Poco::UInt32 cS = hdr.getCompressedSize();
-	Poco::UInt32 uS = hdr.getUncompressedSize();
-	const std::string& fileName = hdr.getFileName();
+	Poco::UInt64 POCO_UNUSED cS = hdr.getCompressedSize();
+	Poco::UInt64 POCO_UNUSED uS = hdr.getUncompressedSize();
+	const std::string& POCO_UNUSED fileName = hdr.getFileName();
 }
 
 
@@ -84,7 +84,7 @@ void ZipTest::testCrcAndSizeAfterData()
 	std::string testFile = getTestFile("data.zip");
 	std::ifstream inp(testFile.c_str(), std::ios::binary);
 	assert (inp.good());
-	Decompress dec(inp, Poco::Path());
+	Decompress dec(inp, Poco::Path::temp());
 	dec.EError += Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
 	dec.decompressAllFiles();
 	dec.EError -= Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
@@ -108,7 +108,7 @@ void ZipTest::testCrcAndSizeAfterDataWithArchive()
 		Poco::Path path(it->second.getFileName());
 		if (path.isFile())
 		{
-			std::ofstream os("test.dat");
+			std::ofstream os(Poco::Path::temp() + "test.dat");
 			Poco::StreamCopier::copyStream(zipis,os);
 		}
 	}
@@ -141,7 +141,7 @@ void ZipTest::testDecompress()
 	std::string testFile = getTestFile("test.zip");
 	std::ifstream inp(testFile.c_str(), std::ios::binary);
 	assert (inp.good());
-	Decompress dec(inp, Poco::Path());
+	Decompress dec(inp, Poco::Path::temp());
 	dec.EError += Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
 	dec.decompressAllFiles();
 	dec.EError -= Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
@@ -155,7 +155,7 @@ void ZipTest::testDecompressFlat()
 	std::string testFile = getTestFile("test.zip");
 	std::ifstream inp(testFile.c_str(), std::ios::binary);
 	assert (inp.good());
-	Decompress dec(inp, Poco::Path(), true);
+	Decompress dec(inp, Poco::Path::temp(), true);
 	dec.EError += Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
 	dec.decompressAllFiles();
 	dec.EError -= Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
@@ -164,6 +164,71 @@ void ZipTest::testDecompressFlat()
 }
 
 
+void ZipTest::testDecompressVuln()
+{
+	std::string testFile = getTestFile("vuln.zip");
+	std::ifstream inp(testFile.c_str(), std::ios::binary);
+	assert(inp.good());
+	Decompress dec(inp, Poco::Path::temp());
+	dec.EError += Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
+	dec.decompressAllFiles();
+	dec.EError -= Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
+	assert (_errCnt == 1);
+	assert (dec.mapping().empty());
+}
+
+
+void ZipTest::testDecompressFlatVuln()
+{
+	std::string testFile = getTestFile("vuln.zip");
+	std::ifstream inp(testFile.c_str(), std::ios::binary);
+	assert(inp.good());
+	Decompress dec(inp, Poco::Path::temp(), true);
+	dec.EError += Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
+	dec.decompressAllFiles();
+	dec.EError -= Poco::Delegate<ZipTest, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string> >(this, &ZipTest::onDecompressError);
+	assert (_errCnt == 0);
+	assert (!dec.mapping().empty());
+}
+
+
+void ZipTest::testValidPath()
+{
+	assert (ZipCommon::isValidPath("."));
+	assert (ZipCommon::isValidPath("file.txt"));
+	assert (ZipCommon::isValidPath(".file.txt"));
+	assert (ZipCommon::isValidPath("..file.txt"));
+	assert (ZipCommon::isValidPath("file.txt.."));
+	assert (ZipCommon::isValidPath(".file..txt"));
+	assert (ZipCommon::isValidPath("~file..txt"));
+	assert (ZipCommon::isValidPath("~file/~"));
+	assert (ZipCommon::isValidPath("dir/~"));
+	assert (ZipCommon::isValidPath("some"));
+	assert (ZipCommon::isValidPath("some/dir"));
+	assert (ZipCommon::isValidPath("some/dir/or/another"));
+	assert (ZipCommon::isValidPath("some/dir/./another"));
+	assert (ZipCommon::isValidPath("some/dir/or/another/file.txt"));
+	assert (ZipCommon::isValidPath("s~me\\d.r\\.or..\\an..her\\file.txt"));
+	assert (ZipCommon::isValidPath("some\\dir\\or\\another"));
+	assert (ZipCommon::isValidPath("some\\dir\\or\\another\\file.txt"));
+	assert (ZipCommon::isValidPath("s~me\\d.r/.or..\\an..her\\file.txt"));
+
+	assert (!ZipCommon::isValidPath("/../"));
+	assert (!ZipCommon::isValidPath("/"));
+	assert (!ZipCommon::isValidPath("\\..\\"));
+	assert (!ZipCommon::isValidPath("/..\\"));
+	assert (!ZipCommon::isValidPath("\\../"));
+	assert (!ZipCommon::isValidPath(".."));
+	assert (!ZipCommon::isValidPath("~/"));
+	assert (!ZipCommon::isValidPath("~/~"));
+	assert (!ZipCommon::isValidPath("/~"));
+	assert (!ZipCommon::isValidPath("/file.txt"));
+	assert (!ZipCommon::isValidPath("~/file.txt"));
+	assert (!ZipCommon::isValidPath("some/dir/or/../another/file.txt"));
+	assert (!ZipCommon::isValidPath("C:\\Windows\\system32"));
+}
+
+
 void ZipTest::onDecompressError(const void* pSender, std::pair<const Poco::Zip::ZipLocalFileHeader, const std::string>& info)
 {
 	++_errCnt;
@@ -189,7 +254,10 @@ CppUnit::Test* ZipTest::suite()
 	CppUnit_addTest(pSuite, ZipTest, testDecompressSingleFile);
 	CppUnit_addTest(pSuite, ZipTest, testDecompress);
 	CppUnit_addTest(pSuite, ZipTest, testDecompressFlat);
+	CppUnit_addTest(pSuite, ZipTest, testDecompressVuln);
+	CppUnit_addTest(pSuite, ZipTest, testDecompressFlatVuln);
 	CppUnit_addTest(pSuite, ZipTest, testCrcAndSizeAfterData);
 	CppUnit_addTest(pSuite, ZipTest, testCrcAndSizeAfterDataWithArchive);
+	CppUnit_addTest(pSuite, ZipTest, testValidPath);
 	return pSuite;
 }
diff --git a/Zip/testsuite/src/ZipTest.h b/Zip/testsuite/src/ZipTest.h
index 8f8adee..17ae48c 100644
--- a/Zip/testsuite/src/ZipTest.h
+++ b/Zip/testsuite/src/ZipTest.h
@@ -30,10 +30,12 @@ public:
 	void testSkipSingleFile();
 	void testDecompressSingleFile();
 	void testDecompress();
+	void testDecompressFlat();
+	void testDecompressVuln();
+	void testDecompressFlatVuln();
 	void testCrcAndSizeAfterData();
 	void testCrcAndSizeAfterDataWithArchive();
-
-	void testDecompressFlat();
+	void testValidPath();
 
 	void setUp();
 	void tearDown();