Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue 20027 - std.zip susceptible to zip malware attacks #7223

Merged
merged 1 commit into from Oct 10, 2019
Merged

Fix issue 20027 - std.zip susceptible to zip malware attacks #7223

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 62 additions & 5 deletions std/zip.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,27 +599,35 @@ public:

this(void[] buffer)
{
uint directorySize;
uint directoryOffset;

this._data = cast(ubyte[]) buffer;

if (data.length > uint.max - 2)
throw new ZipException("zip files bigger than 4 GB are unsupported");

_segs = [Segment(0, cast(uint) data.length)];

findEndOfCentralDirRecord();
uint i = endrecOffset;

int endcommentlength = getUshort(i + 20);
comment = cast(string)(_data[i + 22 .. i + 22 + endcommentlength]);
int endCommentLength = getUshort(i + 20);
comment = cast(string)(_data[i + 22 .. i + 22 + endCommentLength]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have been good to put style fixes in their own commit.


// end of central dir record
removeSegment(endrecOffset, endrecOffset + endOfCentralDirLength + endCommentLength);

uint k = i - zip64EndOfCentralDirLocatorLength;
if (k < i && _data[k .. k + 4] == cast(ubyte[])"PK\x06\x07")
{
_isZip64 = true;
i = k;

// zip64 end of central dir record locator
removeSegment(k, k + zip64EndOfCentralDirLocatorLength);
}

uint directorySize;
uint directoryOffset;

if (isZip64)
{
// Read Zip64 record data
Expand All @@ -635,6 +643,9 @@ public:
if (eocd64Size + i - 12 > data.length)
throw new ZipException("invalid Zip EOCD64 size");

// zip64 end of central dir record
removeSegment(i, cast(uint) (i + 12 + eocd64Size));

_diskNumber = getUint(i + 16);
_diskStartDir = getUint(i + 20);

Expand Down Expand Up @@ -711,6 +722,10 @@ public:
de.internalAttributes = getUshort(i + 36);
de._externalAttributes = getUint(i + 38);
de.offset = getUint(i + 42);

// central directory header
removeSegment(i, i + centralFileHeaderLength + namelen + extralen + commentlen);

i += centralFileHeaderLength;

if (i + namelen + extralen + commentlen > directoryOffset + directorySize)
Expand All @@ -726,6 +741,10 @@ public:
auto localFileHeaderNamelen = getUshort(de.offset + 26);
auto localFileHeaderExtralen = getUshort(de.offset + 28);

// file data
removeSegment(de.offset, de.offset + localFileHeaderLength + localFileHeaderNamelen
+ localFileHeaderExtralen + de._compressedSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping all of these can be later refactored away to avoid the DRY of calculating all these lengths all over the place. The code is pretty difficult to follow as it is. Maybe once the structures are represented as D structs, the removeSegment call can be done by whatever function will be doing the reading of the entire structs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my code to read a Windows PE file's headers:

https://github.com/CyberShadow/ae/blob/218c29e17f6ecfc49f696b92c879c5c6b9917f2e/sys/windows/pe/pe.d#L31-L51

I'm hoping std.zip could closer to that.


immutable uint dataOffset = de.offset + localFileHeaderLength
+ localFileHeaderNamelen + localFileHeaderExtralen;
if (dataOffset + de.compressedSize > endrecOffset)
Expand Down Expand Up @@ -1174,6 +1193,44 @@ the quick brown fox jumps over the lazy dog\r
assert(za.directory["file"].compressedData == [104, 101, 108, 108, 111]);
}

// issue #20027
@system unittest
{
// central file header overlaps end of central directory
auto file =
// lfh
"\x50\x4b\x03\x04\x0a\x00\x00\x00\x00\x00\x8f\x72\x4a\x4f\x86\xa6"~
"\x10\x36\x05\x00\x00\x00\x05\x00\x00\x00\x04\x00\x1c\x00\x66\x69"~
"\x6c\x65\x55\x54\x09\x00\x03\x0d\x22\x9f\x5d\x12\x22\x9f\x5d\x75"~
"\x78\x0b\x00\x01\x04\xf0\x03\x00\x00\x04\xf0\x03\x00\x00\x68\x65"~
"\x6c\x6c\x6f\x50\x4b\x01\x02\x1e\x03\x0a\x00\x00\x00\x00\x00\x8f"~
"\x72\x4a\x4f\x86\xa6\x10\x36\x05\x00\x00\x00\x05\x00\x00\x00\x04"~
"\x00\x18\x00\x04\x00\x00\x00\x01\x00\x00\x00\xb0\x81\x00\x00\x00"~
"\x00\x66\x69\x6c\x65\x55\x54\x05\x00\x03\x0d\x22\x9f\x5d\x75\x78"~
"\x0b\x00\x01\x04\xf0\x03\x00\x00\x04\xf0\x03\x00\x00\x50\x4b\x05"~
"\x06\x00\x00\x00\x00\x01\x00\x01\x00\x4a\x00\x00\x00\x43\x00\x00"~
"\x00\x00\x00";

import std.exception : assertThrown;
assertThrown!ZipException(new ZipArchive(cast(void[]) file));

// local file header and file data overlap second local file header and file data
file =
"\x50\x4b\x03\x04\x0a\x00\x00\x00\x00\x00\x8f\x72\x4a\x4f\x86\xa6"~
"\x10\x36\x05\x00\x00\x00\x05\x00\x00\x00\x04\x00\x1e\x00\x66\x69"~
"\x6c\x65\x55\x54\x09\x00\x03\x0d\x22\x9f\x5d\x12\x22\x9f\x5d\x75"~
"\x78\x0b\x00\x01\x04\xf0\x03\x00\x00\x04\xf0\x03\x00\x00\x68\x65"~
"\x6c\x6c\x6f\x50\x4b\x01\x02\x1e\x03\x0a\x00\x00\x00\x00\x00\x8f"~
"\x72\x4a\x4f\x86\xa6\x10\x36\x05\x00\x00\x00\x05\x00\x00\x00\x04"~
"\x00\x18\x00\x04\x00\x00\x00\x01\x00\x00\x00\xb0\x81\x00\x00\x00"~
"\x00\x66\x69\x6c\x65\x55\x54\x05\x00\x03\x0d\x22\x9f\x5d\x75\x78"~
"\x0b\x00\x01\x04\xf0\x03\x00\x00\x04\xf0\x03\x00\x00\x50\x4b\x05"~
"\x06\x00\x00\x00\x00\x01\x00\x01\x00\x4a\x00\x00\x00\x43\x00\x00"~
"\x00\x00\x00";

assertThrown!ZipException(new ZipArchive(cast(void[]) file));
}

// Non-Android Posix-only, because we can't rely on the unzip command being
// available on Android or Windows
version (Android) {} else
Expand Down