11

Apache POI is opening zip-files on a regular basis because Microsoft Excel/Word/... files are zip-files in their newer format. In order to prevent some types of denial-of-service-attacks, it has functionality when opening Zip-files to not read files which expand a lot and thus could be used to overwhelm the main memory by providing a small malicious file which explodes when uncompressed into memory. Apache POI calls this zip-bomb-protection.

Up to Java 9 it could use some workaround via reflection to inject a counting-InputStream into ZipFile/ZipEntry to detect an explosion in expanded data and this way prevent zip-bombs.

However in Java 10 this is not possible any more because the implementation of ZipFile was changed in a way that prevents this (hard cast to ZipFile$ZipFileInputStream in ZipFile).

So we are looking for a different way to count the number of extracted bytes during extracting to be able to stop as soon as the compression ratio reaches a certain limit.

Is there a way to do zip-bomb-detection differently without resorting to reflection?

centic
  • 15,565
  • 9
  • 68
  • 125
  • 2
    Why don't you pre-check the file using `ZipInputStream` *before* giving the file to POI? – Andreas Mar 31 '18 at 08:51
  • Are you really sure that "zip-bomb-detection" will be your (`apache poi`' s) task? I believe most users are **not** happy with this and do setting `MinInflateRatio` to a value where your whole "zip-bomb-detection" effort is thwarted. So, in my humble opinion, this is unnecessary effort. – Axel Richter Mar 31 '18 at 08:52
  • @AxelRichter, Yes, we want to be "secure by default", otherwise open source software is always the first one to blame if companies fail to enable such checks. I think Apache POI had only a few related questions and it is very easy to disable the check if it is not wanted. – centic Mar 31 '18 at 09:56
  • 1
    @Andreas, I want to fix Apache POI itself here, so I am interested if others have had similar requirements and solved this already in some clever way. – centic Mar 31 '18 at 09:58
  • Have you looked into other zip libraries? Doing it yourself seems a better option than relying on internal features of the Sun/Oracle provided functionality. A quick search found [an Apache library](https://commons.apache.org/proper/commons-compress/zip.html) (hah, right back at you). Why not use / improve that? – Maarten Bodewes May 20 '18 at 14:11
  • The internal library is probably functional for the majority of tasks, but if your subsystem has `.zip` support from external sources as important feature, I would not rely on any internal library. – Maarten Bodewes May 20 '18 at 14:21

2 Answers2

3

After some investigation we used zip-functionality from Apache commons-compress, which allows to perform this kind of check without having to resort to reflection, so we now can do these checks with any version of Java.

Look at https://github.com/apache/poi/tree/trunk/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util for the resulting implementation in Apache POI, especially ZipArchiveThresholdInputStream.

user11153
  • 8,536
  • 5
  • 47
  • 50
centic
  • 15,565
  • 9
  • 68
  • 125
0

I can't imagine why you needed a reflection/injection hack in the first place. You seem to pass not a filename but some instance like zipfile or zipinputstream.

If you have a file (or can save to a file first), then you can first check the zip file entries sizes (not even decompressing) before handing it to the vulnerable library. Even if you needed to pass a zipfie, you could extend the zipfile class to proxy calls.

If you have zip stream and really cannot temp-save to disk and must read as a zipinputstream somehow, then override methods of zipinputstream (getnextentry, read, etc).

user2023577
  • 1,752
  • 1
  • 12
  • 23
  • The reading/unzipping is built into the core of a large library, so not easy to simply work on files if there is an InputStream coming in. Also performance is affected if you have to open/scan files multiple times. However we switched zip-functoinality to commons-compress in the meantime which allows to do this nicely on all versions of Java. And on overriding methods, you obviously did not look at the actual changes in Java 10, because they actively prevent you from doing this as far as I see. – centic Mar 08 '19 at 15:35