Fix issue 20027 - std.zip susceptible to zip malware attacks #7223
Conversation
ghost
requested a review
from 
|
Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#7223" |
| 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]); |
There was a problem hiding this comment.
It would have been good to put style fixes in their own commit.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here is my code to read a Windows PE file's headers:
I'm hoping std.zip could closer to that.
This fixes the main problem of 20027, namely rejecting zip bombs. I moved all remaining problems to own issues (20290 - 20294). Therefore this issue can be closed with this PR.
There is one unittest missing, namely checking for overlaps of zip64 central dir records with other stuff. This unittests needs to be postponed due to a bug in the implementation of zip64 (see issue 20289).
I verified, that it indeed rejects the zip bombs zbsm.zip and zblg.zip mentioned in the issues. The zip64 zip bomb zbxl.zip does not work yet due to bugs in the zip64 implementation, probably introduced by me in PR #7198. I'll fix this as soon as possible.
I also checked some normal zip-files to make sure that the size of _segs is always O(1).