(Reworked) Add ability to limit output size for zip archives (against zip bombs)

[serejke]

Hello,
@satamas asked me to complete the pull-request #115, which had a couple of defects.
Actually, I decided to rewrite the implementation, so this new pull request deserves a separate review and #115 will be dissmissed.

What have been changed:

1. Pull request Add ability to limit output size for zip unarchiver #115 was rebased onto master after "try-with-resources" Refactor: use try with resources pattern #116 commit.
2. I got rid of changes in AbstractUnArchiver
3. I got rid of fragile call to File.size and replaced it with CountingInputStream.
4. Removed duplication of AbstractZipUnArchiver#execute() and AbstractZipUnArchiver#execute(String, File) by delegating to the latter.

I tried to minimize diff of the files and keep code style. Let me know if something was missed.

[plamentotev]

@serejke thanks. I'll take a look at the changes in the coming days. But there is one thing that I've noticed. Does it make sense to move the change to AbstractUnArchiver.java#extractFile (without changing the signature)? This way all un-archivers would benefit (not only ZIP). To me it looks like similar vulnerabilities are not limited only to ZIP but to be honest I didn't looked too much into the matter so I could be wrong.

[serejke]

@plamentotev for sure, other archivers are vulnerable to bombs as well.
But since each archiver has its own implementation of UnArchiver#extract(), we can't protect them all by a check in a single place.

The suggestion to move the change to AbstractUnArchiver#extractFile does not work well:

1. Since we don't change signature of extractFile, the only mean to pass maxOutputSize is to move this field upward to AbstractUnArchiver and decrement it after each call to extractFile. But then AbstractUnArchiver will get an internal state between calls, which doesn't look like a good design.
2. extractFile method is not called by some implementations of AbstractUnArchiver (BZip2UnArchiver, GZipUnArchiver etc), since they implement the extract method by calling copyFully from decoded streams and do not check size limits.

So the only good way to protect all archivers seems to be:

1. Add setMaxOutputSize() method to UnArchiver
2. Reimplement all archivers' extract methods by making them check the remaining size.
   It requires more changes and apparently should be made in a separate pull request.

[serejke]

I believe this pull request can be applied separately.
If we later decide to protect all archivers from zip bombs, we will only need to move upward the setMaxOutputSize, which is a compatible API change.

[plamentotev]

Sorry for the delay, I was busy. I agree with you. We can merge it for Zip archives and implement it later for the rest later and keep the backward compatibility.

Would you please split this in 3 commits:

1. Remove the redundant call to extractFileIfIncluded
2. Remove the code duplication
3. Implement the zip bombs protection

With all changes in single commit is a bit hard to track what is changed. Also there is some code formatting issues - the resolveSymlink formatting is changed although it was correct and for some of the method calls there is no spaces around the braces.

Apart from that I have single question - why the exception is thrown when remainingSpace <= 0. Correct me if I wrong but I think using the remaining space you can't distinguish (in all cases) between when the limit is reached ( BoundedInputStream returned EOF) and when the archive fits exactly the limit.

[plamentotev]

I've played around with the test. And your test file is 9 bytes on the file system, but the limit in the test is set to 10 and the archive somehow exceeds it (the counting input stream returns 11). Do you have an idea why does this happens?

[plamentotev]

Please disregard my last comment. The file is actually 11 bytes long. My bad

[serejke]

Done.

1. I've splitted commits into 3 as you suggested.
2. Fixed a corner case of 'zip size' == 'size limit'. Let's read at most (limit+1) bytes and check that we haven't read over limit.
3. Fixed formatting.

[plamentotev]

Thanks @serejke. I've corrected some minor formatting issues, added additional test case for when the zip size is equal to the size limit and merged it.