package/inc/ZipFile.hxx | 2
package/source/zipapi/ZipFile.cxx | 181 +++++++++++++++++++++++++++++++-------
2 files changed, 152 insertions(+), 31 deletions(-)
New commits:
commit d274785c93b5c1124af9a0a6c47108fe659b5014
Author: Michael Stahl <michael.st...@allotropia.de>
AuthorDate: Fri Jul 19 20:43:05 2024 +0200
Commit: Adolfo Jayme Barrientos <fit...@ubuntu.com>
CommitDate: Wed Aug 7 01:19:44 2024 +0200
package: ZipFile: check Zip64 end of central directory for consistency
Change-Id: Iae873ec8175922e210398ef8e0f83e148a795c2c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170783
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
Tested-by: Jenkins
(cherry picked from commit ca21cc985d57fffe7c834159b17c095206304994)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/171190
Reviewed-by: Adolfo Jayme Barrientos <fit...@ubuntu.com>
diff --git a/package/inc/ZipFile.hxx b/package/inc/ZipFile.hxx
index 491309d9bd68..65df2ad00a60 100644
--- a/package/inc/ZipFile.hxx
+++ b/package/inc/ZipFile.hxx
@@ -94,7 +94,7 @@ private:
sal_uInt64 readLOC(ZipEntry &rEntry);
sal_Int32 readCEN();
- sal_Int32 findEND();
+ std::tuple<sal_Int64, sal_Int64, sal_Int64> findCentralDirectory();
void HandlePK34(std::span<const sal_Int8> data, sal_Int64 dataOffset,
sal_Int64 totalSize);
void HandlePK78(std::span<const sal_Int8> data, sal_Int64 dataOffset);
void recover();
diff --git a/package/source/zipapi/ZipFile.cxx
b/package/source/zipapi/ZipFile.cxx
index e46d7c561e34..bc886c13771c 100644
--- a/package/source/zipapi/ZipFile.cxx
+++ b/package/source/zipapi/ZipFile.cxx
@@ -1108,18 +1108,19 @@ sal_uInt64 ZipFile::readLOC(ZipEntry &rEntry)
return nEnd;
}
-sal_Int32 ZipFile::findEND()
+std::tuple<sal_Int64, sal_Int64, sal_Int64> ZipFile::findCentralDirectory()
{
// this method is called in constructor only, no need for mutex
- sal_Int32 nPos, nEnd;
Sequence < sal_Int8 > aBuffer;
try
{
- sal_Int32 nLength = static_cast <sal_Int32 > (aGrabber.getLength());
+ sal_Int64 const nLength = aGrabber.getLength();
if (nLength < ENDHDR)
- return -1;
- nPos = nLength - ENDHDR - ZIP_MAXNAMELEN;
- nEnd = nPos >= 0 ? nPos : 0 ;
+ {
+ throw ZipException(u"Zip too small!"_ustr);
+ }
+ sal_Int64 nPos = nLength - ENDHDR - ZIP_MAXNAMELEN;
+ sal_Int64 nEnd = nPos >= 0 ? nPos : 0;
aGrabber.seek( nEnd );
@@ -1129,13 +1130,145 @@ sal_Int32 ZipFile::findEND()
const sal_Int8 *pBuffer = aBuffer.getConstArray();
+ sal_Int64 nEndPos = {};
nPos = nSize - ENDHDR;
while ( nPos >= 0 )
{
if (pBuffer[nPos] == 'P' && pBuffer[nPos+1] == 'K' &&
pBuffer[nPos+2] == 5 && pBuffer[nPos+3] == 6 )
- return nPos + nEnd;
+ {
+ nEndPos = nPos + nEnd;
+ break;
+ }
nPos--;
+ if (nPos == 0)
+ {
+ throw ZipException(u"Zip END signature not found!"_ustr);
+ }
+ }
+
+ aGrabber.seek(nEndPos + 4);
+ sal_uInt16 const nEndDisk = aGrabber.ReadUInt16();
+ if (nEndDisk != 0)
+ { // only single disk is supported!
+ throw ZipException(u"invalid end (disk)"_ustr );
+ }
+ sal_uInt16 const nEndDirDisk = aGrabber.ReadUInt16();
+ if (nEndDirDisk != 0)
+ {
+ throw ZipException(u"invalid end (directory disk)"_ustr );
+ }
+ sal_uInt16 const nEndDiskEntries = aGrabber.ReadUInt16();
+ sal_uInt16 const nEndEntries = aGrabber.ReadUInt16();
+ if (nEndDiskEntries != nEndEntries)
+ {
+ throw ZipException(u"invalid end (entries)"_ustr );
+ }
+ sal_Int32 const nEndDirSize = aGrabber.ReadInt32();
+ sal_Int32 const nEndDirOffset = aGrabber.ReadInt32();
+
+ // Zip64 end of central directory locator must immediately precede
+ // end of central directory record
+ if (20 <= nEndPos)
+ {
+ aGrabber.seek(nEndPos - 20);
+ Sequence<sal_Int8> aZip64EndLocator;
+ aGrabber.readBytes(aZip64EndLocator, 20);
+ MemoryByteGrabber loc64Grabber(aZip64EndLocator);
+ if (loc64Grabber.ReadUInt8() == 'P'
+ && loc64Grabber.ReadUInt8() == 'K'
+ && loc64Grabber.ReadUInt8() == 6
+ && loc64Grabber.ReadUInt8() == 7)
+ {
+ sal_uInt32 const nLoc64Disk = loc64Grabber.ReadUInt32();
+ if (nLoc64Disk != 0)
+ {
+ throw ZipException(u"invalid Zip64 end locator
(disk)"_ustr);
+ }
+ sal_Int64 const nLoc64End64Offset = loc64Grabber.ReadUInt64();
+ if (nEndPos < 20 + 56 || (nEndPos - 20 - 56) <
nLoc64End64Offset
+ || nLoc64End64Offset < 0)
+ {
+ throw ZipException(u"invalid Zip64 end locator
(offset)"_ustr);
+ }
+ sal_uInt32 const nLoc64Disks = loc64Grabber.ReadUInt32();
+ if (nLoc64Disks != 1)
+ {
+ throw ZipException(u"invalid Zip64 end locator (number of
disks)"_ustr);
+ }
+ aGrabber.seek(nLoc64End64Offset);
+ Sequence<sal_Int8> aZip64EndDirectory;
+ aGrabber.readBytes(aZip64EndDirectory, nEndPos - 20 -
nLoc64End64Offset);
+ MemoryByteGrabber end64Grabber(aZip64EndDirectory);
+ if (end64Grabber.ReadUInt8() != 'P'
+ || end64Grabber.ReadUInt8() != 'K'
+ || end64Grabber.ReadUInt8() != 6
+ || end64Grabber.ReadUInt8() != 6)
+ {
+ throw ZipException(u"invalid Zip64 end (signature)"_ustr);
+ }
+ sal_Int64 const nEnd64Size = end64Grabber.ReadUInt64();
+ if (nEnd64Size != nEndPos - 20 - nLoc64End64Offset - 12)
+ {
+ throw ZipException(u"invalid Zip64 end (size)"_ustr);
+ }
+ end64Grabber.ReadUInt16(); // ignore version made by
+ end64Grabber.ReadUInt16(); // ignore version needed to extract
+ sal_uInt32 const nEnd64Disk = end64Grabber.ReadUInt32();
+ if (nEnd64Disk != 0)
+ {
+ throw ZipException(u"invalid Zip64 end (disk)"_ustr);
+ }
+ sal_uInt32 const nEnd64EndDisk = end64Grabber.ReadUInt32();
+ if (nEnd64EndDisk != 0)
+ {
+ throw ZipException(u"invalid Zip64 end (directory
disk)"_ustr);
+ }
+ sal_uInt64 const nEnd64DiskEntries = end64Grabber.ReadUInt64();
+ sal_uInt64 const nEnd64Entries = end64Grabber.ReadUInt64();
+ if (nEnd64DiskEntries != nEnd64Entries)
+ {
+ throw ZipException(u"invalid Zip64 end (entries)"_ustr);
+ }
+ sal_Int64 const nEnd64DirSize = end64Grabber.ReadUInt64();
+ sal_Int64 const nEnd64DirOffset = end64Grabber.ReadUInt64();
+ if (nEndEntries != sal_uInt16(-1) && nEnd64Entries !=
nEndEntries)
+ {
+ throw ZipException(u"inconsistent Zip/Zip64 end
(entries)"_ustr);
+ }
+ if (o3tl::make_unsigned(nEndDirSize) != sal_uInt32(-1)
+ && nEnd64DirSize != nEndDirSize)
+ {
+ throw ZipException(u"inconsistent Zip/Zip64 end
(size)"_ustr);
+ }
+ if (o3tl::make_unsigned(nEndDirOffset) != sal_uInt32(-1)
+ && nEnd64DirOffset != nEndDirOffset)
+ {
+ throw ZipException(u"inconsistent Zip/Zip64 end
(offset)"_ustr);
+ }
+
+ sal_Int64 end;
+ if (o3tl::checked_add<sal_Int64>(nEnd64DirOffset,
nEnd64DirSize, end)
+ || nLoc64End64Offset < end
+ || nEnd64DirOffset < 0
+ || nLoc64End64Offset - nEnd64DirSize != nEnd64DirOffset)
+ {
+ throw ZipException(u"Invalid Zip64 end (bad central
directory size)"_ustr);
+ }
+
+ return { nEnd64Entries, nEnd64DirSize, nEnd64DirOffset };
+ }
}
+
+ sal_Int32 end;
+ if (o3tl::checked_add<sal_Int32>(nEndDirOffset, nEndDirSize, end)
+ || nEndPos < end
+ || nEndDirOffset < 0
+ || nEndPos - nEndDirSize != nEndDirOffset)
+ {
+ throw ZipException(u"Invalid END header (bad central directory
size)"_ustr);
+ }
+
+ return { nEndEntries, nEndDirSize, nEndDirOffset };
}
catch ( IllegalArgumentException& )
{
@@ -1149,41 +1282,30 @@ sal_Int32 ZipFile::findEND()
{
throw ZipException(u"Zip END signature not found!"_ustr );
}
- throw ZipException(u"Zip END signature not found!"_ustr );
}
sal_Int32 ZipFile::readCEN()
{
// this method is called in constructor only, no need for mutex
- sal_Int32 nCenPos = -1, nLocPos;
- sal_uInt16 nCount;
+ sal_Int32 nCenPos = -1;
try
{
- sal_Int32 nEndPos = findEND();
- if (nEndPos == -1)
- return -1;
- aGrabber.seek(nEndPos + ENDTOT);
- sal_uInt16 nTotal = aGrabber.ReadUInt16();
- sal_Int32 nCenLen = aGrabber.ReadInt32();
- sal_Int32 nCenOff = aGrabber.ReadInt32();
-
- if ( nTotal * CENHDR > nCenLen )
- throw ZipException(u"invalid END header (bad entry count)"_ustr );
+ auto [nTotal, nCenLen, nCenOff] = findCentralDirectory();
+ nCenPos = nCenOff; // data before start of zip is not supported
if ( nTotal > ZIP_MAXENTRIES )
throw ZipException(u"too many entries in ZIP File"_ustr );
- if ( nCenLen < 0 || nCenLen > nEndPos )
- throw ZipException(u"Invalid END header (bad central directory
size)"_ustr );
-
- nCenPos = nEndPos - nCenLen;
+ if (nCenLen < nTotal * CENHDR) // prevent overflow with ZIP_MAXENTRIES
+ throw ZipException(u"invalid END header (bad entry count)"_ustr );
- if ( nCenOff < 0 || nCenOff > nCenPos )
- throw ZipException(u"Invalid END header (bad central directory
size)"_ustr );
+ if (SAL_MAX_INT32 < nCenLen)
+ {
+ throw ZipException(u"central directory too big"_ustr);
+ }
- nLocPos = nCenPos - nCenOff;
- aGrabber.seek( nCenPos );
+ aGrabber.seek(nCenPos);
Sequence < sal_Int8 > aCENBuffer ( nCenLen );
sal_Int64 nRead = aGrabber.readBytes ( aCENBuffer, nCenLen );
if ( static_cast < sal_Int64 > ( nCenLen ) != nRead )
@@ -1196,6 +1318,7 @@ sal_Int32 ZipFile::readCEN()
::std::vector<std::pair<sal_uInt64, sal_uInt64>> unallocated = { { 0,
nCenPos } };
aEntries.reserve(nTotal);
+ sal_Int64 nCount;
for (nCount = 0 ; nCount < nTotal; nCount++)
{
sal_Int32 nTestSig = aMemGrabber.ReadInt32();
@@ -1261,8 +1384,6 @@ sal_Int32 ZipFile::readCEN()
aEntry.nSize = nSize;
aEntry.nOffset = nOffset;
- if (o3tl::checked_add<sal_Int64>(aEntry.nOffset, nLocPos,
aEntry.nOffset))
- throw ZipException(u"Integer-overflow"_ustr);
if (o3tl::checked_multiply<sal_Int64>(aEntry.nOffset, -1,
aEntry.nOffset))
throw ZipException(u"Integer-overflow"_ustr);
