feat(Poco::Zip): Check archive consistency before extracting (#4807)

* feat(Zip): add checkConsistency() method for checking archive's consistency * refactor(Zip): check archive consistency when decompressing all files * test(Zip): add archive consistency tests * refactor(Zip): make archive consistency check optional * test(Zip): test optional consistency check
pocoproject · Dec 27, 2024 · a8bac05 · a8bac05
1 parent 854d8c8
commit a8bac05
Show file tree

Hide file tree

Showing 8 changed files with 114 additions and 2 deletions.
diff --git a/Zip/include/Poco/Zip/Decompress.h b/Zip/include/Poco/Zip/Decompress.h
@@ -55,9 +55,10 @@ class Zip_API Decompress: public ParseCallback
 	~Decompress() override;
 		/// Destroys the Decompress.
 
-	ZipArchive decompressAllFiles();
+	ZipArchive decompressAllFiles(const bool checkConsistency = true);
 		/// Decompresses all files stored in the zip File. Can only be called once per Decompress object.
 		/// Use mapping to retrieve the location of the decompressed files
+		/// if checkConsistency is set to false, archive won't be checked for consistency before decompression
 
 	bool handleZipEntry(std::istream& zipStream, const ZipLocalFileHeader& hdr) override;
 

diff --git a/Zip/include/Poco/Zip/ZipArchive.h b/Zip/include/Poco/Zip/ZipArchive.h
@@ -52,6 +52,9 @@ class Zip_API ZipArchive
 	~ZipArchive();
 		/// Destroys the ZipArchive.
 
+	void checkConsistency();
+		/// Check archive's consistency
+
 	FileInfos::const_iterator fileInfoBegin() const;
 
 	FileInfos::const_iterator fileInfoEnd() const;

diff --git a/Zip/src/Decompress.cpp b/Zip/src/Decompress.cpp
@@ -64,10 +64,14 @@ Decompress::~Decompress()
 }
 
 
-ZipArchive Decompress::decompressAllFiles()
+ZipArchive Decompress::decompressAllFiles(const bool checkConsistency)
 {
 	poco_assert (_mapping.empty());
 	ZipArchive arch(_in, *this);
+
+	if (checkConsistency)
+		arch.checkConsistency();
+
 	return arch;
 }
 

diff --git a/Zip/src/ZipArchive.cpp b/Zip/src/ZipArchive.cpp
@@ -14,6 +14,7 @@
 
 #include "Poco/Zip/ZipArchive.h"
 #include "Poco/Zip/SkipCallback.h"
+#include "Poco/Zip/ZipException.h"
 #include "Poco/Exception.h"
 #include <cstring>
 
@@ -62,6 +63,32 @@ ZipArchive::~ZipArchive()
 }
 
 
+void ZipArchive::checkConsistency()
+{
+	for (const auto& [filename, dirHeader]: _infos)
+	{
+		Poco::UInt32 dirCRC = dirHeader.getCRC();
+		Poco::UInt64 dirUncompressedSize = dirHeader.getUncompressedSize();
+
+		const auto dataIt = _entries.find(filename);
+		if (dataIt != _entries.end())
+		{
+			const ZipLocalFileHeader &dataHeader = dataIt->second;
+
+			Poco::UInt32 dataCRC = dataHeader.getCRC();
+			Poco::UInt64 dataUncompressedSize = dataHeader.getUncompressedSize();
+
+			if (dataCRC != dirCRC)
+				throw ZipException("CRC-32 mismatch: ", filename);
+
+			if (dataUncompressedSize != dirUncompressedSize)
+				throw ZipException("Uncompressed size mismatch: ", filename);
+		}
+	}
+
+}
+
+
 void ZipArchive::parse(std::istream& in, ParseCallback& pc)
 {
 	// read 4 bytes

diff --git a/Zip/testsuite/data/consistency-crc32.zip b/Zip/testsuite/data/consistency-crc32.zip
diff --git a/Zip/testsuite/data/consistency-uncompressed.zip b/Zip/testsuite/data/consistency-uncompressed.zip
diff --git a/Zip/testsuite/src/ZipTest.cpp b/Zip/testsuite/src/ZipTest.cpp
@@ -11,6 +11,7 @@
 #include "ZipTest.h"
 #include "Poco/Zip/SkipCallback.h"
 #include "Poco/Zip/ZipLocalFileHeader.h"
+#include "Poco/Zip/ZipException.h"
 #include "Poco/Zip/ZipArchive.h"
 #include "Poco/Zip/ZipStream.h"
 #include "Poco/Zip/Decompress.h"
@@ -287,6 +288,80 @@ void ZipTest::testDecompressZip64()
 }
 
 
+void ZipTest::testDecompressConsistency()
+{
+	// test archives with borked file headers, but correct directory
+	const std::string testArchives[] =
+	{
+		"consistency-crc32.zip",
+		"consistency-uncompressed.zip"
+	};
+
+
+	for (const std::string &archive : testArchives)
+	{
+		//
+		// test decompressing all files
+		//
+		{
+			Poco::FileInputStream inputStream(getTestFile("data", archive));
+			assertTrue (inputStream.good());
+
+			Decompress dec(inputStream, Poco::Path::temp());
+
+			try {
+				dec.decompressAllFiles();
+				assertTrue (false);
+			}
+			catch (const ZipException &e)
+			{
+			}
+		}
+
+
+		//
+		// test decompressing all files (ignore consistency check)
+		//
+		{
+			Poco::FileInputStream inputStream(getTestFile("data", archive));
+			assertTrue (inputStream.good());
+
+			Decompress dec(inputStream, Poco::Path::temp());
+
+			// should not throw since consistency checks are ignored
+			try {
+				dec.decompressAllFiles(false);
+			}
+			catch (const ZipException &e)
+			{
+				assertTrue (false);
+			}
+		}
+
+
+		//
+		// test decompressing single file
+		//
+		{
+			Poco::FileInputStream inputStream(getTestFile("data", archive));
+			assertTrue (inputStream.good());
+
+			ZipArchive archive(inputStream);
+
+			// File decompression code is skipped since
+			// we are not expecting archive to be processed further
+			try
+			{
+				archive.checkConsistency();
+				assertTrue (false);
+			}
+			catch (const ZipException &e)
+			{
+			}
+		}
+	};
+}
+
 void ZipTest::testValidPath()
 {
 	assertTrue (ZipCommon::isValidPath("."));
@@ -357,6 +432,7 @@ CppUnit::Test* ZipTest::suite()
 	CppUnit_addTest(pSuite, ZipTest, testCrcAndSizeAfterDataEncapsulated);
 	CppUnit_addTest(pSuite, ZipTest, testDecompressZip64);
 	CppUnit_addTest(pSuite, ZipTest, testValidPath);
+	CppUnit_addTest(pSuite, ZipTest, testDecompressConsistency);
 
 	return pSuite;
 }
diff --git a/Zip/testsuite/src/ZipTest.h b/Zip/testsuite/src/ZipTest.h
@@ -29,6 +29,7 @@ class ZipTest: public CppUnit::TestCase
 	void testDecompressSingleFile();
 	void testDecompressSingleFileInDir();
 	void testDecompress();
+	void testDecompressConsistency();
 	void testDecompressFlat();
 	void testDecompressVuln();
 	void testDecompressFlatVuln();