STORM-3330: More storm-webapp path cleanup

.gitignore

@@ -56,4 +56,8 @@ logs
 hs_err_pid*
 
 # ignore vagrant files
-/integration-test/config/.vagrant/
+/integration-test/config/.vagrant/
+
+# Test jars for zip slip
+!/storm-server/src/test/resources/evil-path-traversal.jar
+!/storm-server/src/test/resources/evil-path-traversal-resources.jar

storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java

@@ -183,24 +183,8 @@ protected void extractDirFromJar(String jarpath, String dir, Path dest) throws I
             Files.createDirectories(dest);
         }
         try (JarFile jarFile = new JarFile(jarpath)) {
-            String toRemove = dir + '/';
-            Enumeration<JarEntry> jarEnums = jarFile.entries();
-            while (jarEnums.hasMoreElements()) {
-                JarEntry entry = jarEnums.nextElement();
-                String name = entry.getName();
-                if (!entry.isDirectory() && name.startsWith(toRemove)) {
-                    String shortenedName = name.substring(toRemove.length());
-                    Path targetFile = dest.resolve(shortenedName);
-                    LOG.debug("EXTRACTING {} SHORTENED to {} into {}", name, shortenedName, targetFile);
-                    fsOps.forceMkdir(targetFile.getParent());
-                    try (FileOutputStream out = new FileOutputStream(targetFile.toFile());
-                         InputStream in = jarFile.getInputStream(entry)) {
-                        IOUtils.copy(in, out);
-                    }
-                } else {
-                    LOG.debug("Skipping {}", entry);
-                }
-            }
+            String prefix = dir + '/';
+            ServerUtils.extractZipFile(jarFile, dest.toFile(), prefix);
         }
     }
 

storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java

@@ -227,17 +227,6 @@ public static void downloadResourcesAsSupervisor(String key, String localFile,
         _instance.downloadResourcesAsSupervisorImpl(key, localFile, cb);
     }
 
-    /**
-     * Extract dir from the jar to destdir
-     *
-     * @param jarpath Path to the jar file
-     * @param dir     Directory in the jar to pull out
-     * @param destdir Path to the directory where the extracted directory will be put
-     */
-    public static void extractDirFromJar(String jarpath, String dir, File destdir) {
-        _instance.extractDirFromJarImpl(jarpath, dir, destdir);
-    }
-
     /**
      * Returns the value of java.class.path System property. Kept separate for testing.
      *
@@ -583,8 +572,17 @@ public static void unpack(File localrsrc, File dst, boolean symLinksDisabled) th
             localrsrc.delete();
         }
     }
-
-    private static void extractZipFile(ZipFile zipFile, File toDir, String prefix) throws IOException {
+
+    /**
+     * Extracts the given file to the given directory. Only zip entries starting with the given prefix are extracted.
+     * The prefix is stripped off entry names before extraction.
+     *
+     * @param zipFile The zip file to extract.
+     * @param toDir The directory to extract to.
+     * @param prefix The prefix to look for in the zip file. If not null only paths starting with the prefix will be
+     * extracted.
+     */
+    public static void extractZipFile(ZipFile zipFile, File toDir, String prefix) throws IOException {
         ensureDirectory(toDir);
         final String base = toDir.getCanonicalPath();
 
@@ -596,7 +594,14 @@ private static void extractZipFile(ZipFile zipFile, File toDir, String prefix) t
                     //No need to extract it, it is not what we are looking for.
                     continue;
                 }
