merge zip entry absolute path vulnerability fix (#1968) from develop

Zip/src/Decompress.cpp

@@ -80,7 +80,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);
@@ -100,7 +100,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();

Zip/src/ZipCommon.cpp

@@ -13,6 +13,7 @@
 
 
 #include "Poco/Zip/ZipCommon.h"
+#include "Poco/Path.h"
 
 
 namespace Poco {
@@ -21,16 +22,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.compare(0, 3, "..\\") == 0)
+	if ((path.size() >= 3) && path.compare(0, 3, "..\\") == 0)
 		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.find("\\../") != std::string::npos)
+		return false;
+	if ((path.size() >= 2) && path.compare(0, 2, "~/") == 0)
+		return false;
+
 	return true;
 }
 

Zip/src/ZipStream.cpp

@@ -174,7 +174,6 @@ int ZipStreamBuf::readFromDevice(char* buffer, std::streamsize length)
 				// now push back the header to the stream, so that the ZipLocalFileHeader can read it
 				Poco::Int32 size = static_cast<Poco::Int32>(nfo.getFullHeaderSize());
 				_expectedCrc32 = nfo.getCRC32();
-				const char* rawHeader = nfo.getRawHeader();
 				_pIstr->seekg(-size, std::ios::cur);
 				if (!_pIstr->good()) throw Poco::IOException("Failed to seek on input stream");
 				if (!crcValid())

Zip/testsuite/some/recursive/dir/test.file

@@ -1 +0,0 @@
-just some test data

Zip/testsuite/src/CompressTest.cpp

@@ -38,7 +38,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("data", "test.zip"));
 	Compress c(out, true);
 	c.addFile(theFile, theFile.getFileName());
@@ -48,10 +48,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())
+	if (aFile.exists()) aFile.remove(true);
-		aFile.remove(true);
 	Poco::File aDir("some/recursive/dir/");
 	aDir.createDirectories();
 	Poco::File aDir2("some/other/recursive/dir/");
@@ -67,19 +66,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("data", "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("data", "test.zip"));
 	ZipArchive archive=zm.commit();
@@ -90,13 +90,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("data", "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", "data.zip"));
 	ZipArchive archive=zm.commit();
@@ -108,13 +108,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("data", "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("data", "doc.zip"));
 	
 	ZipArchive archive=zm.commit();
@@ -126,7 +126,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("data", "test.zip"));
 	Compress c(out, true);
 	c.addFile(theFile, theFile.getFileName());
@@ -157,27 +157,28 @@ void CompressTest::createDataFile(const std::string& path, Poco::UInt64 size)
 
 void CompressTest::testZip64()
 {
+	typedef std::map<std::string, Poco::UInt64> FileMap;
 	std::cout << std::endl;
-	std::map<std::string, Poco::UInt64> files;
+	FileMap files;
 	files["data1.bin"] = static_cast<Poco::UInt64>(KB)*4096+1;
 	files["data2.bin"] = static_cast<Poco::UInt64>(KB)*16;
 	files["data3.bin"] = static_cast<Poco::UInt64>(KB)*4096-1;
 	
-	for(std::map<std::string, Poco::UInt64>::const_iterator it = files.begin(); it != files.end(); it++)
+	for(FileMap::const_iterator it = files.begin(); it != files.end(); it++)
 	{
 		std::cout << '\t' << "createDataFile(" << it->first << ", " << it->second << ");" << std::endl;
 		createDataFile(it->first, it->second);
 	}
-	std::ofstream out("zip64.zip", std::ios::binary | std::ios::trunc);
+	std::ofstream out(Poco::Path::temp() + "zip64.zip", std::ios::binary | std::ios::trunc);
 	Compress c(out, true, true);
-	for(std::map<std::string, Poco::UInt64>::const_iterator it = files.begin(); it != files.end(); it++)
+	for(FileMap::const_iterator it = files.begin(); it != files.end(); it++)
 	{
 		const std::string& path = it->first;
 		std::cout << '\t' << "addFile(" << path <<  ");" << std::endl;
 		c.addFile(path, path, ZipCommon::CM_STORE);
 	}
 	ZipArchive a(c.close());
-	for(std::map<std::string, Poco::UInt64>::const_iterator it = files.begin(); it != files.end(); it++)
+	for(FileMap::const_iterator it = files.begin(); it != files.end(); it++)
 	{
 		const std::string& path = it->first;
 		Poco::UInt64 size = it->second;
@@ -187,6 +188,10 @@ void CompressTest::testZip64()
 		assert(file.getUncompressedSize() == size);
 		assert(file.getCompressedSize() == size);
 	}
+	for (FileMap::const_iterator it = files.begin(); it != files.end(); it++)
+	{
+		Poco::File(it->first).remove();
+	}
 }
 
 

