OZG-5412 xta-adapter: Throw on suspicious ZIP compression ratio

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() {

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);

xta-adapter/src/test/java/de/ozgcloud/eingang/xta/zip/ReadableZipEntryTest.java

+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);
+		}
+
+	}
+}

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)) {

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.verifyLimits(zipFile);
+		}
+
+		@DisplayName("should throw if size limit exceeded")
+		@Test
+		void shouldThrowIfSizeLimitExceeded() {
+			doReturn((long) ZIP_MAX_TOTAL_SIZE + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
 
-			extractor.verifySizeLimit(zipFile);
+			assertThatThrownBy(() -> extractor.verifyLimits(zipFile))
+					.isInstanceOf(TechnicalException.class);
 		}
 
-		@DisplayName("should throw if limit exceeded")
+		@DisplayName("should throw if ratio exceeded")
 		@Test
-		void shouldThrowIfLimitExceeded() {
-			doReturn((long) ZipFileExtractor.ZIP_MAX_TOTAL_SIZE + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
+		void shouldThrowIfRatioExceeded() {
+			when(zipFile.length()).thenReturn(1L);
+			doReturn((long) ZIP_MAX_THRESHOLD + 1).when(extractor).sumUncompressedEntrySizes(zipFile);
 
-			assertThatThrownBy(() -> extractor.verifySizeLimit(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);
 		}