-                File file = new File(toDir, entry.getName());
+                String entryName;
+                if (prefix != null) {
+                    entryName = entry.getName().substring(prefix.length());
+                    LOG.debug("Extracting {} shortened to {} into {}", entry.getName(), entryName, toDir);
+                } else {
+                    entryName = entry.getName();
+                }
+                File file = new File(toDir, entryName);
                 String found = file.getCanonicalPath();
                 if (!found.startsWith(base)) {
                     LOG.error("Invalid location {} is outside of {}", found, base);
@@ -748,14 +753,6 @@ public String currentClasspathImpl() {
         return System.getProperty("java.class.path");
     }
 
-    public void extractDirFromJarImpl(String jarpath, String dir, File destdir) {
-        try (JarFile jarFile = new JarFile(jarpath)) {
-            extractZipFile(jarFile, destdir, dir);
-        } catch (IOException e) {
-            LOG.info("Could not extract {} from {}", dir, jarpath);
-        }
-    }
-
     public void downloadResourcesAsSupervisorImpl(String key, String localFile,
                                                   ClientBlobStore cb) throws AuthorizationException, KeyNotFoundException, IOException {
         final int MAX_RETRY_ATTEMPTS = 2;

storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java

@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 org.apache.storm.utils;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.zip.ZipFile;
+import org.apache.storm.testing.TmpPath;
+import org.junit.jupiter.api.Test;
+
+public class ServerUtilsTest {
+
+    @Test
+    public void testExtractZipFileDisallowsPathTraversal() throws Exception {
+        try (TmpPath path = new TmpPath()) {
+            Path testRoot = Paths.get(path.getPath());
+            Path extractionDest = testRoot.resolve("dest");
+            Files.createDirectories(extractionDest);
+
+            /**
+             * Contains good.txt and ../evil.txt. Evil.txt will path outside the target dir, and should not be extracted.
+             */
+            try (ZipFile zip = new ZipFile(Paths.get("src/test/resources/evil-path-traversal.jar").toFile())) {
+                ServerUtils.extractZipFile(zip, extractionDest.toFile(), null);
+            }
+
+            assertThat(Files.exists(extractionDest.resolve("good.txt")), is(true));
+            assertThat(Files.exists(testRoot.resolve("evil.txt")), is(false));
+        }
+    }
+
+    @Test
+    public void testExtractZipFileDisallowsPathTraversalWhenUsingPrefix() throws Exception {
+        try (TmpPath path = new TmpPath()) {
+            Path testRoot = Paths.get(path.getPath());
+            Path destParent = testRoot.resolve("outer");
+            Path extractionDest = destParent.resolve("resources");
+            Files.createDirectories(extractionDest);
+
+            /**
+             * Contains resources/good.txt and resources/../evil.txt. Evil.txt should not be extracted as it would end
+             * up outside the extraction dest.
+             */
+            try (ZipFile zip = new ZipFile(Paths.get("src/test/resources/evil-path-traversal-resources.jar").toFile())) {
+                ServerUtils.extractZipFile(zip, extractionDest.toFile(), "resources");
+            }
+
+            assertThat(Files.exists(extractionDest.resolve("good.txt")), is(true));
+            assertThat(Files.exists(extractionDest.resolve("evil.txt")), is(false));
+            assertThat(Files.exists(destParent.resolve("evil.txt")), is(false));
+        }
+    }
+}

storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogPageHandler.java

@@ -49,8 +49,10 @@
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -82,8 +84,8 @@ public class LogviewerLogPageHandler {
     private final Meter numPageRead;
     private final Meter numFileOpenExceptions;
     private final Meter numFileReadExceptions;
-    private final String logRoot;
-    private final String daemonLogRoot;
+    private final Path logRoot;
+    private final Path daemonLogRoot;
     private final WorkerLogs workerLogs;
     private final ResourceAuthorizer resourceAuthorizer;
     private final DirectoryCleaner directoryCleaner;
@@ -101,8 +103,8 @@ public LogviewerLogPageHandler(String logRoot, String daemonLogRoot,
                                    WorkerLogs workerLogs,
                                    ResourceAuthorizer resourceAuthorizer,
                                    StormMetricsRegistry metricsRegistry) {
-        this.logRoot = logRoot;
-        this.daemonLogRoot = daemonLogRoot;
+        this.logRoot = Paths.get(logRoot).toAbsolutePath().normalize();
+        this.daemonLogRoot = Paths.get(daemonLogRoot).toAbsolutePath().normalize();
         this.workerLogs = workerLogs;
         this.resourceAuthorizer = resourceAuthorizer;
         this.numPageRead = metricsRegistry.registerMeter("logviewer:num-page-read");
@@ -129,7 +131,7 @@ public Response listLogFiles(String user, Integer port, String topologyId, Strin
             } else {
                 fileResults = new ArrayList<>();
 
-                File[] logRootFiles = new File(logRoot).listFiles();
+                File[] logRootFiles = logRoot.toFile().listFiles();
                 if (logRootFiles != null) {
                     for (File topoDir : logRootFiles) {
                         File[] topoDirFiles = topoDir.listFiles();
@@ -147,9 +149,12 @@ public Response listLogFiles(String user, Integer port, String topologyId, Strin
             if (port == null) {
                 fileResults = new ArrayList<>();
 
-                File topoDir = new File(logRoot, topologyId);
-                if (topoDir.exists()) {
-                    File[] topoDirFiles = topoDir.listFiles();
+                Path topoDir = logRoot.resolve(topologyId).toAbsolutePath().normalize();
+                if (!topoDir.startsWith(logRoot)) {
+                    return LogviewerResponseBuilder.buildSuccessJsonResponse(Collections.emptyList(), callback, origin);
+                }
+                if (topoDir.toFile().exists()) {
+                    File[] topoDirFiles = topoDir.toFile().listFiles();
                     if (topoDirFiles != null) {
                         for (File portDir : topoDirFiles) {
                             fileResults.addAll(directoryCleaner.getFilesForDir(portDir.toPath()));
@@ -158,7 +163,10 @@ public Response listLogFiles(String user, Integer port, String topologyId, Strin
                 }
 
             } else {
-                File portDir = ConfigUtils.getWorkerDirFromRoot(logRoot, topologyId, port);
+                File portDir = ConfigUtils.getWorkerDirFromRoot(logRoot.toString(), topologyId, port).getCanonicalFile();
+                if (!portDir.getPath().startsWith(logRoot.toString())) {
+                    return LogviewerResponseBuilder.buildSuccessJsonResponse(Collections.emptyList(), callback, origin);
+                }
                 if (portDir.exists()) {
                     fileResults = directoryCleaner.getFilesForDir(portDir.toPath());
                 }
@@ -190,21 +198,24 @@ public Response listLogFiles(String user, Integer port, String topologyId, Strin
      */
     public Response logPage(String fileName, Integer start, Integer length, String grep, String user)
             throws IOException, InvalidRequestException {
-        String rootDir = logRoot;
+        Path rawFile = logRoot.resolve(fileName);
+        Path absFile = rawFile.toAbsolutePath().normalize();
+        if (!absFile.startsWith(logRoot) || !rawFile.normalize().toString().equals(rawFile.toString())) {
+            //Ensure filename doesn't contain ../ parts 
+            return LogviewerResponseBuilder.buildResponsePageNotFound();
+        }
+
         if (resourceAuthorizer.isUserAllowedToAccessFile(user, fileName)) {
             workerLogs.setLogFilePermission(fileName);
 
-            File file = new File(rootDir, fileName).getCanonicalFile();
-            String path = file.getCanonicalPath();
-            File topoDir = file.getParentFile().getParentFile();
-
-            if (file.exists() && new File(rootDir).getCanonicalFile().equals(topoDir.getParentFile())) {
+            Path topoDir = absFile.getParent().getParent();
+            if (absFile.toFile().exists()) {
                 SortedSet<Path> logFiles;
                 try {
-                    logFiles = Arrays.stream(topoDir.listFiles())
+                    logFiles = Arrays.stream(topoDir.toFile().listFiles())
                         .flatMap(Unchecked.function(portDir -> directoryCleaner.getFilesForDir(portDir.toPath()).stream()))
                         .filter(Files::isRegularFile)
-                        .collect(toCollection(TreeSet::new));
+                            .collect(toCollection(TreeSet::new));
                 } catch (UncheckedIOException e) {
                     throw e.getCause();
                 }
@@ -216,13 +227,13 @@ public Response logPage(String fileName, Integer start, Integer length, String g
                 reorderedFilesStr.add(fileName);
 
                 length = length != null ? Math.min(10485760, length) : LogviewerConstant.DEFAULT_BYTES_PER_PAGE;
-                final boolean isZipFile = path.endsWith(".gz");
-                long fileLength = getFileLength(file, isZipFile);
+                final boolean isZipFile = absFile.getFileName().toString().endsWith(".gz");
+                long fileLength = getFileLength(absFile.toFile(), isZipFile);
                 if (start == null) {
                     start = Long.valueOf(fileLength - length).intValue();
                 }
 
-                String logString = isTxtFile(fileName) ? escapeHtml(pageFile(path, isZipFile, fileLength, start, length)) :
+                String logString = isTxtFile(fileName) ? escapeHtml(pageFile(absFile.toString(), isZipFile, fileLength, start, length)) :
                     escapeHtml("This is a binary file and cannot display! You may download the full file.");
 
                 List<DomContent> bodyContents = new ArrayList<>();
@@ -275,13 +286,15 @@ public Response logPage(String fileName, Integer start, Integer length, String g
      */
     public Response daemonLogPage(String fileName, Integer start, Integer length, String grep, String user)
             throws IOException, InvalidRequestException {
-        String rootDir = daemonLogRoot;
-        File file = new File(rootDir, fileName).getCanonicalFile();
-        String path = file.getCanonicalPath();
+        Path file = daemonLogRoot.resolve(fileName).toAbsolutePath().normalize();
+        if (!file.startsWith(daemonLogRoot) || Paths.get(fileName).getNameCount() != 1) {
+            //Prevent fileName from pathing into worker logs, or outside daemon log root 
+            return LogviewerResponseBuilder.buildResponsePageNotFound();
+        }
 
-        if (file.exists() && new File(rootDir).getCanonicalFile().equals(file.getParentFile())) {
+        if (file.toFile().exists()) {
             // all types of files included
-            List<File> logFiles = Arrays.stream(new File(rootDir).listFiles())
+            List<File> logFiles = Arrays.stream(daemonLogRoot.toFile().listFiles())
                     .filter(File::isFile)
                     .collect(toList());
 
@@ -292,13 +305,13 @@ public Response daemonLogPage(String fileName, Integer start, Integer length, St
             reorderedFilesStr.add(fileName);
 
             length = length != null ? Math.min(10485760, length) : LogviewerConstant.DEFAULT_BYTES_PER_PAGE;
-            final boolean isZipFile = path.endsWith(".gz");
-            long fileLength = getFileLength(file, isZipFile);
+            final boolean isZipFile = file.getFileName().toString().endsWith(".gz");
+            long fileLength = getFileLength(file.toFile(), isZipFile);
             if (start == null) {
                 start = Long.valueOf(fileLength - length).intValue();
             }
 
-            String logString = isTxtFile(fileName) ? escapeHtml(pageFile(path, isZipFile, fileLength, start, length)) :
+            String logString = isTxtFile(fileName) ? escapeHtml(pageFile(file.toString(), isZipFile, fileLength, start, length)) :
                     escapeHtml("This is a binary file and cannot display! You may download the full file.");
 
             List<DomContent> bodyContents = new ArrayList<>();