Fix: add size limit when reading data from a zip file entry (#1673)

* fix: ensure that only a given maximum number of bytes are read when reading a zip archive entry, cleanups

* add check for negative file sizes and read at most the number of declared size in the zip entry

* include crafted test zip with wrong file sizes, improve tests and error messages

Author: netomi

server/src/main/java/org/eclipse/openvsx/ExtensionService.java

@@ -14,6 +14,7 @@
 import jakarta.transaction.Transactional;
 import jakarta.transaction.Transactional.TxType;
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.openvsx.admin.RemoveFileJobRequest;
 import org.eclipse.openvsx.cache.CacheService;
@@ -187,9 +188,7 @@ private TempFile createExtensionFile(InputStream content) {
             }
 
             if (size > maxContentSize) {
-                try {
-                    extensionFile.close();
-                } catch (IOException _) {}
+                IOUtils.closeQuietly(extensionFile);
                 var maxSize = FileUtils.byteCountToDisplaySize(maxContentSize);
                 throw new ErrorResultException("The extension package exceeds the size limit of " + maxSize + ".", HttpStatus.PAYLOAD_TOO_LARGE);
             }

server/src/main/java/org/eclipse/openvsx/util/ArchiveUtil.java

@@ -9,6 +9,10 @@
  ********************************************************************************/
 package org.eclipse.openvsx.util;
 
+import jakarta.annotation.Nullable;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
+
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.InvalidPathException;
@@ -25,31 +29,52 @@ public final class ArchiveUtil {
 
     private ArchiveUtil() {}
 
-    public static ZipEntry getEntryIgnoreCase(ZipFile archive, String entryName) {
+    public static @Nullable ZipEntry getEntryIgnoreCase(ZipFile archive, String entryName) {
         return archive.stream()
                 .filter(entry -> entry.getName().equalsIgnoreCase(entryName))
                 .findAny()
                 .orElse(null);
     }
 
     public static TempFile readEntry(ZipFile archive, String entryName) throws IOException {
+        return readEntry(archive, entryName, MAX_ENTRY_SIZE);
+    }
+
+    static TempFile readEntry(ZipFile archive, String entryName, long maxEntrySize) throws IOException {
         var entry = archive.getEntry(entryName);
-        if (entry == null)
-            return null;
-        return readEntry(archive, entry);
+        return entry != null ? readEntry(archive, entry, maxEntrySize) : null;
     }
 
     public static TempFile readEntry(ZipFile archive, ZipEntry entry) throws IOException {
+        return readEntry(archive, entry, MAX_ENTRY_SIZE);
+    }
+
+    static TempFile readEntry(ZipFile archive, ZipEntry entry, long maxEntrySize) throws IOException {
         var fileNameIndex = entry.getName().lastIndexOf('/');
         var fileName = fileNameIndex == -1 ? entry.getName() : entry.getName().substring(fileNameIndex + 1);
         var suffixIndex = fileName.lastIndexOf('.');
         var suffix = suffixIndex == -1 ? null : fileName.substring(suffixIndex);
         var prefix = suffixIndex == -1 ? fileName : fileName.substring(0, suffixIndex);
         var file = new TempFile(prefix, suffix);
         try (var out = Files.newOutputStream(file.getPath())){
-            if (entry.getSize() > MAX_ENTRY_SIZE)
-                throw new ErrorResultException("The file " + entry.getName() + " exceeds the size limit of 32 MB.");
-            archive.getInputStream(entry).transferTo(out);
+            if (entry.getSize() < 0) {
+                IOUtils.closeQuietly(file);
+                throw new ErrorResultException("The file " + entry.getName() + " has a negative size.");
+            }
+
+            if (entry.getSize() > maxEntrySize) {
+                IOUtils.closeQuietly(file);
+                var maxSize = FileUtils.byteCountToDisplaySize(maxEntrySize);
+                throw new ErrorResultException("The file " + entry.getName() + " exceeds the size limit of " + maxSize + ".");
+            }
+
+            // Read at most the number of bytes as declared by the entry
+            try (var is = new SizeLimitInputStream(archive.getInputStream(entry), entry.getSize())) {
+                is.transferTo(out);
+            } catch (IOException e) {
+                IOUtils.closeQuietly(file);
+                throw new ErrorResultException("Failed to read " + entry.getName() + ": " + e.getMessage());
+            }
         }
         return file;
     }
@@ -92,5 +117,4 @@ public static boolean isSafePath(String filePath) {
             return false;
         }
     }
-
 }

server/src/main/java/org/eclipse/openvsx/util/FileUtil.java

@@ -7,7 +7,6 @@
  *
  * SPDX-License-Identifier: EPL-2.0
  * ****************************************************************************** */
-
 package org.eclipse.openvsx.util;
 
 import java.nio.file.Files;
@@ -31,7 +30,7 @@ protected boolean removeEldestEntry(Map.Entry eldest){
         });
     }
 
