CON-626: Fix zip extraction vulnerability when installing plugins (#339)

* add unit test that reproduces the bug

* move ZipFiles util to concourse-server project

* add better impls in ZipFiles

* update method usage in PluginManager

* fix formatting

* update changelog

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 #### Version 0.9.0 (TBD)
 
+##### Vulnerabilities
+* Fixed a vulnerability that made it possible for a malicious plugin archive that contained entry names with path traversal elements to execute arbitraty code on the filesystem, if installed. This vulnerability, which was first disclosed by the [Snyk Security Research Team](https://snyk.io/docs/security), existed because Concourse did not verify that an entry, potentially extracted from a zipfile, would exist within the target directory if actually extracted. We've fixed this vulnerability by switching to the [zt-zip](https://github.com/zeroturnaround/zt-zip) library for internal zip handling. In addition to having protections againist this vulnerability, `zt-zip` is battle-tested and well maintained by [ZeroTurnaround](https://zeroturnaround.com/). Thanks again to the Snyk Security Research Team for disclosing this vulnerability.
+
 ##### Security Model
 * Added a notion of *user roles*. Each user account can either have the `ADMIN` or `USER` role. `ADMIN` users are permitted to invoke management functions whereas accounts with the `USER` role are not.
 	* All previously existing users are assigned the `ADMIN` role on upgrade. You can change a user's role using the `users` CLI.

NOTICE

@@ -22,6 +22,13 @@
      https://github.com/cinchapi/bucket
      Copyright 2017 Cinchapi Inc.
 
+     CCL
+     ---
+     The official specification and API library for the Concourse Criteria Language (CCL).
+
+     https://github.com/cinchapi/ccl
+     Copyright 2018 Cinchapi Inc.
+
   2. Includes software developed by the Apache Software Foundation
      (http://www.apache.org) or released under the Apache License,
      Version 2.0:
@@ -349,6 +356,13 @@
       https://github.com/xerial/sqlite-jdbc
       Copyright Taro L. Saito, David Crawshaw and associated authors
 
+      zt-zip
+      ------
+      The zt-zip project, started and coded by Rein Raudjärv and maintained by ZeroTurnaround, is a library for handling zip files programmatically.
+
+      https://zeroturnaround.com/
+      Copyright 2012 ZeroTurnaround LLC.
+
   3. Includes software released under the MIT License, which is compatible with
      the Apache License, Version 2.0:
 

concourse-server/build.gradle

@@ -40,6 +40,7 @@ dependencies {
     compile (group:'com.cinchapi', name: 'bucket', version: bucketVersion, changing:true) {
       exclude group: 'com.cinchapi'
     }
+    compile 'org.zeroturnaround:zt-zip:1.13'
 
     testCompile 'com.carrotsearch:junit-benchmarks:0.7.2'
     testCompile 'io.takari.junit:takari-cpsuite:1.2.7'

concourse-server/src/main/java/com/cinchapi/concourse/server/plugin/PluginManager.java

@@ -389,7 +389,7 @@ public void installBundle(String bundle) {
                 .getNameWithoutExtension(bundle);
         String name = null;
         try {
-            String manifest = ZipFiles.getEntryContent(bundle,
+            String manifest = ZipFiles.getEntryContentUtf8(bundle,
                     basename + File.separator + MANIFEST_FILE);
             JsonObject json = (JsonObject) new JsonParser().parse(manifest);
             name = json.get("bundleName").getAsString();

concourse-server/src/main/java/com/cinchapi/concourse/util/ZipFiles.java

@@ -0,0 +1,90 @@
+/*
+ * Copyright (c) 2013-2018 Cinchapi Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.cinchapi.concourse.util;
+
+import java.io.File;
+import java.nio.ByteBuffer;
+
+import org.zeroturnaround.zip.ZipUtil;
+
+import com.cinchapi.common.base.Verify;
+import com.cinchapi.common.io.ByteBuffers;
+
+/**
+ * A utility class for handling zip files
+ * 
+ * @author Jeff Nelson
+ */
+public final class ZipFiles {
+
+    static {
+        Logging.disable(ZipUtil.class);
+    }
+
+    /**
+     * Get the content for the entry at {@code relativeEntryPath} from within
+     * the zip file.
+     * 
+     * @param zipPath the path to the zip file
+     * @param relativeEntryPath the path of the entry to extract
+     * @return the content of the entry as a {@link ByteBuffer}
+     */
+    public static ByteBuffer getEntryContent(String zipFile,
+            String relativeEntryPath) {
+        return ByteBuffer.wrap(
+                ZipUtil.unpackEntry(new File(zipFile), relativeEntryPath));
+    }
+
+    /**
+     * Get the content for the entry at {@code relativeEntryPath} from within
+     * the zip file as a UTF-8 string.
+     * 
+     * @param zipPath the path to the zip file
+     * @param relativeEntryPath the path of the entry to extract
+     * @return the content of the entry as a UTF-8 string
+     */
+    public static String getEntryContentUtf8(String zipFile,
+            String relativeEntryPath) {
+        return ByteBuffers
+                .getUtf8String(getEntryContent(zipFile, relativeEntryPath));
+    }
+
+    /**
+     * Unzip the contents of the file at {@code zipPath} to the
+     * {@code destination} directory.
+     * 
+     * @param zipPath the absolute path to the zip file
+     * @param destination the absolute path to the destination
+     */
+    public static void unzip(String zipPath, String destination) {
+        File dest = new File(destination);
+        if(!dest.exists()) {
+            dest.mkdirs();
+        }
+        Verify.thatArgument(dest.isDirectory(),
+                "Unzip destination must be a directory");
+        try {
+            ZipUtil.unpack(new File(zipPath), dest);
+        }
+        catch (org.zeroturnaround.zip.ZipException e) {
+            IllegalArgumentException ex = new IllegalArgumentException(
+                    e.getMessage());
+            ex.setStackTrace(e.getStackTrace());
+            throw ex;
+        }
+    }
+
+}

concourse-server/src/test/java/com/cinchapi/concourse/util/ZipFilesTest.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2013-2018 Cinchapi Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.cinchapi.concourse.util;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Unit tests for the {@link ZipFiles} utility class.
+ *
+ * @author Jeff Nelson
+ */
+public class ZipFilesTest {
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testCannotExtractZipContainingEntryWithPathTraversalCharacters()
+            throws IOException { // CON-626
+        String zip = Resources.getAbsolutePath("/evil.zip");
+        String parent = FileOps.tempDir("zip");
+        String dest = Paths.get(parent).resolve("target").toAbsolutePath()
+                .toString();
+        ZipFiles.unzip(zip, dest);
+        Assert.assertEquals(1, Files.list(Paths.get(dest)).count());
+        Assert.assertEquals(1, Files.list(Paths.get(parent)).count());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testCannotExtractZipFileToNonDestination() {
+        String zip = Resources.getAbsolutePath("/good.zip");
+        String dest = FileOps.tempFile();
+        ZipFiles.unzip(zip, dest);
+    }
+
+    @Test
+    public void testGetEntryContentUtf8() {
+        String zip = Resources.getAbsolutePath("/good.zip");
+        String content = ZipFiles.getEntryContentUtf8(zip, "file.txt");
+        Assert.assertEquals("Hello World", content);
+    }
+
+    @Test
+    public void testUnzip() throws IOException {
+        String zip = Resources.getAbsolutePath("/good.zip");
+        String parent = FileOps.tempDir("zip");
+        String dest = Paths.get(parent).resolve("target").toAbsolutePath()
+                .toString();
+        ZipFiles.unzip(zip, dest);
+        Assert.assertEquals(1, Files.list(Paths.get(dest)).count());
+        Assert.assertEquals(1, Files.list(Paths.get(parent)).count());
+    }
+
+}