From bb7e5feece68ccfd8660caee93da25c5c39a4707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnter=20Obiltschnig?= Date: Thu, 2 Nov 2017 09:30:27 +0100 Subject: [PATCH] merge zip entry absolute path vulnerability fix (#1968) from develop --- Zip/src/Decompress.cpp | 4 +- Zip/src/ZipCommon.cpp | 26 +++++- Zip/src/ZipStream.cpp | 1 - Zip/testsuite/some/recursive/dir/test.file | 1 - Zip/testsuite/src/CompressTest.cpp | 37 ++++---- Zip/testsuite/src/ZipTest.cpp | 100 +++++++++++++++++---- Zip/testsuite/src/ZipTest.h | 6 +- 7 files changed, 133 insertions(+), 42 deletions(-) delete mode 100644 Zip/testsuite/some/recursive/dir/test.file diff --git a/Zip/src/Decompress.cpp b/Zip/src/Decompress.cpp index 630bcc7ac..26bd44267 100644 --- a/Zip/src/Decompress.cpp +++ b/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(); diff --git a/Zip/src/ZipCommon.cpp b/Zip/src/ZipCommon.cpp index 60c6b29bc..ecac0ce6d 100644 --- a/Zip/src/ZipCommon.cpp +++ b/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; } diff --git a/Zip/src/ZipStream.cpp b/Zip/src/ZipStream.cpp index 33678c980..cc2c192e6 100644 --- a/Zip/src/ZipStream.cpp +++ b/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(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()) diff --git a/Zip/testsuite/some/recursive/dir/test.file b/Zip/testsuite/some/recursive/dir/test.file deleted file mode 100644 index a87d4977c..000000000 --- a/Zip/testsuite/some/recursive/dir/test.file +++ /dev/null @@ -1 +0,0 @@ -just some test data \ No newline at end of file diff --git a/Zip/testsuite/src/CompressTest.cpp b/Zip/testsuite/src/CompressTest.cpp index ddcda6790..aa459da6d 100644 --- a/Zip/testsuite/src/CompressTest.cpp +++ b/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()) - aFile.remove(true); + if (aFile.exists()) 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 FileMap; std::cout << std::endl; - std::map files; + FileMap files; files["data1.bin"] = static_cast(KB)*4096+1; files["data2.bin"] = static_cast(KB)*16; files["data3.bin"] = static_cast(KB)*4096-1; - for(std::map::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::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::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(); + } } diff --git a/Zip/testsuite/src/ZipTest.cpp b/Zip/testsuite/src/ZipTest.cpp index 64ebdcaa6..09057c9e2 100644 --- a/Zip/testsuite/src/ZipTest.cpp +++ b/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 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(); } @@ -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 >(this, &ZipTest::onDecompressError); dec.decompressAllFiles(); dec.EError -= Poco::Delegate >(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 >(this, &ZipTest::onDecompressError); dec.decompressAllFiles(); dec.EError -= Poco::Delegate >(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 >(this, &ZipTest::onDecompressError); + dec.decompressAllFiles(); + dec.EError -= Poco::Delegate >(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 >(this, &ZipTest::onDecompressError); + dec.decompressAllFiles(); + dec.EError -= Poco::Delegate >(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 >(this, &ZipTest::onDecompressError); dec.decompressAllFiles(); dec.EError -= Poco::Delegate >(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(buffer2.size())); in.read(buffer2.begin(), bytesToRead); - assert(!in.fail() ); - assert(std::memcmp(buffer1.begin(), buffer2.begin(), static_cast(bytesToRead)) == 0); + assert (!in.fail() ); + assert (std::memcmp(buffer1.begin(), buffer2.begin(), static_cast(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 files; - files["data1.bin"] = static_cast(KB)*4096+1; - files["data2.bin"] = static_cast(KB)*16; - files["data3.bin"] = static_cast(KB)*4096-1; + files[Poco::Path::temp() + "data1.bin"] = static_cast(KB)*4096+1; + files[Poco::Path::temp() + "data2.bin"] = static_cast(KB)*16; + files[Poco::Path::temp() + "data3.bin"] = static_cast(KB)*4096-1; for(std::map::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); - Decompress c(in, "."); + std::ifstream in(Poco::Path::temp() + "zip64.zip", std::ios::binary); + Decompress c(in, Poco::Path::temp()); c.decompressAllFiles(); for(std::map::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& 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; } diff --git a/Zip/testsuite/src/ZipTest.h b/Zip/testsuite/src/ZipTest.h index 1e005895d..2ac09ad94 100644 --- a/Zip/testsuite/src/ZipTest.h +++ b/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();