Merge pull request #27 from pixee/npe-in-hardenedzipinputstream

🐛 Fix NPE in HardenedZipInputStream
pixee · Mar 7, 2024 · 691062c · 691062c
2 parents b885b03 + 839cdfa
commit 691062c
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 18 deletions.
diff --git a/.github/badges/branches.svg b/.github/badges/branches.svg
diff --git 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
@@ -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,30 +49,34 @@ 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 {
     String currentDir = new File("").getCanonicalFile().getName();
     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
   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) {}
+  }
 }