From a8bac051c3995f4292c7753e64c8a5e775fccd58 Mon Sep 17 00:00:00 2001
From: nyashbox <nyashbox@tutanota.com>
Date: Fri, 27 Dec 2024 13:46:05 +0200
Subject: [PATCH] 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
---
 Zip/include/Poco/Zip/Decompress.h             |   3 +-
 Zip/include/Poco/Zip/ZipArchive.h             |   3 +
 Zip/src/Decompress.cpp                        |   6 +-
 Zip/src/ZipArchive.cpp                        |  27 +++++++
 Zip/testsuite/data/consistency-crc32.zip      | Bin 0 -> 171 bytes
 .../data/consistency-uncompressed.zip         | Bin 0 -> 171 bytes
 Zip/testsuite/src/ZipTest.cpp                 |  76 ++++++++++++++++++
 Zip/testsuite/src/ZipTest.h                   |   1 +
 8 files changed, 114 insertions(+), 2 deletions(-)
 create mode 100644 Zip/testsuite/data/consistency-crc32.zip
 create mode 100644 Zip/testsuite/data/consistency-uncompressed.zip

diff --git a/Zip/include/Poco/Zip/Decompress.h b/Zip/include/Poco/Zip/Decompress.h
index 83b959c2e..5cf7323b2 100644
--- a/Zip/include/Poco/Zip/Decompress.h
+++ b/Zip/include/Poco/Zip/Decompress.h
@@ -55,9 +55,10 @@ public:
 	~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
index b6446886f..8c194ca6a 100644
--- a/Zip/include/Poco/Zip/ZipArchive.h
+++ b/Zip/include/Poco/Zip/ZipArchive.h
@@ -52,6 +52,9 @@ public:
 	~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
index 605ad7227..7075516e1 100644
--- 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
index 454544b3b..ebc500de7 100644
--- 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
new file mode 100644
index 0000000000000000000000000000000000000000..0872da56292360bdab7fd9cb92bda32992694ee7
GIT binary patch
literal 171
zcmWIWW@h1H00Aw@=Ezk*zzSr8Fb9JSLrP*vqFzZwNoWWs1G7VvU%Dj_msW5yFtWU0
zW?%pliAl*RTmjyUOmfT!gVrqRia;170WzO~5r~&Gf>;P+Ss})v85!Ws$_7%$2!wt>
J+8xAU001Wt9wz_*

literal 0
HcmV?d00001

diff --git a/Zip/testsuite/data/consistency-uncompressed.zip b/Zip/testsuite/data/consistency-uncompressed.zip
new file mode 100644
index 0000000000000000000000000000000000000000..ff034f867dccf4b4b9ed01dc629a85ebd2d9cbd6
GIT binary patch
literal 171
zcmWIWW@h1H00Aw@=EyZmx*}MC>{UR_!63tsl30?cS5i?D8p6rI>=5Ob9t^~#72FJr
zEH9WD7{EkgQgRAcfHxzP95bdtAj3EqB!DCX10xVGX#}wl#<D_;MKdzMo0Scuju8m`
KfV4Y^!vFwGj~;LU

literal 0
HcmV?d00001

diff --git a/Zip/testsuite/src/ZipTest.cpp b/Zip/testsuite/src/ZipTest.cpp
index e16f3da15..61b48570d 100644
--- 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
index cd77baaae..fa4971525 100644
--- a/Zip/testsuite/src/ZipTest.h
+++ b/Zip/testsuite/src/ZipTest.h
@@ -29,6 +29,7 @@ public:
 	void testDecompressSingleFile();
 	void testDecompressSingleFileInDir();
 	void testDecompress();
+	void testDecompressConsistency();
 	void testDecompressFlat();
 	void testDecompressVuln();
 	void testDecompressFlatVuln();