13 ms·

The .zip file specification is flawed

214 points by AndyKelley 10y ago

winteriscoming 10y ago
I haven't taken a look at the spec, but assuming what's noted in that issue comment is indeed the case, then I find it surprising that something as obvious as this would not be fixed/raised during the RFC and review of the spec itself.
- gpcz 10y ago
  There was/is no RFC for the ZIP file format (there is for gzip and the DEFLATE algorithm). It was Phil Katz releasing a text file called APPNOTE.TXT describing the file structure (latest version is available at https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT ).
  - andrewmcwatters 10y ago
    It almost makes .zip feel much more proprietary than it is!
jbb555 10y ago
Presumably this flaw explains why zip files are so obscure and difficult to use. (Yeah it's no great but clearly it's not been an issue)
londons_explore 10y ago
Frakenfiles like this are made by PHP coders who have: Download.php: if (isset($POST['whatever']) write($file_contents); print "download page contents here"
- mschuster91 10y ago
  When you're done flaming PHP, read the whole Github thread, and especially comment #4. The file was produced by a .NET library.
  - TazeTSchnitzel 10y ago
    Even so, it could easily be produced by this kind of error in a PHP script. Including an ending tag (?>) followed by whitespace could have the same effect.
  - Graphon1 10y ago
    Yep, it has nothing to do with PHP. It's easy to produce this kind of frankenfile with an incorrect cp command. Or with incorrect stream management using any zip library that emits streams.
andrewmcwatters 10y ago
Fascinating... I wonder how many .zip files in the wild have this issue and if perhaps as a result many unarchiving applications tolerate it.
- __jal 10y ago
  I exploited this once when designing a puzzle. One clue was "encrypted" text appended to the end of an otherwise-innocent zip file. (One person found it, but the whole thing was way too hard. I drastically overestimated people's operational cleverness.)
  - pavel_lishin 10y ago
    > I drastically overestimated people's operational cleverness. Maybe your target audience was just not quite right? People go gaga over ARGs and devote thousands of hours to 'em. Maybe hint that your game has something to do with HalfLife 3...
    - user5994461 10y ago
      > Maybe your target audience was just not quite right? Nah. It's insanely difficult to find this kind of stuff when you have no clue what to look for.
- diggernet 10y ago
  I'm actually surprised yauzl can get away with rejecting .zip files due to appended data. I recall that back in the BBS days downloaded files frequently had trailing bytes on them (perhaps padding to block size, or something?).
  - mark-r 10y ago
    CP/M didn't record the file size, so all files were a multiple of 128 bytes. The Ctrl-Z convention to mark the end of a text file came from this restriction, and that convention is still used by Windows today.
  - 10y ago
GFK_of_xmaspast 10y ago
See also those tricks you can do with a file that's both a zip and a gif at the same time.
- klodolph 10y ago
  There are a ton of formats you can turn into "both a ZIP and X" at the same time, because most formats have a header at the beginning of the file and ZIP has a header at the end. Self-extracting EXEs are really just some standard EXE which is also a ZIP file, so you can use unzip to extract them without having to install WINE or whatever.
  - xenadu02 10y ago
    Same applies to PDF. I believe it is possible to construct a file that is an EXE, PDF, and ZIP all in one.
- rabidrat 10y ago
  I made a tool "mkizo" that generates .iso files that are also .zip files: https://github.com/century-arcade/src/tree/master/tools/mkizo https://github.com/century-arcade/src/tree/master/tools/mkiz...
Aldo_MX 10y ago
Which is the consensus about better alternatives?
- pkulak 10y ago
  tar.gz ?
  - Aldo_MX 10y ago
    tar.gz is not good with random access
    - duskwuff 10y ago
      "Not good" is an understatement. tar.gz is maximally bad at random access -- there is no way to uncompress a single file, or to list files in the archive, without decompressing the entire gzip stream.
      - Zardoz84 10y ago
        and gzip every file and tar it ? ie gz.tar
        duskwuff 10y ago
        Gzipping each file individually makes the compressor have to start over with a fresh context on each file. This will result in a much worse compression ratio than if a single compressor is used for the whole archive, especially if the archive contains a lot of small, similar files. (Note that this is essentially what PKZIP does, though, so it's not awful.) Also, there are no tools I'm aware of that will handle that archive structure.
        mjevans 10y ago
        While a 'solid' archive wouldn't typically have the compression penalty or the 'listing' penalty it would still have the extraction penalty.
trothamel 10y ago
Another fairly major flaw with zip is a lack of a defined encoding on filenames. As far as I can tell, it's impossible to 100% reliably include non-ASCII filenames inside a zip. (Probably because the format predates unicode's general acceptance.)
- TAForObvReasons 10y ago
  APPNOTE.TXT actually has a comment about it: > D.1 The ZIP format has historically supported only the original IBM PC character encoding set, commonly referred to as IBM Code Page 437. This limits storing file name characters to only those within the original MS-DOS range of values and does not properly support file names in other character encoding, or languages. Extra fields are generally used to implement other code pages. There is a direct way to do it with an extension to the GPBF. Problem is that some writers don't implement the proper flags.
- niftich 10y ago
  The spec says filenames are encoded in CP437. UTF-8 filenames are supported as overlays in two different 'extra headers', which essentially amount to vendor-specified extensions. There are noncompliant implementations that generate zip files with filenames in ISO 8859-1 and other 8-bit codepages, and conversely, there are decompressors that try to accommodate such malformed files.
- Graphon1 10y ago
  > it's impossible to 100% reliably include non-ASCII filenames inside a zip. Yes, the spec does not accommodate this. However there are conventions among popular ZIP creation tools that allow it. If the zip spec were governed by an open body, then this is the kind of thing that would get quickly fixed. (basically standardize the common practice already in use). I don't know why PKWARE never fixed the spec. Maybe because there's no money in it for them, and the pain for the industry is really not that severe.
jplasmeier 10y ago
This strikes me of an example of a sort of dualism that exists within software. The .zip spec is flawed, and can be broken in all sorts of different ways. However, after years and years of using computers, I've never had an issue with extracting or using a .zip file. I wonder if this same pattern can be applied to physical engineering disciplines- e.g. a structural engineer assessing a bridge and finding numerous faults in the design, despite traffic still using the bridge as normal...
- niftich 10y ago
  Most out-in-the-wild usage of zip files fits the 'Robustness Principle', or, after its author, Postel's Law, found in RFC 1122 [1]: > Be liberal in what you accept, and conservative in what you send There are several analyses of this maxim and whether it's a good choice for designing robust, secure systems. This particular Internet Draft doesn't agree [2]. [1] https://tools.ietf.org/html/rfc1122#section-1.2.2 https://tools.ietf.org/html/rfc1122#section-1.2.2 [2] https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00 https://tools.ietf.org/html/draft-thomson-postel-was-wrong-0...
  - user5994461 10y ago
    For user interfaces: Be liberal in what you accept from the user. For M2M (machine to machine): Be strict in what you accept, and strict in what you send. That's my take on it now. For instance, a user will copy/paste URLs to his browser, there will probably be a space too much before or after. It's okay to clean it (and a better experience than a "site not found"). When a machine sends something "weird". Well, it's not possible to know if it's really wrong and it can't be corrected in any meaningful way. Just fail and throw an error so the developer can fix it.
    - wolrah 10y ago
      I agree with those positions, but what about the middle ground? Browsers for example, taking machine-to-machine data which sometimes is human generated. I'm a fan of the strict solution as far as that goes, but there's a reason XHTML failed. Somehow asking people who write web sites to do it right or else it doesn't work is a big deal.
xenadu02 10y ago
Why can't this algorithm work reliably: 1. Begin linear search backwards from end of file 2. If magic constant found, push onto stack of possible locations 3. Once 32k has been reached stop searching 4. For each location on the stack, validate the rest of the header and that the remaining comment length matches the recorded length. 5. The first location to validate as a correct header is chosen This should account for the appearance of the magic header in comments or data preceding the end of directory entry. Have I missed something?
- diggernet 10y ago
  That should work reasonably reliably, and is essentially what TFA describes as the behavior of 'unzip'. But it then goes on to say, "The zip file spec does not allow this arbitrary data [at the end of a zipfile], so unzip is non-conformantly tolerating this data." So I think what you missed is pedantry. :)
- ambrop7 10y ago
  Yes you have missed that a valid header may appear by chance somewhere near the end of main zip data. Because your algorithm will prefer picking up headers toward the front, it might misinterpret a valid file, even one without a magic number in the comment; if the last 64k-(actual header+comment size) happens to contain an apparently valid header.
  - j_s 10y ago
    This happens with nested .ZIPs.
- grkvlt 10y ago
  Yes. Step 4, specifically "the remaining comment length matches the recorded length" will fail in the presence of appended data, as there is no end of comment or end of record marker. This is exactly how the file failed in the article, it used that algorithm and found zero valid headers.
arh68 10y ago
I don't understand why this program should fail, though, if other tools can unzip the file just fine. I specifically wonder about this part of the code [0]: if (commentLength !== expectedCommentLength) { return callback(new Error("invalid comment length. expected: " + expectedCommentLength + ". found: " + commentLength)); } Why not just determine this isn't the record, keep looking, and handle the error after the loop? if (commentLength !== expectedCommentLength) { continue } [0] https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L124-L126 https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L...
- bonzini 10y ago
  This would not fix the problem. The issue is that there is extra content after the comment (for what we know there could be no comment at all, actually). The library expected the comment to end right at the end of the file.
- x1798DE 10y ago
  I think the argument would be that if you write a non-standards-compliant unzip utility, in many cases people generating malformed zip files won't get any warning because the software "knows what you meant to do", then people emitting malformed zip files with a random amount of data at the end won't know that they're doing anything wrong until one day they hit the limit of what is acceptable, way down the road and after they've been emitting malformed zip files for ages. I think it's the programming equivalent of not telling someone they have some food on their face - yeah, it's no skin off your back if you don't tell them, but who knows who they're going to run into later. That said, a hybrid approach which fails by default but then offers a way to "force unzip" the file in a non-standards-compliant way might be the best of both worlds here, because the users get an immediate way to fix the problem and the emitters are likely to be notified that they are creating malformed zips.
  - TAForObvReasons 10y ago
    The problem is that the ZIP format is not a full-file specification: it permits data before and after the logical extents of the ZIP archive. Among other things, it allows for self-extracting archives by prepending the extractor code. the library in this case is erroneously assuming that the archive spans the entire file.
BentFranklin 10y ago
There is an issue of POC|GTFO that discusses zip frankenfiles.
Meic 10y ago
See also zip bombs [0] [0] https://en.m.wikipedia.org/wiki/Zip_bomb https://en.m.wikipedia.org/wiki/Zip_bomb
ztravis 10y ago
Having browsed through a few different zip parsers in the course of writing my own, most handle a range of malformed/modified input, including prepended/appended garbage, filename encodings (CP437, Unicode "extra field" flags), etc. I found this set of test files useful: https://github.com/Stuk/jszip/tree/master/test/ref https://github.com/Stuk/jszip/tree/master/test/ref It is annoying that support for these non-standard zips has become de-facto standard (you don't want your tool to fail where others succeed)...
optionalparens 10y ago
Most specs I encounter have seriously flawed implementations in the wild. This is regardless of the quality of the spec. Here's a short-list off the top of my head of tech with specs and horrific offenders in the wild, including many popular implementors. * Telnet * VT-XXX, i.e. VT-100 or any term spec for that matter * MIDI * MKV, AVI, etc. * GIF, JPEG * A huge amount of tech in any browser * SAUCE * ANSI escape parsers/writers/sequences * LZH/LHA * Open Document Format * XMP * MAPI * POP, IMAP As I think of this, I realize I'll be typing all night. In some cases, the implementors all adopt things from the most popular implementors even if they are historical. In other cases, things just don't work or break in awesome ways. Zip bombs were mentioned, but there are plenty of awesome things. I'll finish by saying I wonder if many authors of anything related to terminals even ever bother to read specs or care. Over the years I've started to wonder how some of these things even work in the wild, but the answer is most people don't notice what actually isn't working, while anyone who needs to care has to draw from what become unofficial new specs.
Dylan16807 10y ago
So the flaw in the spec is extremely minor and irrelevant to the real complaint. The real complaint is about programs making and accepting slightly invalid files.
kazinator 10y ago
What the article doesn't cover is more braindamage in the ZIP spec which prevents a forward scan through the individual file records. This is detailed in the Wikipedia. Tools that correctly read .ZIP archives must scan for the end of central directory record signature, and then, as appropriate, the other, indicated, central directory records. They must not scan for entries from the top of the ZIP file, because only the central directory specifies where a file chunk starts. Scanning could lead to false positives, as the format does not forbid other data to be between chunks, nor file data streams from containing such signatures.
- 762236 10y ago
  I wonder who here remembers self-extracting zip files under DOS/Windows? They put the zip file into the executable file. And you could extract with a normal unzip program, I believe.
  - flukus 10y ago
    You're probably thinking if NSIS (http://nsis.sourceforge.net/Main_Page http://nsis.sourceforge.net/Main_Page) and a few others.
  - khedoros1 10y ago
    I still run into those a couple times a year. Admittedly, mostly when I'm going on some kind of retro binge.
twr 10y ago
I almost wrote a zip parser. After studying the format a bit, I gave up that idea. - A valid zip does not begin with, as you would normally expect, a magic number. - A valid zip can contain arbitrary prepended data. - A valid zip can contain sections with no identifier value. - A valid zip can contain arbitrary data between sections. - A valid zip is validated starting with a tail section located at the end. - A valid zip can have a valid tail section that contains a valid tail section. - A valid zip can contain arbitrary appended data. I don't get it: Why design a format that's so hard to parse? Implementing a single-pass streaming parser is impossible. It should be a basic requirement for most file formats. /usr/bin/unzip cannot even extract from standard input. I'm sure the implementer didn't feel like receiving user complaints about exhausted memory.
- badsectoracula 10y ago
  Because the format was originally written by PKWARE for their PKZIP and PKUNZIP programs for DOS in the 80s and used for backup purposes, among others (being able to span multiple floppy disks was an often used feature). To write the directory at the beginning they'd need to keep all the data in memory or do some extra postprocessing that would make it prohibitively slow on a 4.7MHz machine with a 20MD hard disk and a slow 360K floppy disk drive. So the directory goes at the end. The files do not begin with a magic number because ZIP files can be embedded in other files - most commonly executable files for the self-extracting feature of PKZIP that placed the ZIP file right after the decompressor. This allowed the executable to be used both by itself and by PKZIP. As for the data after the magic number, it was used for the comment which was most likely added at a later point and they decided to put it after the magic number so that it remains compatible with existing decompressors. In the 80s and early 90s people didn't had Internet to get the latest version and a lot of old versions of PKUNZIP were floating around for many years.
  - twr 10y ago
    That is an excellent explanation, thank you.
  - dkonofalski 10y ago
    To further add on to that, I would frequently run into programs (and sometimes games) that would span multiple floppies where almost the entire first part of the archive was just installer data. Sometimes, the actual .zip file data would start on the tail end of the first disk and then span however many disks were needed while other times the first disk was just the installer and then the .zip data was on the other disks. In either case, PKZIP and PKUNZIP would be able to read that file but only in the 2nd case would they be able to read the file without that first disk. Things got further complicated when RARs made the scene because I remember that some of the early .RAR apps offered their own implementations of .zip support that would create multi-span .zips where the installer data was on the first disk, the .zip data was on the other disks, but the container spanned all the disks. This meant that you could potentially have 3 different ways of dealing with the same data and there was no safe way to assume which method was used: 1. Installer on disk 1, multi-spanned .zip on disks 2-n 2. Multi-span .zip on disks 1-n 3. Multi-span .zip on disks 1-n but installer data only on disk 1

214 points by AndyKelley 10y ago

I haven't taken a look at the spec, but assuming what's noted in that issue comment is indeed the case, then I find it surprising that something as obvious as this would not be fixed/raised during the RFC and review of the spec itself.

gpcz 10y ago
There was/is no RFC for the ZIP file format (there is for gzip and the DEFLATE algorithm). It was Phil Katz releasing a text file called APPNOTE.TXT describing the file structure (latest version is available at https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT ).
- andrewmcwatters 10y ago
  It almost makes .zip feel much more proprietary than it is!

jbb555 10y ago

Presumably this flaw explains why zip files are so obscure and difficult to use. (Yeah it's no great but clearly it's not been an issue)

londons_explore 10y ago

Frakenfiles like this are made by PHP coders who have: Download.php: if (isset($POST['whatever']) write($file_contents); print "download page contents here"

mschuster91 10y ago
When you're done flaming PHP, read the whole Github thread, and especially comment #4. The file was produced by a .NET library.
- TazeTSchnitzel 10y ago
  Even so, it could easily be produced by this kind of error in a PHP script. Including an ending tag (?>) followed by whitespace could have the same effect.
- Graphon1 10y ago
  Yep, it has nothing to do with PHP. It's easy to produce this kind of frankenfile with an incorrect cp command. Or with incorrect stream management using any zip library that emits streams.

andrewmcwatters 10y ago

Fascinating... I wonder how many .zip files in the wild have this issue and if perhaps as a result many unarchiving applications tolerate it.

__jal 10y ago
I exploited this once when designing a puzzle. One clue was "encrypted" text appended to the end of an otherwise-innocent zip file. (One person found it, but the whole thing was way too hard. I drastically overestimated people's operational cleverness.)
- pavel_lishin 10y ago
  > I drastically overestimated people's operational cleverness. Maybe your target audience was just not quite right? People go gaga over ARGs and devote thousands of hours to 'em. Maybe hint that your game has something to do with HalfLife 3...
  - user5994461 10y ago
    > Maybe your target audience was just not quite right? Nah. It's insanely difficult to find this kind of stuff when you have no clue what to look for.
diggernet 10y ago
I'm actually surprised yauzl can get away with rejecting .zip files due to appended data. I recall that back in the BBS days downloaded files frequently had trailing bytes on them (perhaps padding to block size, or something?).
- mark-r 10y ago
  CP/M didn't record the file size, so all files were a multiple of 128 bytes. The Ctrl-Z convention to mark the end of a text file came from this restriction, and that convention is still used by Windows today.
- 10y ago

GFK_of_xmaspast 10y ago

See also those tricks you can do with a file that's both a zip and a gif at the same time.

klodolph 10y ago
There are a ton of formats you can turn into "both a ZIP and X" at the same time, because most formats have a header at the beginning of the file and ZIP has a header at the end. Self-extracting EXEs are really just some standard EXE which is also a ZIP file, so you can use unzip to extract them without having to install WINE or whatever.
- xenadu02 10y ago
  Same applies to PDF. I believe it is possible to construct a file that is an EXE, PDF, and ZIP all in one.
rabidrat 10y ago
I made a tool "mkizo" that generates .iso files that are also .zip files: https://github.com/century-arcade/src/tree/master/tools/mkizo https://github.com/century-arcade/src/tree/master/tools/mkiz...

Aldo_MX 10y ago

Which is the consensus about better alternatives?

pkulak 10y ago
tar.gz ?
- Aldo_MX 10y ago
  tar.gz is not good with random access
  - duskwuff 10y ago
    "Not good" is an understatement. tar.gz is maximally bad at random access -- there is no way to uncompress a single file, or to list files in the archive, without decompressing the entire gzip stream.
    - Zardoz84 10y ago
      and gzip every file and tar it ? ie gz.tar
      - duskwuff 10y ago
        Gzipping each file individually makes the compressor have to start over with a fresh context on each file. This will result in a much worse compression ratio than if a single compressor is used for the whole archive, especially if the archive contains a lot of small, similar files. (Note that this is essentially what PKZIP does, though, so it's not awful.) Also, there are no tools I'm aware of that will handle that archive structure.
        mjevans 10y ago
        While a 'solid' archive wouldn't typically have the compression penalty or the 'listing' penalty it would still have the extraction penalty.

trothamel 10y ago

Another fairly major flaw with zip is a lack of a defined encoding on filenames. As far as I can tell, it's impossible to 100% reliably include non-ASCII filenames inside a zip. (Probably because the format predates unicode's general acceptance.)

TAForObvReasons 10y ago
APPNOTE.TXT actually has a comment about it: > D.1 The ZIP format has historically supported only the original IBM PC character encoding set, commonly referred to as IBM Code Page 437. This limits storing file name characters to only those within the original MS-DOS range of values and does not properly support file names in other character encoding, or languages. Extra fields are generally used to implement other code pages. There is a direct way to do it with an extension to the GPBF. Problem is that some writers don't implement the proper flags.
niftich 10y ago
The spec says filenames are encoded in CP437. UTF-8 filenames are supported as overlays in two different 'extra headers', which essentially amount to vendor-specified extensions. There are noncompliant implementations that generate zip files with filenames in ISO 8859-1 and other 8-bit codepages, and conversely, there are decompressors that try to accommodate such malformed files.
Graphon1 10y ago
> it's impossible to 100% reliably include non-ASCII filenames inside a zip. Yes, the spec does not accommodate this. However there are conventions among popular ZIP creation tools that allow it. If the zip spec were governed by an open body, then this is the kind of thing that would get quickly fixed. (basically standardize the common practice already in use). I don't know why PKWARE never fixed the spec. Maybe because there's no money in it for them, and the pain for the industry is really not that severe.

jplasmeier 10y ago

This strikes me of an example of a sort of dualism that exists within software. The .zip spec is flawed, and can be broken in all sorts of different ways. However, after years and years of using computers, I've never had an issue with extracting or using a .zip file. I wonder if this same pattern can be applied to physical engineering disciplines- e.g. a structural engineer assessing a bridge and finding numerous faults in the design, despite traffic still using the bridge as normal...

niftich 10y ago
Most out-in-the-wild usage of zip files fits the 'Robustness Principle', or, after its author, Postel's Law, found in RFC 1122 [1]: > Be liberal in what you accept, and conservative in what you send There are several analyses of this maxim and whether it's a good choice for designing robust, secure systems. This particular Internet Draft doesn't agree [2]. [1] https://tools.ietf.org/html/rfc1122#section-1.2.2 https://tools.ietf.org/html/rfc1122#section-1.2.2 [2] https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00 https://tools.ietf.org/html/draft-thomson-postel-was-wrong-0...
- user5994461 10y ago
  For user interfaces: Be liberal in what you accept from the user. For M2M (machine to machine): Be strict in what you accept, and strict in what you send. That's my take on it now. For instance, a user will copy/paste URLs to his browser, there will probably be a space too much before or after. It's okay to clean it (and a better experience than a "site not found"). When a machine sends something "weird". Well, it's not possible to know if it's really wrong and it can't be corrected in any meaningful way. Just fail and throw an error so the developer can fix it.
  - wolrah 10y ago
    I agree with those positions, but what about the middle ground? Browsers for example, taking machine-to-machine data which sometimes is human generated. I'm a fan of the strict solution as far as that goes, but there's a reason XHTML failed. Somehow asking people who write web sites to do it right or else it doesn't work is a big deal.

xenadu02 10y ago

Why can't this algorithm work reliably: 1. Begin linear search backwards from end of file 2. If magic constant found, push onto stack of possible locations 3. Once 32k has been reached stop searching 4. For each location on the stack, validate the rest of the header and that the remaining comment length matches the recorded length. 5. The first location to validate as a correct header is chosen This should account for the appearance of the magic header in comments or data preceding the end of directory entry. Have I missed something?

diggernet 10y ago
That should work reasonably reliably, and is essentially what TFA describes as the behavior of 'unzip'. But it then goes on to say, "The zip file spec does not allow this arbitrary data [at the end of a zipfile], so unzip is non-conformantly tolerating this data." So I think what you missed is pedantry. :)
ambrop7 10y ago
Yes you have missed that a valid header may appear by chance somewhere near the end of main zip data. Because your algorithm will prefer picking up headers toward the front, it might misinterpret a valid file, even one without a magic number in the comment; if the last 64k-(actual header+comment size) happens to contain an apparently valid header.
- j_s 10y ago
  This happens with nested .ZIPs.
grkvlt 10y ago
Yes. Step 4, specifically "the remaining comment length matches the recorded length" will fail in the presence of appended data, as there is no end of comment or end of record marker. This is exactly how the file failed in the article, it used that algorithm and found zero valid headers.

arh68 10y ago

I don't understand why this program should fail, though, if other tools can unzip the file just fine. I specifically wonder about this part of the code [0]: if (commentLength !== expectedCommentLength) { return callback(new Error("invalid comment length. expected: " + expectedCommentLength + ". found: " + commentLength)); } Why not just determine this isn't the record, keep looking, and handle the error after the loop? if (commentLength !== expectedCommentLength) { continue } [0] https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L124-L126 https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L...

bonzini 10y ago
This would not fix the problem. The issue is that there is extra content after the comment (for what we know there could be no comment at all, actually). The library expected the comment to end right at the end of the file.
x1798DE 10y ago
I think the argument would be that if you write a non-standards-compliant unzip utility, in many cases people generating malformed zip files won't get any warning because the software "knows what you meant to do", then people emitting malformed zip files with a random amount of data at the end won't know that they're doing anything wrong until one day they hit the limit of what is acceptable, way down the road and after they've been emitting malformed zip files for ages. I think it's the programming equivalent of not telling someone they have some food on their face - yeah, it's no skin off your back if you don't tell them, but who knows who they're going to run into later. That said, a hybrid approach which fails by default but then offers a way to "force unzip" the file in a non-standards-compliant way might be the best of both worlds here, because the users get an immediate way to fix the problem and the emitters are likely to be notified that they are creating malformed zips.
- TAForObvReasons 10y ago
  The problem is that the ZIP format is not a full-file specification: it permits data before and after the logical extents of the ZIP archive. Among other things, it allows for self-extracting archives by prepending the extractor code. the library in this case is erroneously assuming that the archive spans the entire file.

BentFranklin 10y ago

There is an issue of POC|GTFO that discusses zip frankenfiles.

Meic 10y ago

See also zip bombs [0] [0] https://en.m.wikipedia.org/wiki/Zip_bomb https://en.m.wikipedia.org/wiki/Zip_bomb

ztravis 10y ago

Having browsed through a few different zip parsers in the course of writing my own, most handle a range of malformed/modified input, including prepended/appended garbage, filename encodings (CP437, Unicode "extra field" flags), etc. I found this set of test files useful: https://github.com/Stuk/jszip/tree/master/test/ref https://github.com/Stuk/jszip/tree/master/test/ref It is annoying that support for these non-standard zips has become de-facto standard (you don't want your tool to fail where others succeed)...

optionalparens 10y ago

Most specs I encounter have seriously flawed implementations in the wild. This is regardless of the quality of the spec. Here's a short-list off the top of my head of tech with specs and horrific offenders in the wild, including many popular implementors. * Telnet * VT-XXX, i.e. VT-100 or any term spec for that matter * MIDI * MKV, AVI, etc. * GIF, JPEG * A huge amount of tech in any browser * SAUCE * ANSI escape parsers/writers/sequences * LZH/LHA * Open Document Format * XMP * MAPI * POP, IMAP As I think of this, I realize I'll be typing all night. In some cases, the implementors all adopt things from the most popular implementors even if they are historical. In other cases, things just don't work or break in awesome ways. Zip bombs were mentioned, but there are plenty of awesome things. I'll finish by saying I wonder if many authors of anything related to terminals even ever bother to read specs or care. Over the years I've started to wonder how some of these things even work in the wild, but the answer is most people don't notice what actually isn't working, while anyone who needs to care has to draw from what become unofficial new specs.

Dylan16807 10y ago

So the flaw in the spec is extremely minor and irrelevant to the real complaint. The real complaint is about programs making and accepting slightly invalid files.

kazinator 10y ago

What the article doesn't cover is more braindamage in the ZIP spec which prevents a forward scan through the individual file records. This is detailed in the Wikipedia. Tools that correctly read .ZIP archives must scan for the end of central directory record signature, and then, as appropriate, the other, indicated, central directory records. They must not scan for entries from the top of the ZIP file, because only the central directory specifies where a file chunk starts. Scanning could lead to false positives, as the format does not forbid other data to be between chunks, nor file data streams from containing such signatures.

762236 10y ago
I wonder who here remembers self-extracting zip files under DOS/Windows? They put the zip file into the executable file. And you could extract with a normal unzip program, I believe.
- flukus 10y ago
  You're probably thinking if NSIS (http://nsis.sourceforge.net/Main_Page http://nsis.sourceforge.net/Main_Page) and a few others.
- khedoros1 10y ago
  I still run into those a couple times a year. Admittedly, mostly when I'm going on some kind of retro binge.

twr 10y ago

I almost wrote a zip parser. After studying the format a bit, I gave up that idea. - A valid zip does not begin with, as you would normally expect, a magic number. - A valid zip can contain arbitrary prepended data. - A valid zip can contain sections with no identifier value. - A valid zip can contain arbitrary data between sections. - A valid zip is validated starting with a tail section located at the end. - A valid zip can have a valid tail section that contains a valid tail section. - A valid zip can contain arbitrary appended data. I don't get it: Why design a format that's so hard to parse? Implementing a single-pass streaming parser is impossible. It should be a basic requirement for most file formats. /usr/bin/unzip cannot even extract from standard input. I'm sure the implementer didn't feel like receiving user complaints about exhausted memory.

badsectoracula 10y ago
Because the format was originally written by PKWARE for their PKZIP and PKUNZIP programs for DOS in the 80s and used for backup purposes, among others (being able to span multiple floppy disks was an often used feature). To write the directory at the beginning they'd need to keep all the data in memory or do some extra postprocessing that would make it prohibitively slow on a 4.7MHz machine with a 20MD hard disk and a slow 360K floppy disk drive. So the directory goes at the end. The files do not begin with a magic number because ZIP files can be embedded in other files - most commonly executable files for the self-extracting feature of PKZIP that placed the ZIP file right after the decompressor. This allowed the executable to be used both by itself and by PKZIP. As for the data after the magic number, it was used for the comment which was most likely added at a later point and they decided to put it after the magic number so that it remains compatible with existing decompressors. In the 80s and early 90s people didn't had Internet to get the latest version and a lot of old versions of PKUNZIP were floating around for many years.
- twr 10y ago
  That is an excellent explanation, thank you.
- dkonofalski 10y ago
  To further add on to that, I would frequently run into programs (and sometimes games) that would span multiple floppies where almost the entire first part of the archive was just installer data. Sometimes, the actual .zip file data would start on the tail end of the first disk and then span however many disks were needed while other times the first disk was just the installer and then the .zip data was on the other disks. In either case, PKZIP and PKUNZIP would be able to read that file but only in the 2nd case would they be able to read the file without that first disk. Things got further complicated when RARs made the scene because I remember that some of the early .RAR apps offered their own implementations of .zip support that would create multi-span .zips where the installer data was on the first disk, the .zip data was on the other disks, but the container spanned all the disks. This meant that you could potentially have 3 different ways of dealing with the same data and there was no safe way to assume which method was used: 1. Installer on disk 1, multi-spanned .zip on disks 2-n 2. Multi-span .zip on disks 1-n 3. Multi-span .zip on disks 1-n but installer data only on disk 1