Zip/testsuite/src/ZipTest.cpp

@@ -53,16 +53,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::UInt64 cS = hdr.getCompressedSize();
+	Poco::UInt64 POCO_UNUSED cS = hdr.getCompressedSize();
-	Poco::UInt64 uS = hdr.getUncompressedSize();
+	Poco::UInt64 POCO_UNUSED uS = hdr.getUncompressedSize();
-	const std::string& fileName = hdr.getFileName();
+	const std::string& POCO_UNUSED fileName = hdr.getFileName();
 }
 
 
@@ -101,7 +101,7 @@ void ZipTest::testCrcAndSizeAfterData()
 	std::string testFile = getTestFile("data", "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);
@@ -125,7 +125,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);
 		}
 	}
@@ -162,7 +162,7 @@ void ZipTest::testDecompress()
 	std::string testFile = getTestFile("data", "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);
@@ -176,7 +176,35 @@ void ZipTest::testDecompressFlat()
 	std::string testFile = getTestFile("data", "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);
+	assert (_errCnt == 0);
+	assert (!dec.mapping().empty());
+}
+
+
+void ZipTest::testDecompressVuln()
+{
+	std::string testFile = getTestFile("data", "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("data", "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);
@@ -197,8 +225,8 @@ void ZipTest::verifyDataFile(const std::string& path, Poco::UInt64 size)
 		std::memset(buffer2.begin(), 0, buffer2.size());
 		Poco::UInt64 bytesToRead = std::min(size, static_cast<Poco::UInt64>(buffer2.size()));
 		in.read(buffer2.begin(), bytesToRead);
-		assert(!in.fail() );
+		assert (!in.fail() );
-		assert(std::memcmp(buffer1.begin(), buffer2.begin(), static_cast<std::size_t>(bytesToRead)) == 0);
+		assert (std::memcmp(buffer1.begin(), buffer2.begin(), static_cast<std::size_t>(bytesToRead)) == 0);
 		size -= bytesToRead;
 	}
 	char c;
@@ -210,9 +238,9 @@ void ZipTest::verifyDataFile(const std::string& path, Poco::UInt64 size)
 void ZipTest::testDecompressZip64()
 {
 	std::map<std::string, Poco::UInt64> files;
-	files["data1.bin"] = static_cast<Poco::UInt64>(KB)*4096+1;
+	files[Poco::Path::temp() + "data1.bin"] = static_cast<Poco::UInt64>(KB)*4096+1;
-	files["data2.bin"] = static_cast<Poco::UInt64>(KB)*16;
+	files[Poco::Path::temp() + "data2.bin"] = static_cast<Poco::UInt64>(KB)*16;
-	files["data3.bin"] = static_cast<Poco::UInt64>(KB)*4096-1;
+	files[Poco::Path::temp() + "data3.bin"] = static_cast<Poco::UInt64>(KB)*4096-1;
 
 	for(std::map<std::string, Poco::UInt64>::const_iterator it = files.begin(); it != files.end(); it++)
 	{
@@ -220,8 +248,8 @@ void ZipTest::testDecompressZip64()
 		if(file.exists())
 			file.remove();
 	}
-	std::ifstream in("zip64.zip", std::ios::binary);
+	std::ifstream in(Poco::Path::temp() + "zip64.zip", std::ios::binary);
-	Decompress c(in, ".");
+	Decompress c(in, Poco::Path::temp());
 	c.decompressAllFiles();
 	for(std::map<std::string, Poco::UInt64>::const_iterator it = files.begin(); it != files.end(); it++)
 	{
@@ -230,6 +258,43 @@ void ZipTest::testDecompressZip64()
 }
 
 
+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;
@@ -256,9 +321,12 @@ CppUnit::Test* ZipTest::suite()
 	CppUnit_addTest(pSuite, ZipTest, testDecompressSingleFileInDir);
 	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, testDecompressZip64);
+	CppUnit_addTest(pSuite, ZipTest, testValidPath);
 
 	return pSuite;
 }

Zip/testsuite/src/ZipTest.h

@@ -29,15 +29,17 @@ public:
 	void testDecompressSingleFile();
 	void testDecompressSingleFileInDir();
 	void testDecompress();
+	void testDecompressFlat();
+	void testDecompressVuln();
+	void testDecompressFlatVuln();
 	void testCrcAndSizeAfterData();
 	void testCrcAndSizeAfterDataWithArchive();
 
-	void testDecompressFlat();
-
 	static const Poco::UInt64 KB = 1024;
 	static const Poco::UInt64 MB = 1024*KB;
 	void verifyDataFile(const std::string& path, Poco::UInt64 size);
 	void testDecompressZip64();
+	void testValidPath();
 
 	void setUp();
 	void tearDown();