diff --git a/.github/badges/branches.svg b/.github/badges/branches.svg index 0b8d9dd..ef724ad 100644 --- a/.github/badges/branches.svg +++ b/.github/badges/branches.svg @@ -1 +1 @@ -branches60.6% \ No newline at end of file +branches60.8% \ No newline at end of file diff --git a/src/main/java/io/github/pixee/security/ZipSecurity.java b/src/main/java/io/github/pixee/security/ZipSecurity.java index 3e2f917..c94f6ab 100644 --- a/src/main/java/io/github/pixee/security/ZipSecurity.java +++ b/src/main/java/io/github/pixee/security/ZipSecurity.java @@ -53,9 +53,11 @@ private HardenedZipInputStream(final InputStream in, final Charset charset) { @Override public ZipEntry getNextEntry() throws IOException { final ZipEntry entry = super.getNextEntry(); + if (entry == null) { + return null; + } final String name = entry.getName(); - - if (!"".equals(name.trim())) { + if (!name.trim().isEmpty()) { if (isRootFileEntry(name)) { throw new SecurityException("encountered zip file path that is absolute: " + name); } @@ -79,7 +81,8 @@ private boolean containsEscapesAndTargetsBelowRoot(final String name) { return false; } - private boolean isBelowOrSisterToCurrentDirectory(final String untrustedFileWithEscapes) throws IOException { + private boolean isBelowOrSisterToCurrentDirectory(final String untrustedFileWithEscapes) + throws IOException { // Get the absolute path of the current directory final File currentDirectory = new File("").getCanonicalFile(); final Path currentPathRoot = currentDirectory.toPath(); diff --git a/src/test/java/io/github/pixee/security/ZipSecurityTest.java b/src/test/java/io/github/pixee/security/ZipSecurityTest.java index bd3a3b4..d8e8abd 100644 --- a/src/test/java/io/github/pixee/security/ZipSecurityTest.java +++ b/src/test/java/io/github/pixee/security/ZipSecurityTest.java @@ -20,10 +20,13 @@ void it_doesnt_prevent_normal_zip_file_reads() throws IOException { ZipEntry entry = new ZipEntry("normal.txt"); InputStream is = createZipFrom(entry); - ZipInputStream hardenedStream = - ZipSecurity.createHardenedInputStream(is, StandardCharsets.UTF_8); - ZipEntry retrievedEntry = hardenedStream.getNextEntry(); - assertThat(retrievedEntry.getName(), equalTo("normal.txt")); + try (ZipInputStream hardenedStream = + ZipSecurity.createHardenedInputStream(is, StandardCharsets.UTF_8)) { + ZipEntry retrievedEntry; + while ((retrievedEntry = hardenedStream.getNextEntry()) != null) { + assertThat(retrievedEntry.getName(), equalTo("normal.txt")); + } + } } @ParameterizedTest @@ -32,9 +35,12 @@ void it_doesnt_prevent_normal_zip_files_with_safe_escapes(String path) throws IO ZipEntry entry = new ZipEntry(path); InputStream is = createZipFrom(entry); - ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is); - ZipEntry retrievedEntry = hardenedStream.getNextEntry(); - assertThat(retrievedEntry.getName(), equalTo(path)); + try (ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is)) { + ZipEntry retrievedEntry; + while ((retrievedEntry = hardenedStream.getNextEntry()) != null) { + assertThat(retrievedEntry.getName(), equalTo(path)); + } + } } @ParameterizedTest @@ -43,12 +49,14 @@ void it_prevents_escapes(String path) throws IOException { ZipEntry entry = new ZipEntry(path); InputStream is = createZipFrom(entry); - ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is); - assertThrows(SecurityException.class, hardenedStream::getNextEntry); + try (ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is)) { + assertThrows(SecurityException.class, () -> readAllEntries(hardenedStream)); + } } /** - * This tests that there is no regression of CVE-2024-24569, which was a partial path traversal bypass that allowed access to the current working directory's sibling directories. + * This tests that there is no regression of CVE-2024-24569, which was a partial path traversal + * bypass that allowed access to the current working directory's sibling directories. */ @Test void it_prevents_sister_directory_escape() throws IOException { @@ -56,8 +64,9 @@ void it_prevents_sister_directory_escape() throws IOException { ZipEntry entry = new ZipEntry("foo/../../" + currentDir + "-other-sister-dir"); InputStream is = createZipFrom(entry); - ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is); - assertThrows(SecurityException.class, hardenedStream::getNextEntry); + try (ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is)) { + assertThrows(SecurityException.class, () -> readAllEntries(hardenedStream)); + } } @Test @@ -65,8 +74,9 @@ void it_prevents_absolute_paths_in_zip_entries() throws IOException { ZipEntry entry = new ZipEntry("/foo.txt"); InputStream is = createZipFrom(entry); - ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is); - assertThrows(SecurityException.class, hardenedStream::getNextEntry); + try (ZipInputStream hardenedStream = ZipSecurity.createHardenedInputStream(is)) { + assertThrows(SecurityException.class, () -> readAllEntries(hardenedStream)); + } } private InputStream createZipFrom(final ZipEntry entry) throws IOException { @@ -78,4 +88,9 @@ private InputStream createZipFrom(final ZipEntry entry) throws IOException { return new ByteArrayInputStream(os.toByteArray()); } + + @SuppressWarnings("StatementWithEmptyBody") + private static void readAllEntries(final ZipInputStream zis) throws IOException { + while (zis.getNextEntry() != null) {} + } }