-    private FileUtil(){}
+    private FileUtil() {}
 
     /***
      * Write to file synchronously, if it doesn't already exist.

server/src/main/java/org/eclipse/openvsx/util/SizeLimitInputStream.java

@@ -12,6 +12,8 @@
  ********************************************************************************/
 package org.eclipse.openvsx.util;
 
+import org.apache.commons.io.FileUtils;
+
 import java.io.FilterInputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -51,7 +53,8 @@ public int read(byte[] b, int off, int len) throws IOException {
     private void checkLimit(long n) throws IOException {
         bytesRead += n;
         if (bytesRead > maxBytes) {
-            throw new IOException("File size exceeds limit of " + maxBytes + " bytes");
+            var maxSize = FileUtils.byteCountToDisplaySize(maxBytes);
+            throw new IOException("File size exceeds limit of " + maxSize + ".");
         }
     }
 }

server/src/main/java/org/eclipse/openvsx/util/TempFile.java

@@ -12,11 +12,12 @@
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.entities.Namespace;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 
-public class TempFile implements AutoCloseable {
+public class TempFile implements Closeable {
 
     private final Path path;
     private FileResource resource;

server/src/test/java/org/eclipse/openvsx/util/ArchiveUtilTest.java

@@ -9,8 +9,10 @@
  ********************************************************************************/
 package org.eclipse.openvsx.util;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.*;
+import static org.assertj.core.api.InstanceOfAssertFactories.PATH;
 
+import java.io.IOException;
 import java.nio.file.Files;
 import java.util.zip.ZipFile;
 
@@ -21,7 +23,10 @@ class ArchiveUtilTest {
     @Test
     void testTodoTree() throws Exception {
         var packageUrl = getClass().getResource("todo-tree.zip");
+
+        assertThat(packageUrl).isNotNull();
         assertThat(packageUrl.getProtocol()).isEqualTo("file");
+
         try (
             var archive = new ZipFile(packageUrl.getPath());
             var packageFile = ArchiveUtil.readEntry(archive, "extension/package.json");
@@ -34,4 +39,22 @@ void testTodoTree() throws Exception {
         }
     }
 
+    @Test
+    void testExceedMaxEntrySize() throws IOException {
+        // an artificially crafted zip file with a file whose size is set lower as its actual content
+        var packageUrl = getClass().getResource("wrong-size.zip");
+
+        assertThat(packageUrl).isNotNull();
+        assertThat(packageUrl.getProtocol()).isEqualTo("file");
+
+        try (var archive = new ZipFile(packageUrl.getPath())) {
+            assertThatThrownBy(() -> ArchiveUtil.readEntry(archive, "extension/README.md", 8192))
+                    .isExactlyInstanceOf(ErrorResultException.class)
+                    .hasMessage("The file extension/README.md exceeds the size limit of 8 KB.");
+
+            assertThatThrownBy(() -> ArchiveUtil.readEntry(archive, "extension/package.json", 8192))
+                    .isExactlyInstanceOf(ErrorResultException.class)
+                    .hasMessageContaining("Failed to read extension/package.json: File size exceeds limit of 0 bytes.");
+        }
+    }
 }