Merge pull request #8749 from vespa-engine/mpolden/harden-zip-handling

Harden zip handling

5 files changed, 189 insertions, 39 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java
index 131164c5c5e..78500d26865 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.application;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.hash.Hashing;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentSpec;
@@ -13,9 +14,11 @@ import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.io.UncheckedIOException;
 import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * A representation of the content of an application package.
@@ -43,10 +46,11 @@ public class ApplicationPackage {
     public ApplicationPackage(byte[] zippedContent) {
         this.zippedContent = Objects.requireNonNull(zippedContent, "The application package content cannot be null");
         this.contentHash = Hashing.sha1().hashBytes(zippedContent).toString();
-        this.deploymentSpec = extractFile("deployment.xml", zippedContent).map(DeploymentSpec::fromXml).orElse(DeploymentSpec.empty);
-        this.validationOverrides = extractFile("validation-overrides.xml", zippedContent).map(ValidationOverrides::fromXml).orElse(ValidationOverrides.empty);
-        Optional<Inspector> buildMetaObject = extractFileBytes("build-meta.json", zippedContent)
-                .map(SlimeUtils::jsonToSlime).map(Slime::get);
+
+        Files files = Files.extract(Set.of("deployment.xml", "validation-overrides.xml", "build-meta.json"), zippedContent);
+        this.deploymentSpec = files.getAsReader("deployment.xml").map(DeploymentSpec::fromXml).orElse(DeploymentSpec.empty);
+        this.validationOverrides = files.getAsReader("validation-overrides.xml").map(ValidationOverrides::fromXml).orElse(ValidationOverrides.empty);
+        Optional<Inspector> buildMetaObject = files.get("build-meta.json").map(SlimeUtils::jsonToSlime).map(Slime::get);
         this.compileVersion = buildMetaObject.map(object -> Version.fromString(object.field("compileVersion").asString()));
         this.buildTime = buildMetaObject.map(object -> Instant.ofEpochMilli(object.field("buildTime").asLong()));
     }
@@ -75,21 +79,51 @@ public class ApplicationPackage {
     /** Returns the time this package was built, if known. */
     public Optional<Instant> buildTime() { return buildTime; }
 
-    private static Optional<byte[]> extractFileBytes(String fileName, byte[] zippedContent) {
-        try (ByteArrayInputStream stream = new ByteArrayInputStream(zippedContent)) {
-            ZipStreamReader reader = new ZipStreamReader(stream);
-            for (ZipStreamReader.ZipEntryWithContent entry : reader.entries())
-                if (entry.zipEntry().getName().equals(fileName) || entry.zipEntry().getName().equals("application/" + fileName)) // TODO: Remove application/ directory support
-                    return Optional.of(entry.content());
-            return Optional.empty();
+
+    private static class Files {
+
+        /** Max size of each extracted file */
+        private static final int maxSize = 10 * 1024 * 1024; // 10 MiB
+
+        // TODO: Vespa 8: Remove application/ directory support
+        private static final String applicationDir = "application/";
+
+        private final ImmutableMap<String, byte[]> files;
+
+        private Files(ImmutableMap<String, byte[]> files) {
+            this.files = files;
+        }
+
+        public static Files extract(Set<String> filesToExtract, byte[] zippedContent) {
+            ImmutableMap.Builder<String, byte[]> builder = ImmutableMap.builder();
+            try (ByteArrayInputStream stream = new ByteArrayInputStream(zippedContent)) {
+                ZipStreamReader reader = new ZipStreamReader(stream,
+                                                             (name) -> filesToExtract.contains(withoutLegacyDir(name)),
+                                                             maxSize);
+                for (ZipStreamReader.ZipEntryWithContent entry : reader.entries()) {
+                    builder.put(withoutLegacyDir(entry.zipEntry().getName()), entry.content());
+                }
+            } catch (IOException e) {
+                throw new UncheckedIOException("Exception reading application package", e);
+            }
+            return new Files(builder.build());
         }
-        catch (IOException e) {
-            throw new IllegalArgumentException("Exception reading application package", e);
+
+        /** Get content of given file name */
+        public Optional<byte[]> get(String name) {
+            return Optional.ofNullable(files.get(name));
+        }
+
+        /** Get reader for the content of given file name */
+        public Optional<Reader> getAsReader(String name) {
+            return get(name).map(ByteArrayInputStream::new).map(InputStreamReader::new);
+        }
+
+        private static String withoutLegacyDir(String name) {
+            if (name.startsWith(applicationDir)) return name.substring(applicationDir.length());
+            return name;
         }
-    }
 
-    private static Optional<Reader> extractFile(String fileName, byte[] zippedContent) {
-        return extractFileBytes(fileName, zippedContent).map(ByteArrayInputStream::new).map(InputStreamReader::new);
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
index 69c846f2562..7576bcae8ee 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReader.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.application;
 
 import com.google.common.collect.ImmutableList;
@@ -6,7 +6,11 @@ import com.google.common.collect.ImmutableList;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Path;
+import java.util.Arrays;
 import java.util.List;
+import java.util.function.Predicate;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
 
@@ -14,50 +18,66 @@ import java.util.zip.ZipInputStream;
  * @author bratseth
  */
 public class ZipStreamReader {
-    
+
     private final ImmutableList<ZipEntryWithContent> entries;
+    private final int maxEntrySizeInBytes;
 
-    public ZipStreamReader(InputStream input) {
+    public ZipStreamReader(InputStream input, Predicate<String> entryNameMatcher, int maxEntrySizeInBytes) {
+        this.maxEntrySizeInBytes = maxEntrySizeInBytes;
         try (ZipInputStream zipInput = new ZipInputStream(input)) {
             ImmutableList.Builder<ZipEntryWithContent> builder = new ImmutableList.Builder<>();
             ZipEntry zipEntry;
-            while (null != (zipEntry = zipInput.getNextEntry()))
+            while (null != (zipEntry = zipInput.getNextEntry())) {
+                if (!entryNameMatcher.test(requireName(zipEntry.getName()))) continue;
                 builder.add(new ZipEntryWithContent(zipEntry, readContent(zipInput)));
+            }
             entries = builder.build();
-        }
-        catch (IOException e) {
-            throw new IllegalArgumentException("IO error reading zip content", e);
+        } catch (IOException e) {
+            throw new UncheckedIOException("IO error reading zip content", e);
         }
     }
-    
+
     private byte[] readContent(ZipInputStream zipInput) {
         try (ByteArrayOutputStream bis = new ByteArrayOutputStream()) {
             byte[] buffer = new byte[2048];
             int read;
-            while ( -1 != (read = zipInput.read(buffer)))
+            long size = 0;
+            while ( -1 != (read = zipInput.read(buffer))) {
+                size += read;
+                if (size > maxEntrySizeInBytes) {
+                    throw new IllegalArgumentException("Entry in zip content exceeded size limit of " +
+                                                       maxEntrySizeInBytes + " bytes");
+                }
                 bis.write(buffer, 0, read);
+            }
             return bis.toByteArray();
-        }
-        catch (IOException e) {
-            throw new IllegalArgumentException("Failed reading from zipped content", e);
+        } catch (IOException e) {
+            throw new UncheckedIOException("Failed reading from zipped content", e);
         }
     }
-    
+
     public List<ZipEntryWithContent> entries() { return entries; }
-    
+
+    private static String requireName(String name) {
+        IllegalArgumentException e = new IllegalArgumentException("Unexpected non-normalized path found in zip content");
+        if (Arrays.asList(name.split("/")).contains("..")) throw e;
+        if (!name.equals(Path.of(name).normalize().toString())) throw e;
+        return name;
+    }
+
     public static class ZipEntryWithContent {
-        
+
         private final ZipEntry zipEntry;
         private final byte[] content;
-        
+
         public ZipEntryWithContent(ZipEntry zipEntry, byte[] content) {
             this.zipEntry = zipEntry;
             this.content = content;
         }
-        
+
         public ZipEntry zipEntry() { return zipEntry; }
         public byte[] content() { return content; }
-        
+
     }
-    
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 63a9d0ce905..6eb5061056f 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -524,6 +524,17 @@ public class ControllerTest {
         tester.controller().applications().deactivate(app.id(), ZoneId.from(Environment.prod, RegionName.from("us-west-1")));
     }
 
+    @Test
+    public void testDeployApplicationPackageWithApplicationDir() {
+        DeploymentTester tester = new DeploymentTester();
+        Application application = tester.createApplication("app1", "tenant1", 1, 1L);
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .region("us-west-1")
+                .build(true);
+        tester.deployCompletely(application, applicationPackage);
+    }
+
     private void runUpgrade(DeploymentTester tester, ApplicationId application, ApplicationVersion version) {
         Version next = Version.fromString("6.2");
         tester.upgradeSystem(next);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReaderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReaderTest.java
new file mode 100644
index 00000000000..92462a67e48
--- /dev/null
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/ZipStreamReaderTest.java
@@ -0,0 +1,77 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller.application;
+
+import org.junit.Test;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * @author mpolden
+ */
+public class ZipStreamReaderTest {
+
+    @Test
+    public void test_size_limit() {
+        Map<String, String> entries = Map.of("foo.xml", "foobar");
+        try {
+            new ZipStreamReader(new ByteArrayInputStream(zip(entries)), "foo.xml"::equals, 1);
+            fail("Expected exception");
+        } catch (IllegalArgumentException ignored) {}
+
+        entries = Map.of("foo.xml", "foobar",
+                         "foo.zip", "0".repeat(100) // File not extracted and thus not subject to size limit
+        );
+        ZipStreamReader reader = new ZipStreamReader(new ByteArrayInputStream(zip(entries)), "foo.xml"::equals,10);
+        byte[] extracted = reader.entries().get(0).content();
+        assertEquals("foobar", new String(extracted, StandardCharsets.UTF_8));
+    }
+
+    @Test
+    public void test_paths() {
+        Map<String, Boolean> tests = Map.of(
+                "../../services.xml", true,
+                "/../.././services.xml", true,
+                "./application/././services.xml", true,
+                "application//services.xml", true,
+                "services..xml", false,
+                "application/services.xml", false,
+                "components/foo-bar-deploy.jar", false,
+                "services.xml", false
+        );
+        tests.forEach((name, expectException) -> {
+            try {
+                new ZipStreamReader(new ByteArrayInputStream(zip(Map.of(name, "foo"))), name::equals, 1024);
+                assertFalse("Expected exception for '" + name + "'", expectException);
+            } catch (IllegalArgumentException ignored) {
+                assertTrue("Unexpected exception for '" + name + "'", expectException);
+            }
+        });
+    }
+
+    private static byte[] zip(Map<String, String> entries) {
+        ByteArrayOutputStream zip = new ByteArrayOutputStream();
+        try (ZipOutputStream out = new ZipOutputStream(zip)) {
+            for (Map.Entry<String, String> entry : entries.entrySet()) {
+                out.putNextEntry(new ZipEntry(entry.getKey()));
+                out.write(entry.getValue().getBytes(StandardCharsets.UTF_8));
+                out.closeEntry();
+            }
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+        return zip.toByteArray();
+    }
+
+}
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index dcc4a2071de..29bec7246ee 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -172,18 +172,26 @@ public class ApplicationPackageBuilder {
     }
 
     public ApplicationPackage build() {
+        return build(false);
+    }
+
+    public ApplicationPackage build(boolean useApplicationDir) {
+        String dir = "";
+        if (useApplicationDir) {
+            dir = "application/";
+        }
         ByteArrayOutputStream zip = new ByteArrayOutputStream();
         try (ZipOutputStream out = new ZipOutputStream(zip)) {
-            out.putNextEntry(new ZipEntry("deployment.xml"));
+            out.putNextEntry(new ZipEntry(dir + "deployment.xml"));
             out.write(deploymentSpec());
             out.closeEntry();
-            out.putNextEntry(new ZipEntry("validation-overrides.xml"));
+            out.putNextEntry(new ZipEntry(dir + "validation-overrides.xml"));
             out.write(validationOverrides());
             out.closeEntry();
-            out.putNextEntry(new ZipEntry("search-definitions/test.sd"));
+            out.putNextEntry(new ZipEntry(dir + "search-definitions/test.sd"));
             out.write(searchDefinition());
             out.closeEntry();
-            out.putNextEntry(new ZipEntry("build-meta.json"));
+            out.putNextEntry(new ZipEntry(dir + "build-meta.json"));
             out.write(buildMeta());
             out.closeEntry();
         } catch (IOException e) {