From e2e5449df186256a7e2f7590a7c6c8c30ac00b07 Mon Sep 17 00:00:00 2001
From: "Zickermann, Jan" <jan.zickermann@dataport.de>
Date: Mon, 3 Jun 2024 09:52:11 +0200
Subject: [PATCH] OZG-5412 xta-adapter: Throw on suspicious ZIP compression
ratio
---
.../eingang/xta/zip/ReadableZipEntry.java | 9 ++-
.../eingang/xta/zip/ZipFileExtractor.java | 31 +++++--
.../eingang/xta/zip/ReadableZipEntryTest.java | 80 +++++++++++++++++++
.../eingang/xta/zip/TestZipFileFactory.java | 14 +++-
.../eingang/xta/zip/ZipFileExtractorTest.java | 50 +++++++-----
5 files changed, 153 insertions(+), 31 deletions(-)
create mode 100644 xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntryTest.java
diff --git a/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntry.java b/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntry.java
index f82f0090a..fec9014ca 100644
--- a/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntry.java
+++ b/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntry.java
@@ -5,6 +5,7 @@ import java.io.InputStream;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
+import de.ozgcloud.eingang.common.errorhandling.TechnicalException;
import lombok.Builder;
@Builder
@@ -13,8 +14,12 @@ record ReadableZipEntry(ZipEntry zipEntry, ZipFile parentZip) {
return parentZip.getInputStream(zipEntry);
}
- public Long getSize() {
- return zipEntry.getSize();
+ public Long getPositiveSize() {
+ var size = zipEntry.getSize();
+ if (size < 0) {
+ throw new TechnicalException("Size of ZIP entry is unknown.");
+ }
+ return size;
}
public String getName() {
diff --git a/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractor.java b/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractor.java
index 66480bc98..8f162061d 100644
--- a/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractor.java
+++ b/xta-adapter/src/main/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractor.java
@@ -20,23 +20,38 @@ import de.ozgcloud.eingang.common.formdata.IncomingFile;
// TODO Resolve code duplication with ZipAttachmentReader in semantik-adapter common.zip
// Note: In contrast to the ZipAttachmentReader, here, the zip file is not included in list of extracted files
-// Further, there is no ZIP_MAX_THRESHOLD detection, instead the reader will abort if the inputstream exceeds zipFile.getSize().
+// Further, the suspicious compression ratio ZIP_MAX_THRESHOLD is evaluated on the whole zipFile, instead of individual entries
@Component
public class ZipFileExtractor {
+ static final double ZIP_MAX_THRESHOLD = 100;
static final int ZIP_MAX_TOTAL_SIZE = 500 * 1024 * 1024;
static final int ZIP_MAX_ENTRIES = 100;
public List<IncomingFile> extractIncomingFilesSafely(IncomingFile zipIncomingFile) {
var zipFile = zipIncomingFile.getFile();
- verifySizeLimit(zipFile);
+ verifyLimits(zipFile);
return extractIncomingFiles(zipFile);
}
- void verifySizeLimit(File zipFile) {
- var totalSize = sumUncompressedEntrySizes(zipFile);
- if (totalSize > getZipMaxTotalSize()) {
- throw new TechnicalException("Expect uncompressed size %s to be smaller than %d!".formatted(totalSize, getZipMaxTotalSize()));
+ void verifyLimits(File zipFile) {
+ var uncompressedSize = sumUncompressedEntrySizes(zipFile);
+ verifySizeLimit(uncompressedSize);
+ verifyCompressionRatio(zipFile, uncompressedSize);
+ }
+
+ private void verifySizeLimit(long uncompressedSize) {
+ if (uncompressedSize > getZipMaxTotalSize()) {
+ throw new TechnicalException("Expect uncompressed size %s to be smaller than %d!".formatted(uncompressedSize, getZipMaxTotalSize()));
+ }
+ }
+
+ private void verifyCompressionRatio(File zipFile, long totalSize) {
+ var compressionRatio = (double) totalSize / zipFile.length();
+ if (compressionRatio > ZIP_MAX_THRESHOLD) {
+ throw new TechnicalException(
+ "Expect compression ratio %s to be smaller than %s! A zip bomb attack is suspected!".formatted(compressionRatio,
+ ZIP_MAX_THRESHOLD));
}
}
@@ -45,7 +60,7 @@ public class ZipFileExtractor {
}
Long sumUncompressedEntrySizes(File zipFile) {
- return mapZipEntries(zipFile, ReadableZipEntry::getSize)
+ return mapZipEntries(zipFile, ReadableZipEntry::getPositiveSize)
.stream()
.mapToLong(Long::longValue)
.sum();
@@ -57,7 +72,7 @@ public class ZipFileExtractor {
private IncomingFile mapReadableEntryToIncomingFile(ReadableZipEntry entry) {
File file;
- try (var inputStream = new LimitedInputStream(entry.getInputStream(), entry.getSize())) {
+ try (var inputStream = new LimitedInputStream(entry.getInputStream(), entry.getPositiveSize())) {
file = TempFileUtils.writeTmpFile(inputStream);
} catch (IOException | de.ozgcloud.common.errorhandling.TechnicalException e) {
throw new TechnicalException("Failed reading zip file element %s!".formatted(entry.getName()), e);
diff --git a/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntryTest.java b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntryTest.java
new file mode 100644
index 000000000..e5bfea088
--- /dev/null
+++ b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntryTest.java
@@ -0,0 +1,80 @@
+package de.ozgcloud.eingang.xta.zip;
+
+import static org.assertj.core.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+import java.io.InputStream;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+
+import de.ozgcloud.eingang.common.errorhandling.TechnicalException;
+import lombok.SneakyThrows;
+
+class ReadableZipEntryTest {
+
+ @Mock
+ ZipEntry zipEntry;
+
+ @Mock
+ ZipFile zipFile;
+
+ private ReadableZipEntry readableZipEntry;
+
+ @BeforeEach
+ void mock() {
+ readableZipEntry = ReadableZipEntry.builder()
+ .zipEntry(zipEntry)
+ .parentZip(zipFile)
+ .build();
+
+ }
+
+ @DisplayName("get input stream")
+ @Nested
+ class TestGetInputStream {
+ @Mock
+ private InputStream inputStream;
+
+ @SneakyThrows
+ @DisplayName("should return input stream")
+ @Test
+ void shouldReturnInputStream() {
+ when(zipFile.getInputStream(zipEntry)).thenReturn(inputStream);
+
+ var inputStreamResult = readableZipEntry.getInputStream();
+
+ assertThat(inputStreamResult).isEqualTo(inputStream);
+ }
+ }
+
+ @DisplayName("get positive size")
+ @Nested
+ class TestGetPositiveSize {
+ @DisplayName("should return size")
+ @Test
+ void shouldReturnSize() {
+ var size = 123L;
+ when(zipEntry.getSize()).thenReturn(size);
+
+ var sizeResult = readableZipEntry.getPositiveSize();
+
+ assertThat(sizeResult).isEqualTo(size);
+ }
+
+ @DisplayName("should throw if size is negative")
+ @Test
+ void shouldThrowIfSizeIsNegative() {
+ var size = -1L;
+ when(zipEntry.getSize()).thenReturn(size);
+
+ assertThatThrownBy(() -> readableZipEntry.getPositiveSize()).isInstanceOf(TechnicalException.class);
+ }
+
+ }
+}
diff --git a/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/TestZipFileFactory.java b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/TestZipFileFactory.java
index cb721d211..8ac1b3595 100644
--- a/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/TestZipFileFactory.java
+++ b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/TestZipFileFactory.java
@@ -40,10 +40,17 @@ public class TestZipFileFactory {
}
public static File createTempZipBomb(int maxTotalSize) {
+ return overwriteFileWithZipEntrySize(
+ createTempZipWithSingleEntry(maxTotalSize * 2),
+ maxTotalSize
+ );
+ }
+
+ private static File createTempZipWithSingleEntry(int entrySize) {
var file = TempFileUtils.createTmpFile().toFile();
try (var zipOutputStream = new ZipOutputStream(new FileOutputStream(file))) {
var entry = new ZipEntry(EXPANDED_ENTRY_NAME);
- var content = "A".repeat(2 * maxTotalSize).getBytes();
+ var content = "A".repeat(entrySize).getBytes();
zipOutputStream.putNextEntry(entry);
zipOutputStream.write(content);
@@ -52,10 +59,13 @@ public class TestZipFileFactory {
} catch (IOException e) {
throw new RuntimeException("Failed to create temporary zip file", e);
}
+ return file;
+ }
+ private static File overwriteFileWithZipEntrySize(File file, int newSize) {
try {
var zipFileBytes = IOUtils.toByteArray(new FileInputStream(file));
- overwriteZipEntrySize(zipFileBytes, EXPANDED_ENTRY_NAME, maxTotalSize);
+ overwriteZipEntrySize(zipFileBytes, EXPANDED_ENTRY_NAME, newSize);
// Write the adjusted ZIP content back to the file
try (var fos = new FileOutputStream(file)) {
diff --git a/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractorTest.java b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractorTest.java
index aeb945cc8..d23838973 100644
--- a/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractorTest.java
+++ b/xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ZipFileExtractorTest.java
@@ -54,16 +54,17 @@ class ZipFileExtractorTest {
outIncomingFiles = List.of(outIncomingFile);
when(incomingZipFile.getFile()).thenReturn(zipFile);
- doNothing().when(extractor).verifySizeLimit(zipFile);
+ doNothing().when(extractor).verifyLimits(zipFile);
+
doReturn(outIncomingFiles).when(extractor).extractIncomingFiles(zipFile);
}
- @DisplayName("should call verify size limit")
+ @DisplayName("should call verify limits")
@Test
void shouldCallVerifySizeLimit() {
extractor.extractIncomingFilesSafely(incomingZipFile);
- verify(extractor).verifySizeLimit(zipFile);
+ verify(extractor).verifyLimits(zipFile);
}
@DisplayName("should return")
@@ -75,26 +76,37 @@ class ZipFileExtractorTest {
}
}
- @DisplayName("verify size limit")
+ @DisplayName("verify limits")
@Nested
- class TestVerifySizeLimit {
+ class TestVerifyLimits {
@Mock
File zipFile;
@DisplayName("should return")
@Test
void shouldReturn() {
- doReturn((long) ZipFileExtractor.ZIP_MAX_TOTAL_SIZE).when(extractor).sumUncompressedEntrySizes(zipFile);
+ when(zipFile.length()).thenReturn((long) ZIP_MAX_TOTAL_SIZE / 2);
+ doReturn((long) ZIP_MAX_TOTAL_SIZE).when(extractor).sumUncompressedEntrySizes(zipFile);
- extractor.verifySizeLimit(zipFile);
+ extractor.verifyLimits(zipFile);
}
- @DisplayName("should throw if limit exceeded")
+ @DisplayName("should throw if size limit exceeded")
@Test
- void shouldThrowIfLimitExceeded() {
- doReturn((long) ZipFileExtractor.ZIP_MAX_TOTAL_SIZE + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
+ void shouldThrowIfSizeLimitExceeded() {
+ doReturn((long) ZIP_MAX_TOTAL_SIZE + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
- assertThatThrownBy(() -> extractor.verifySizeLimit(zipFile))
+ assertThatThrownBy(() -> extractor.verifyLimits(zipFile))
+ .isInstanceOf(TechnicalException.class);
+ }
+
+ @DisplayName("should throw if ratio exceeded")
+ @Test
+ void shouldThrowIfRatioExceeded() {
+ when(zipFile.length()).thenReturn(1L);
+ doReturn((long) ZIP_MAX_THRESHOLD + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
+
+ assertThatThrownBy(() -> extractor.verifyLimits(zipFile))
.isInstanceOf(TechnicalException.class);
}
}
@@ -164,13 +176,13 @@ class ZipFileExtractorTest {
@DisplayName("should throw with too many entries")
@Test
void shouldThrow() {
- var zipFile = createTempZipFile(IntStream.range(0, ZIP_MAX_ENTRIES + 1).mapToObj(i -> TestZipEntry.builder()
+ var zipFile = createIncomingFile(createTempZipFile(IntStream.range(0, ZIP_MAX_ENTRIES + 1).mapToObj(i -> TestZipEntry.builder()
.name("file%d.txt".formatted(i))
.content(toBytes("A".repeat(2)))
.build()
- ).toList());
+ ).toList()));
- assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(createIncomingFile(zipFile)))
+ assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(zipFile))
.isInstanceOf(TechnicalException.class);
}
@@ -179,9 +191,9 @@ class ZipFileExtractorTest {
@Test
void shouldThrowWithFakeGetSize() {
doReturn(SMALLER_MAX_ZIP_FILE_SIZE).when(extractor).getZipMaxTotalSize();
- var zipBomb = createTempZipBomb(SMALLER_MAX_ZIP_FILE_SIZE);
+ var zipBomb = createIncomingFile(createTempZipBomb(SMALLER_MAX_ZIP_FILE_SIZE));
- assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(createIncomingFile(zipBomb)))
+ assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(zipBomb))
.isInstanceOf(TechnicalException.class)
.hasRootCauseMessage(LimitedInputStream.LIMITED_EXCEEDED_MESSAGE);
}
@@ -190,13 +202,13 @@ class ZipFileExtractorTest {
@Test
void shouldThrowWithTooLargeSize() {
doReturn(SMALLER_MAX_ZIP_FILE_SIZE).when(extractor).getZipMaxTotalSize();
- var zipFile = createTempZipFile(List.of(TestZipEntry.builder()
+ var zipFile = createIncomingFile(createTempZipFile(List.of(TestZipEntry.builder()
.name("toolargefile.txt")
.content(toBytes("A".repeat(SMALLER_MAX_ZIP_FILE_SIZE + 1)))
.build()
- ));
+ )));
- assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(createIncomingFile(zipFile)))
+ assertThatThrownBy(() -> extractor.extractIncomingFilesSafely(zipFile))
.isInstanceOf(TechnicalException.class);
}
--
GitLab