From e909b3d604367e7c47c3bbf3ec8e7f6b672ff778 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stig=20Rohde=20D=C3=B8ssing?= Date: Thu, 21 Feb 2019 11:29:57 +0100 Subject: [PATCH] STORM-3330: More storm-webapp path cleanup --- .gitignore | 6 +- .../localizer/LocallyCachedTopologyBlob.java | 20 +- .../org/apache/storm/utils/ServerUtils.java | 41 ++- .../apache/storm/utils/ServerUtilsTest.java | 74 ++++++ .../evil-path-traversal-resources.jar | Bin 0 -> 270 bytes .../test/resources/evil-path-traversal.jar | Bin 0 -> 230 bytes .../handler/LogviewerLogPageHandler.java | 69 +++-- .../handler/LogviewerLogSearchHandler.java | 129 +++++----- .../handler/LogviewerProfileHandler.java | 60 +++-- .../logviewer/utils/LogFileDownloader.java | 30 ++- .../utils/LogviewerResponseBuilder.java | 3 +- .../logviewer/utils/ResourceAuthorizer.java | 20 +- .../daemon/logviewer/utils/WorkerLogs.java | 13 +- .../LogviewerLogDownloadHandlerTest.java | 142 +++++++++++ .../handler/LogviewerLogPageHandlerTest.java | 96 ++++++- .../LogviewerLogSearchHandlerTest.java | 239 +++++++++--------- .../handler/LogviewerProfileHandlerTest.java | 165 ++++++++++++ .../logviewer/utils/LogCleanerTest.java | 35 +-- .../utils/ResourceAuthorizerTest.java | 22 +- 19 files changed, 834 insertions(+), 330 deletions(-) create mode 100644 storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java create mode 100644 storm-server/src/test/resources/evil-path-traversal-resources.jar create mode 100644 storm-server/src/test/resources/evil-path-traversal.jar create mode 100644 storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java create mode 100644 storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java diff --git a/.gitignore b/.gitignore index 636570f7e76..35d0e6017f6 100644 --- a/.gitignore +++ b/.gitignore @@ -56,4 +56,8 @@ logs hs_err_pid* # ignore vagrant files -/integration-test/config/.vagrant/ \ No newline at end of file +/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 \ No newline at end of file diff --git a/storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java b/storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java index ae0a8a9ed74..48ca064ad74 100644 --- a/storm-server/src/main/java/org/apache/storm/localizer/LocallyCachedTopologyBlob.java +++ b/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 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); } } diff --git a/storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java b/storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java index 615aead12ab..85ade016ed3 100644 --- a/storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java +++ b/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; diff --git a/storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java b/storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java new file mode 100644 index 00000000000..2dc63e09e81 --- /dev/null +++ b/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)); + } + } +} diff --git a/storm-server/src/test/resources/evil-path-traversal-resources.jar b/storm-server/src/test/resources/evil-path-traversal-resources.jar new file mode 100644 index 0000000000000000000000000000000000000000..ad3655ab153c5cb44fb8212789af3320e5c0203b GIT binary patch literal 270 zcmWIWW@Zs#0D(oZ!G0%iME~XlvO!oBh>KE-^Gl18Q;YTW^z>89GIR7wDoQ-na}_{r zt^l~+RY1LLsrM3KdWBH*rswCUz%+u`K#hz{BFwn01?mQa|BWCDt0Rz&z-Bwd00xHt ajW#Goz}ynx&B_K+!vuuQK)MvfVE_QHhC9*# literal 0 HcmV?d00001 diff --git a/storm-server/src/test/resources/evil-path-traversal.jar b/storm-server/src/test/resources/evil-path-traversal.jar new file mode 100644 index 0000000000000000000000000000000000000000..4ee509ee2605d0c5941a6b80d5301b74c253702c GIT binary patch literal 230 zcmWIWW@Zs#0D(oZ!G0%iME~XlvO$;|i1qaJQ_C`Q^hzp9Jk@g*Ky0o6xQ(); - 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 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 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 logFiles = Arrays.stream(new File(rootDir).listFiles()) + List 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 bodyContents = new ArrayList<>(); diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java index 49c1590abf9..ab0ba188c19 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandler.java @@ -80,6 +80,7 @@ import org.slf4j.LoggerFactory; public class LogviewerLogSearchHandler { + private static final Logger LOG = LoggerFactory.getLogger(LogviewerLogSearchHandler.class); public static final int GREP_MAX_SEARCH_SIZE = 1024; public static final int GREP_BUF_SIZE = 2048; @@ -91,7 +92,7 @@ public class LogviewerLogSearchHandler { private final Meter numSearchRequestNoResult; private final Meter numFileOpenExceptions; private final Meter numFileReadExceptions; - + private final Map stormConf; private final Path logRoot; private final Path daemonLogRoot; @@ -110,10 +111,10 @@ public class LogviewerLogSearchHandler { * @param metricsRegistry The logviewer metrics registry */ public LogviewerLogSearchHandler(Map stormConf, Path logRoot, Path daemonLogRoot, - ResourceAuthorizer resourceAuthorizer, StormMetricsRegistry metricsRegistry) { + ResourceAuthorizer resourceAuthorizer, StormMetricsRegistry metricsRegistry) { this.stormConf = stormConf; - this.logRoot = logRoot; - this.daemonLogRoot = daemonLogRoot; + this.logRoot = logRoot.toAbsolutePath().normalize(); + this.daemonLogRoot = daemonLogRoot.toAbsolutePath().normalize(); this.resourceAuthorizer = resourceAuthorizer; Object httpsPort = stormConf.get(DaemonConfig.LOGVIEWER_HTTPS_PORT); if (httpsPort == null) { @@ -145,14 +146,23 @@ public LogviewerLogSearchHandler(Map stormConf, Path logRoot, Pa * @return Response containing JSON content representing search result */ public Response searchLogFile(String fileName, String user, boolean isDaemon, String search, - String numMatchesStr, String offsetStr, String callback, String origin) - throws IOException, InvalidRequestException { + String numMatchesStr, String offsetStr, String callback, String origin) + throws IOException, InvalidRequestException { boolean noResult = true; Path rootDir = isDaemon ? daemonLogRoot : logRoot; - Path file = rootDir.resolve(fileName).toAbsolutePath().normalize(); + Path rawFile = rootDir.resolve(fileName); + Path absFile = rawFile.toAbsolutePath().normalize(); + if (!absFile.startsWith(rootDir) || !rawFile.normalize().toString().equals(rawFile.toString())) { + //Ensure filename doesn't contain ../ parts + return searchLogFileNotFound(callback); + } + if (isDaemon && Paths.get(fileName).getNameCount() != 1) { + //Don't permit path traversal for calls intended to read from the daemon logs + return searchLogFileNotFound(callback); + } Response response; - if (file.toFile().exists()) { + if (absFile.toFile().exists()) { if (isDaemon || resourceAuthorizer.isUserAllowedToAccessFile(user, fileName)) { Integer numMatchesInt = numMatchesStr != null ? tryParseIntParam("num-matches", numMatchesStr) : null; Integer offsetInt = offsetStr != null ? tryParseIntParam("start-byte-offset", offsetStr) : null; @@ -161,14 +171,14 @@ public Response searchLogFile(String fileName, String user, boolean isDaemon, St if (StringUtils.isNotEmpty(search) && search.getBytes("UTF-8").length <= GREP_MAX_SEARCH_SIZE) { Map entity = new HashMap<>(); entity.put("isDaemon", isDaemon ? "yes" : "no"); - Map res = substringSearch(file, search, isDaemon, numMatchesInt, offsetInt); + Map res = substringSearch(absFile, search, isDaemon, numMatchesInt, offsetInt); entity.putAll(res); noResult = ((List) res.get("matches")).isEmpty(); response = LogviewerResponseBuilder.buildSuccessJsonResponse(entity, callback, origin); } else { throw new InvalidRequestException("Search substring must be between 1 and 1024 " - + "UTF-8 bytes in size (inclusive)"); + + "UTF-8 bytes in size (inclusive)"); } } catch (Exception ex) { response = LogviewerResponseBuilder.buildExceptionJsonResponse(ex, callback); @@ -178,12 +188,7 @@ public Response searchLogFile(String fileName, String user, boolean isDaemon, St response = LogviewerResponseBuilder.buildUnauthorizedUserJsonResponse(user, callback); } } else { - // not found - Map entity = new HashMap<>(); - entity.put("error", "Not Found"); - entity.put("errorMessage", "The file was not found on this node."); - - response = new JsonResponseBuilder().setData(entity).setCallback(callback).setStatus(404).build(); + response = searchLogFileNotFound(callback); } if (noResult) { @@ -192,14 +197,22 @@ public Response searchLogFile(String fileName, String user, boolean isDaemon, St return response; } + private Response searchLogFileNotFound(String callback) { + Map entity = new HashMap<>(); + entity.put("error", "Not Found"); + entity.put("errorMessage", "The file was not found on this node."); + + return new JsonResponseBuilder().setData(entity).setCallback(callback).setStatus(404).build(); + } + /** * Advanced search across worker log files in a topology. * * @param topologyId topology ID * @param user username * @param search search string - * @param numMatchesStr the count of maximum matches. Note that this number is with respect to - * each port, not to each log or each search request + * @param numMatchesStr the count of maximum matches. Note that this number is with respect to each port, not to each log or each search + * request * @param portStr worker port, null or '*' if the request wants to search from all worker logs * @param fileOffsetStr index (offset) of the log files * @param offsetStr start offset for log file @@ -209,15 +222,15 @@ public Response searchLogFile(String fileName, String user, boolean isDaemon, St * @return Response containing JSON content representing search result */ public Response deepSearchLogsForTopology(String topologyId, String user, String search, - String numMatchesStr, String portStr, String fileOffsetStr, String offsetStr, - Boolean searchArchived, String callback, String origin) throws IOException { + String numMatchesStr, String portStr, String fileOffsetStr, String offsetStr, + Boolean searchArchived, String callback, String origin) throws IOException { int numMatchedFiles = 0; int numScannedFiles = 0; Path rootDir = logRoot; + Path absTopoDir = rootDir.resolve(topologyId).toAbsolutePath().normalize(); Object returnValue; - Path topologyDir = rootDir.resolve(topologyId); - if (StringUtils.isEmpty(search) || !topologyDir.toFile().exists()) { + if (StringUtils.isEmpty(search) || !absTopoDir.toFile().exists() || !absTopoDir.startsWith(rootDir)) { returnValue = new ArrayList<>(); } else { int fileOffset = ObjectReader.getInt(fileOffsetStr, 0); @@ -225,7 +238,7 @@ public Response deepSearchLogsForTopology(String topologyId, String user, String int numMatches = ObjectReader.getInt(numMatchesStr, 1); if (StringUtils.isEmpty(portStr) || portStr.equals("*")) { - try (Stream topoDir = Files.list(topologyDir)) { + try (Stream topoDir = Files.list(absTopoDir)) { // check for all ports Stream> portsOfLogs = topoDir .map(portDir -> logsForPort(user, portDir)) @@ -247,18 +260,19 @@ public Response deepSearchLogsForTopology(String topologyId, String user, String // check just the one port @SuppressWarnings("unchecked") List slotsPorts = (List) stormConf.getOrDefault(DaemonConfig.SUPERVISOR_SLOTS_PORTS, - new ArrayList<>()); + new ArrayList<>()); boolean containsPort = slotsPorts.stream() - .anyMatch(slotPort -> slotPort != null && (slotPort == port)); + .anyMatch(slotPort -> slotPort != null && (slotPort == port)); if (!containsPort) { returnValue = new ArrayList<>(); } else { - Path portDir = rootDir.resolve(topologyId).resolve(Integer.toString(port)); + Path absPortDir = absTopoDir.resolve(Integer.toString(port)).toAbsolutePath().normalize(); - if (!portDir.toFile().exists() || logsForPort(user, portDir).isEmpty()) { + if (!absPortDir.toFile().exists() + || !absPortDir.startsWith(absTopoDir)) { returnValue = new ArrayList<>(); } else { - List filteredLogs = logsForPort(user, portDir); + List filteredLogs = logsForPort(user, absPortDir); if (BooleanUtils.isNotTrue(searchArchived)) { filteredLogs = Collections.singletonList(first(filteredLogs)); fileOffset = 0; @@ -287,22 +301,22 @@ private Integer tryParseIntParam(String paramName, String value) throws InvalidR } @VisibleForTesting - Map substringSearch(Path file, String searchString) throws InvalidRequestException { + Map substringSearch(Path file, String searchString) throws InvalidRequestException { return substringSearch(file, searchString, false, 10, 0); } @VisibleForTesting - Map substringSearch(Path file, String searchString, int numMatches) throws InvalidRequestException { + Map substringSearch(Path file, String searchString, int numMatches) throws InvalidRequestException { return substringSearch(file, searchString, false, numMatches, 0); } @VisibleForTesting - Map substringSearch(Path file, String searchString, int numMatches, int startByteOffset) throws InvalidRequestException { + Map substringSearch(Path file, String searchString, int numMatches, int startByteOffset) throws InvalidRequestException { return substringSearch(file, searchString, false, numMatches, startByteOffset); } - private Map substringSearch(Path file, String searchString, boolean isDaemon, Integer numMatches, - Integer startByteOffset) throws InvalidRequestException { + private Map substringSearch(Path file, String searchString, boolean isDaemon, Integer numMatches, + Integer startByteOffset) throws InvalidRequestException { if (StringUtils.isEmpty(searchString)) { throw new IllegalArgumentException("Precondition fails: search string should not be empty."); } @@ -314,7 +328,7 @@ private Map substringSearch(Path file, String searchString, boole boolean isZipFile = file.toString().endsWith(".gz"); try (InputStream fis = Files.newInputStream(file)) { try (InputStream gzippedInputStream = isZipFile ? new GZIPInputStream(fis) : fis; - BufferedInputStream stream = new BufferedInputStream(gzippedInputStream)) { + BufferedInputStream stream = new BufferedInputStream(gzippedInputStream)) { //It's more likely to be a file read exception here, so we don't differentiate int fileLength = isZipFile ? (int) ServerUtils.zipFileSize(file.toFile()) : (int) Files.size(file); @@ -398,7 +412,7 @@ private Map substringSearch(Path file, String searchString, boole } @VisibleForTesting - Map substringSearchDaemonLog(Path file, String searchString) throws InvalidRequestException { + Map substringSearchDaemonLog(Path file, String searchString) throws InvalidRequestException { return substringSearch(file, searchString, true, 10, 0); } @@ -409,8 +423,8 @@ Map substringSearchDaemonLog(Path file, String searchString) thro List logsForPort(String user, Path portDir) { try { List workerLogs = directoryCleaner.getFilesForDir(portDir).stream() - .filter(file -> WORKER_LOG_FILENAME_PATTERN.asPredicate().test(file.getFileName().toString())) - .collect(toList()); + .filter(file -> WORKER_LOG_FILENAME_PATTERN.asPredicate().test(file.getFileName().toString())) + .collect(toList()); return workerLogs.stream() .filter(log -> resourceAuthorizer.isUserAllowedToAccessFile(user, WorkerLogs.getTopologyPortWorkerLog(log))) @@ -425,6 +439,7 @@ List logsForPort(String user, Path portDir) { /** * Find the first N matches of target string in files. + * * @param logs all candidate log files to search * @param numMatches number of matches expected * @param fileOffset number of log files to skip initially @@ -468,7 +483,7 @@ Matched findNMatches(List logs, int numMatches, int fileOffset, int startB currentFileMatch.put("port", truncatePathToLastElements(firstLogAbsPath, 2).getName(0).toString()); newMatches.add(currentFileMatch); - int newCount = matchCount + ((List)matchInLog.getOrDefault("matches", Collections.emptyList())).size(); + int newCount = matchCount + ((List) matchInLog.getOrDefault("matches", Collections.emptyList())).size(); if (newCount == matchCount) { // matches and matchCount is not changed logs = rest(logs); @@ -491,16 +506,15 @@ Matched findNMatches(List logs, int numMatches, int fileOffset, int startB return new Matched(fileOffset, targetStr, matches, scannedFiles); } - /** - * As the file is read into a buffer, 1/2 the buffer's size at a time, we search the buffer for matches of the - * substring and return a list of zero or more matches. + * As the file is read into a buffer, 1/2 the buffer's size at a time, we search the buffer for matches of the substring and return a + * list of zero or more matches. */ private SubstringSearchResult bufferSubstringSearch(boolean isDaemon, Path file, int fileLength, int offsetToBuf, - int initBufOffset, BufferedInputStream stream, Integer bytesSkipped, - int bytesRead, ByteBuffer haystack, byte[] needle, - List> initialMatches, Integer numMatches, byte[] beforeBytes) - throws IOException { + int initBufOffset, BufferedInputStream stream, Integer bytesSkipped, + int bytesRead, ByteBuffer haystack, byte[] needle, + List> initialMatches, Integer numMatches, byte[] beforeBytes) + throws IOException { int bufOffset = initBufOffset; List> matches = initialMatches; @@ -525,7 +539,7 @@ private SubstringSearchResult bufferSubstringSearch(boolean isDaemon, Path file, bufOffset = offset + needle.length; matches.add(mkMatchData(needle, haystack, offset, fileOffset, - file.toAbsolutePath().normalize(), isDaemon, beforeArg, afterArg)); + file.toAbsolutePath().normalize(), isDaemon, beforeArg, afterArg)); } else { int beforeStrToOffset = Math.min(haystack.limit(), GREP_MAX_SEARCH_SIZE); int beforeStrFromOffset = Math.max(0, beforeStrToOffset - GREP_CONTEXT_SIZE); @@ -561,10 +575,9 @@ private int rotateGrepBuffer(ByteBuffer buf, BufferedInputStream stream, int tot return totalBytesRead + bytesRead; } - private Map mkMatchData(byte[] needle, ByteBuffer haystack, int haystackOffset, int fileOffset, Path canonicalPath, - boolean isDaemon, byte[] beforeBytes, byte[] afterBytes) - throws UnsupportedEncodingException, UnknownHostException { + boolean isDaemon, byte[] beforeBytes, byte[] afterBytes) + throws UnsupportedEncodingException, UnknownHostException { String url; if (isDaemon) { url = urlToMatchCenteredInLogPageDaemonFile(needle, canonicalPath, fileOffset, logviewerPort); @@ -625,11 +638,10 @@ private Map mkMatchData(byte[] needle, ByteBuffer haystack, int } /** - * Tries once to read ahead in the stream to fill the context and - * resets the stream to its position before the call. + * Tries once to read ahead in the stream to fill the context and resets the stream to its position before the call. */ private byte[] tryReadAhead(BufferedInputStream stream, ByteBuffer haystack, int offset, int fileLength, int bytesRead) - throws IOException { + throws IOException { int numExpected = Math.min(fileLength - bytesRead, GREP_CONTEXT_SIZE); byte[] afterBytes = new byte[numExpected]; stream.mark(numExpected); @@ -640,8 +652,8 @@ private byte[] tryReadAhead(BufferedInputStream stream, ByteBuffer haystack, int } /** - * Searches a given byte array for a match of a sub-array of bytes. - * Returns the offset to the byte that matches, or -1 if no match was found. + * Searches a given byte array for a match of a sub-array of bytes. Returns the offset to the byte that matches, or -1 if no match was + * found. */ private int offsetOfBytes(byte[] buffer, byte[] search, int initOffset) { if (search.length <= 0) { @@ -692,7 +704,7 @@ private int offsetOfBytes(byte[] buffer, byte[] search, int initOffset) { * This response data only includes a next byte offset if there is more of the file to read. */ private Map mkGrepResponse(byte[] searchBytes, Integer offset, List> matches, - Integer nextByteOffset) throws UnsupportedEncodingException { + Integer nextByteOffset) throws UnsupportedEncodingException { Map ret = new HashMap<>(); ret.put("searchString", new String(searchBytes, "UTF-8")); ret.put("startByteOffset", offset); @@ -702,7 +714,7 @@ private Map mkGrepResponse(byte[] searchBytes, Integer offset, L } return ret; } - + @VisibleForTesting String urlToMatchCenteredInLogPage(byte[] needle, Path canonicalPath, int offset, Integer port) throws UnknownHostException { final String host = Utils.hostname(); @@ -731,6 +743,7 @@ String urlToMatchCenteredInLogPageDaemonFile(byte[] needle, Path canonicalPath, @VisibleForTesting public static class Matched implements JSONAware { + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); private int fileOffset; @@ -741,7 +754,8 @@ public static class Matched implements JSONAware { /** * Constructor. - * @param fileOffset offset (index) of the files + * + * @param fileOffset offset (index) of the files * @param searchString search string * @param matches map representing matched search result * @param openedFiles number of files scanned, used for metrics only @@ -776,6 +790,7 @@ public String toJSONString() { } private static class SubstringSearchResult { + private List> matches; private Integer newByteOffset; private byte[] newBeforeBytes; diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java index 757bbe2a080..47c6ae3c2b2 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java @@ -34,6 +34,7 @@ import java.io.File; import java.io.IOException; import java.nio.file.Path; +import java.nio.file.Paths; import java.util.List; import javax.ws.rs.core.Response; @@ -47,10 +48,10 @@ public class LogviewerProfileHandler { public static final String WORKER_LOG_FILENAME = "worker.log"; - + private final Meter numFileDownloadExceptions; - - private final String logRoot; + + private final Path logRoot; private final ResourceAuthorizer resourceAuthorizer; private final DirectoryCleaner directoryCleaner; @@ -62,7 +63,7 @@ public class LogviewerProfileHandler { * @param metricsRegistry The logviewer metrisc registry */ public LogviewerProfileHandler(String logRoot, ResourceAuthorizer resourceAuthorizer, StormMetricsRegistry metricsRegistry) { - this.logRoot = logRoot; + this.logRoot = Paths.get(logRoot).toAbsolutePath().normalize(); this.resourceAuthorizer = resourceAuthorizer; this.numFileDownloadExceptions = metricsRegistry.registerMeter(ExceptionMeterNames.NUM_FILE_DOWNLOAD_EXCEPTIONS); this.directoryCleaner = new DirectoryCleaner(metricsRegistry); @@ -78,12 +79,17 @@ public LogviewerProfileHandler(String logRoot, ResourceAuthorizer resourceAuthor */ public Response listDumpFiles(String topologyId, String hostPort, String user) throws IOException { String portStr = hostPort.split(":")[1]; - File dir = new File(String.join(File.separator, logRoot, topologyId, portStr)); + Path rawDir = logRoot.resolve(topologyId).resolve(portStr); + Path absDir = rawDir.toAbsolutePath().normalize(); + if (!absDir.startsWith(logRoot) || !rawDir.normalize().toString().equals(rawDir.toString())) { + //Ensure filename doesn't contain ../ parts + return LogviewerResponseBuilder.buildResponsePageNotFound(); + } - if (dir.exists()) { + if (absDir.toFile().exists()) { String workerFileRelativePath = String.join(File.separator, topologyId, portStr, WORKER_LOG_FILENAME); if (resourceAuthorizer.isUserAllowedToAccessFile(user, workerFileRelativePath)) { - String content = buildDumpFileListPage(topologyId, hostPort, dir); + String content = buildDumpFileListPage(topologyId, hostPort, absDir.toFile()); return LogviewerResponseBuilder.buildSuccessHtmlResponse(content); } else { return LogviewerResponseBuilder.buildResponseUnauthorizedUser(user); @@ -105,13 +111,17 @@ public Response listDumpFiles(String topologyId, String hostPort, String user) t */ public Response downloadDumpFile(String topologyId, String hostPort, String fileName, String user) throws IOException { String portStr = hostPort.split(":")[1]; - File dir = new File(String.join(File.separator, logRoot, topologyId, portStr)); - File file = new File(dir, fileName); + Path rawFile = logRoot.resolve(topologyId).resolve(portStr).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 (dir.exists() && file.exists()) { + if (absFile.toFile().exists()) { String workerFileRelativePath = String.join(File.separator, topologyId, portStr, WORKER_LOG_FILENAME); if (resourceAuthorizer.isUserAllowedToAccessFile(user, workerFileRelativePath)) { - return LogviewerResponseBuilder.buildDownloadFile(file, numFileDownloadExceptions); + return LogviewerResponseBuilder.buildDownloadFile(absFile.toFile(), numFileDownloadExceptions); } else { return LogviewerResponseBuilder.buildResponseUnauthorizedUser(user); } @@ -122,19 +132,19 @@ public Response downloadDumpFile(String topologyId, String hostPort, String file private String buildDumpFileListPage(String topologyId, String hostPort, File dir) throws IOException { List liTags = getProfilerDumpFiles(dir).stream() - .map(file -> li(a(file).withHref("/api/v1/dumps/" + topologyId + "/" + hostPort + "/" + file))) - .collect(toList()); + .map(file -> li(a(file).withHref("/api/v1/dumps/" + topologyId + "/" + hostPort + "/" + file))) + .collect(toList()); return html( - head( - title("File Dumps - Storm Log Viewer"), - link().withRel("stylesheet").withHref("/css/bootstrap-3.3.1.min.css"), - link().withRel("stylesheet").withHref("/css/jquery.dataTables.1.10.4.min.css"), - link().withRel("stylesheet").withHref("/css/style.css") - ), - body( - ul(liTags.toArray(new DomContent[]{})) - ) + head( + title("File Dumps - Storm Log Viewer"), + link().withRel("stylesheet").withHref("/css/bootstrap-3.3.1.min.css"), + link().withRel("stylesheet").withHref("/css/jquery.dataTables.1.10.4.min.css"), + link().withRel("stylesheet").withHref("/css/style.css") + ), + body( + ul(liTags.toArray(new DomContent[]{})) + ) ).render(); } @@ -143,10 +153,10 @@ private List getProfilerDumpFiles(File dir) throws IOException { return filesForDir.stream() .map(path -> path.toFile()) .filter(file -> { - String fileName = file.getName(); - return StringUtils.isNotEmpty(fileName) + String fileName = file.getName(); + return StringUtils.isNotEmpty(fileName) && (fileName.endsWith(".txt") || fileName.endsWith(".jfr") || fileName.endsWith(".bin")); - }).map(File::getName).collect(toList()); + }).map(File::getName).collect(toList()); } } diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java index 35cba340e04..5c3eea0fd0c 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java @@ -23,6 +23,8 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; import javax.ws.rs.core.Response; @@ -33,8 +35,8 @@ public class LogFileDownloader { private final Histogram fileDownloadSizeDistMb; private final Meter numFileDownloadExceptions; - private final String logRoot; - private final String daemonLogRoot; + private final Path logRoot; + private final Path daemonLogRoot; private final ResourceAuthorizer resourceAuthorizer; /** @@ -47,8 +49,8 @@ public class LogFileDownloader { */ public LogFileDownloader(String logRoot, String daemonLogRoot, 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.resourceAuthorizer = resourceAuthorizer; this.fileDownloadSizeDistMb = metricsRegistry.registerHistogram("logviewer:download-file-size-rounded-MB"); this.numFileDownloadExceptions = metricsRegistry.registerMeter(ExceptionMeterNames.NUM_FILE_DOWNLOAD_EXCEPTIONS); @@ -63,12 +65,22 @@ public LogFileDownloader(String logRoot, String daemonLogRoot, ResourceAuthorize * @return a Response which lets browsers download that file. */ public Response downloadFile(String fileName, String user, boolean isDaemon) throws IOException { - String rootDir = isDaemon ? daemonLogRoot : logRoot; - File file = new File(rootDir, fileName).getCanonicalFile(); - if (file.exists()) { + Path rootDir = isDaemon ? daemonLogRoot : logRoot; + Path rawFile = rootDir.resolve(fileName); + Path file = rawFile.toAbsolutePath().normalize(); + if (!file.startsWith(rootDir) || !rawFile.normalize().toString().equals(rawFile.toString())) { + //Ensure filename doesn't contain ../ parts + return LogviewerResponseBuilder.buildResponsePageNotFound(); + } + if (isDaemon && Paths.get(fileName).getNameCount() != 1) { + //Prevent daemon log reads from pathing into worker logs + return LogviewerResponseBuilder.buildResponsePageNotFound(); + } + + if (file.toFile().exists()) { if (isDaemon || resourceAuthorizer.isUserAllowedToAccessFile(user, fileName)) { - fileDownloadSizeDistMb.update(Math.round((double) file.length() / FileUtils.ONE_MB)); - return LogviewerResponseBuilder.buildDownloadFile(file, numFileDownloadExceptions); + fileDownloadSizeDistMb.update(Math.round((double) file.toFile().length() / FileUtils.ONE_MB)); + return LogviewerResponseBuilder.buildDownloadFile(file.toFile(), numFileDownloadExceptions); } else { return LogviewerResponseBuilder.buildResponseUnauthorizedUser(user); } diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java index 406f91f1975..3a4b57abd56 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java @@ -33,6 +33,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.file.Files; import java.util.HashMap; import java.util.Map; @@ -79,7 +80,7 @@ public static Response buildSuccessJsonResponse(Object entity, String callback, public static Response buildDownloadFile(File file, Meter numFileDownloadExceptions) throws IOException { try { // do not close this InputStream in method: it will be used from jetty server - InputStream is = new FileInputStream(file); + InputStream is = Files.newInputStream(file.toPath()); return Response.status(OK) .entity(wrapWithStreamingOutput(is)) .type(MediaType.APPLICATION_OCTET_STREAM_TYPE) diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizer.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizer.java index 08543501bd8..32f56d79afa 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizer.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizer.java @@ -23,6 +23,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.FileSystems; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -30,6 +31,7 @@ import java.util.Set; import org.apache.commons.lang.StringUtils; +import org.apache.commons.lang3.Validate; import org.apache.storm.Config; import org.apache.storm.DaemonConfig; import org.apache.storm.security.auth.ClientAuthUtils; @@ -38,9 +40,13 @@ import org.apache.storm.utils.ObjectReader; import org.apache.storm.utils.ServerConfigUtils; import org.apache.storm.utils.Utils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class ResourceAuthorizer { + private static final Logger LOG = LoggerFactory.getLogger(ResourceAuthorizer.class); + private final Map stormConf; private final IGroupMappingServiceProvider groupMappingServiceProvider; private final IPrincipalToLocal principalToLocal; @@ -59,7 +65,7 @@ public ResourceAuthorizer(Map stormConf) { /** * Checks whether user is allowed to access a Logviewer file via UI. Always true when the Logviewer filter is not configured. * - * @param fileName file name to access + * @param fileName file name to access. The file name must not contain upward path traversal sequences (e.g. "../"). * @param user username */ public boolean isUserAllowedToAccessFile(String user, String fileName) { @@ -70,15 +76,17 @@ public boolean isUserAllowedToAccessFile(String user, String fileName) { * Checks whether user is authorized to access file. Checks regardless of UI filter. * * @param user username - * @param fileName file name to access + * @param fileName file name to access. The file name must not contain upward path traversal sequences (e.g. "../"). */ public boolean isAuthorizedLogUser(String user, String fileName) { - if (StringUtils.isEmpty(user) || StringUtils.isEmpty(fileName) - || getLogUserGroupWhitelist(fileName) == null) { + Validate.isTrue(!fileName.contains(".." + FileSystems.getDefault().getSeparator())); + if (StringUtils.isEmpty(user) || StringUtils.isEmpty(fileName)) { + return false; + } + LogUserGroupWhitelist whitelist = getLogUserGroupWhitelist(fileName); + if (whitelist == null) { return false; } else { - LogUserGroupWhitelist whitelist = getLogUserGroupWhitelist(fileName); - List logsUsers = new ArrayList<>(); logsUsers.addAll(ObjectReader.getStrings(stormConf.get(DaemonConfig.LOGS_USERS))); logsUsers.addAll(ObjectReader.getStrings(stormConf.get(Config.NIMBUS_ADMINS))); diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/WorkerLogs.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/WorkerLogs.java index 9ed50f61dab..3164aef9944 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/WorkerLogs.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/WorkerLogs.java @@ -74,7 +74,7 @@ public class WorkerLogs { */ public WorkerLogs(Map stormConf, Path logRootDir, StormMetricsRegistry metricsRegistry) { this.stormConf = stormConf; - this.logRootDir = logRootDir; + this.logRootDir = logRootDir.toAbsolutePath().normalize(); this.numSetPermissionsExceptions = metricsRegistry.registerMeter(ExceptionMeterNames.NUM_SET_PERMISSION_EXCEPTIONS); this.directoryCleaner = new DirectoryCleaner(metricsRegistry); } @@ -85,7 +85,10 @@ public WorkerLogs(Map stormConf, Path logRootDir, StormMetricsRe * @param fileName log file */ public void setLogFilePermission(String fileName) throws IOException { - Path file = logRootDir.resolve(fileName).toAbsolutePath().normalize(); + Path absFile = logRootDir.resolve(fileName).toAbsolutePath().normalize(); + if (!absFile.startsWith(logRootDir)) { + return; + } boolean runAsUser = ObjectReader.getBoolean(stormConf.get(SUPERVISOR_RUN_WORKER_AS_USER), false); Path parent = logRootDir.resolve(fileName).getParent(); Optional mdFile = (parent == null) ? Optional.empty() : getMetadataFileForWorkerLogDir(parent); @@ -93,11 +96,11 @@ public void setLogFilePermission(String fileName) throws IOException { ? Optional.of(getTopologyOwnerFromMetadataFile(mdFile.get().toAbsolutePath().normalize())) : Optional.empty(); - if (runAsUser && topoOwner.isPresent() && file.toFile().exists() && !Files.isReadable(file)) { + if (runAsUser && topoOwner.isPresent() && absFile.toFile().exists() && !Files.isReadable(absFile)) { LOG.debug("Setting permissions on file {} with topo-owner {}", fileName, topoOwner); try { ClientSupervisorUtils.processLauncherAndWait(stormConf, topoOwner.get(), - Lists.newArrayList("blob", file.toAbsolutePath().normalize().toString()), null, + Lists.newArrayList("blob", absFile.toAbsolutePath().normalize().toString()), null, "setup group read permissions for file: " + fileName); } catch (IOException e) { numSetPermissionsExceptions.mark(); @@ -120,7 +123,7 @@ public List getAllLogsForRootDir() throws IOException { return files; } - + /** * Return a set of all worker directories in root log directory. */ diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java new file mode 100644 index 00000000000..478f788d8cd --- /dev/null +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java @@ -0,0 +1,142 @@ +/* + * 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.daemon.logviewer.handler; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.junit.Assert.assertThat; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import javax.ws.rs.core.Response; +import org.apache.storm.daemon.logviewer.utils.ResourceAuthorizer; +import org.apache.storm.daemon.logviewer.utils.WorkerLogs; +import org.apache.storm.metric.StormMetricsRegistry; +import org.apache.storm.testing.TmpPath; +import org.apache.storm.utils.Utils; +import org.junit.jupiter.api.Test; + +public class LogviewerLogDownloadHandlerTest { + + @Test + public void testDownloadLogFile() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerLogDownloadHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.downloadLogFile("topoA/1111/worker.log", "user"); + Response topoBResponse = handler.downloadLogFile("topoB/1111/worker.log", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.OK.getStatusCode())); + assertThat(topoAResponse.getEntity(), not(nullValue())); + assertThat(topoBResponse.getStatus(), is(Response.Status.OK.getStatusCode())); + assertThat(topoBResponse.getEntity(), not(nullValue())); + } + } + + @Test + public void testDownloadLogFileTraversal() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerLogDownloadHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.downloadLogFile("../nimbus.log", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDownloadDaemonLogFile() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerLogDownloadHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response response = handler.downloadDaemonLogFile("nimbus.log", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(response.getStatus(), is(Response.Status.OK.getStatusCode())); + assertThat(response.getEntity(), not(nullValue())); + } + } + + @Test + public void testDownloadDaemonLogFilePathIntoWorkerLogs() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerLogDownloadHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response response = handler.downloadDaemonLogFile("workers-artifacts/topoA/1111/worker.log", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(response.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDownloadDaemonLogFilePathOutsideLogRoot() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerLogDownloadHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response response = handler.downloadDaemonLogFile("../evil.sh", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(response.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + private LogviewerLogDownloadHandler createHandlerTraversalTests(Path rootPath) throws IOException { + Path daemonLogRoot = rootPath.resolve("logs"); + Path fileOutsideDaemonRoot = rootPath.resolve("evil.sh"); + Path workerLogRoot = daemonLogRoot.resolve("workers-artifacts"); + Path daemonFile = daemonLogRoot.resolve("nimbus.log"); + Path topoA = workerLogRoot.resolve("topoA"); + Path file1 = topoA.resolve("1111").resolve("worker.log"); + Path file2 = topoA.resolve("2222").resolve("worker.log"); + Path file3 = workerLogRoot.resolve("topoB").resolve("1111").resolve("worker.log"); + + Files.createDirectories(file1.getParent()); + Files.createDirectories(file2.getParent()); + Files.createDirectories(file3.getParent()); + Files.createFile(file1); + Files.createFile(file2); + Files.createFile(file3); + Files.createFile(fileOutsideDaemonRoot); + Files.createFile(daemonFile); + + Map stormConf = Utils.readStormConfig(); + StormMetricsRegistry metricsRegistry = new StormMetricsRegistry(); + return new LogviewerLogDownloadHandler(workerLogRoot.toString(), daemonLogRoot.toString(), + new WorkerLogs(stormConf, workerLogRoot, metricsRegistry), new ResourceAuthorizer(stormConf), metricsRegistry); + } + +} diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogPageHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogPageHandlerTest.java index bc998295e0b..08796b43754 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogPageHandlerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogPageHandlerTest.java @@ -18,7 +18,9 @@ package org.apache.storm.daemon.logviewer.handler; +import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import com.fasterxml.jackson.databind.ObjectMapper; @@ -27,7 +29,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; -import java.nio.file.attribute.FileAttribute; +import java.nio.file.Path; import java.util.List; import java.util.Map; @@ -37,6 +39,7 @@ import org.apache.storm.daemon.logviewer.utils.ResourceAuthorizer; import org.apache.storm.daemon.logviewer.utils.WorkerLogs; import org.apache.storm.metric.StormMetricsRegistry; +import org.apache.storm.testing.TmpPath; import org.apache.storm.utils.Utils; import org.assertj.core.util.Lists; import org.junit.Test; @@ -48,8 +51,7 @@ public class LogviewerLogPageHandlerTest { */ @Test public void testListLogFiles() throws IOException { - FileAttribute[] attrs = new FileAttribute[0]; - String rootPath = Files.createTempDirectory("workers-artifacts", attrs).toFile().getCanonicalPath(); + String rootPath = Files.createTempDirectory("workers-artifacts").toFile().getCanonicalPath(); File file1 = new File(String.join(File.separator, rootPath, "topoA", "1111"), "worker.log"); File file2 = new File(String.join(File.separator, rootPath, "topoA", "2222"), "worker.log"); File file3 = new File(String.join(File.separator, rootPath, "topoB", "1111"), "worker.log"); @@ -64,7 +66,7 @@ public void testListLogFiles() throws IOException { String origin = "www.origin.server.net"; Map stormConf = Utils.readStormConfig(); StormMetricsRegistry metricsRegistry = new StormMetricsRegistry(); - LogviewerLogPageHandler handler = new LogviewerLogPageHandler(rootPath, null, + LogviewerLogPageHandler handler = new LogviewerLogPageHandler(rootPath, rootPath, new WorkerLogs(stormConf, Paths.get(rootPath), metricsRegistry), new ResourceAuthorizer(stormConf), metricsRegistry); final Response expectedAll = LogviewerResponseBuilder.buildSuccessJsonResponse( @@ -105,4 +107,90 @@ private void assertEqualsJsonResponse(Response expected, Response actual, Cl assertEquals(expected.getStatus(), actual.getStatus()); assertTrue(expected.getHeaders().equalsIgnoreValueOrder(actual.getHeaders())); } + + @Test + public void testListLogFilesOutsideLogRoot() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + String origin = "www.origin.server.net"; + LogviewerLogPageHandler handler = createHandlerForTraversalTests(rootPath.getFile().toPath()); + + //The response should be empty, since you should not be able to list files outside the worker log root. + final Response expected = LogviewerResponseBuilder.buildSuccessJsonResponse( + Lists.newArrayList(), + null, + origin + ); + + final Response returned = handler.listLogFiles("user", null, "../", null, origin); + + assertEqualsJsonResponse(expected, returned, List.class); + } + } + + @Test + public void testLogPageOutsideLogRoot() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + LogviewerLogPageHandler handler = createHandlerForTraversalTests(rootPath.getFile().toPath()); + + final Response returned = handler.logPage("../nimbus.log", 0, 100, null, "user"); + + Utils.forceDelete(rootPath.toString()); + + //Should not show files outside worker log root. + assertThat(returned.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDaemonLogPageOutsideLogRoot() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + LogviewerLogPageHandler handler = createHandlerForTraversalTests(rootPath.getFile().toPath()); + + final Response returned = handler.daemonLogPage("../evil.sh", 0, 100, null, "user"); + + Utils.forceDelete(rootPath.toString()); + + //Should not show files outside daemon log root. + assertThat(returned.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDaemonLogPagePathIntoWorkerLogs() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + LogviewerLogPageHandler handler = createHandlerForTraversalTests(rootPath.getFile().toPath()); + + final Response returned = handler.daemonLogPage("workers-artifacts/topoA/worker.log", 0, 100, null, "user"); + + Utils.forceDelete(rootPath.toString()); + + //Should not show files outside log root. + assertThat(returned.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + private LogviewerLogPageHandler createHandlerForTraversalTests(Path rootPath) throws IOException { + Path daemonLogRoot = rootPath.resolve("logs"); + Path fileOutsideDaemonRoot = rootPath.resolve("evil.sh"); + Path daemonFile = daemonLogRoot.resolve("nimbus.log"); + Path workerLogRoot = daemonLogRoot.resolve("workers-artifacts"); + Path topoA = workerLogRoot.resolve("topoA"); + Path file1 = topoA.resolve("1111").resolve("worker.log"); + Path file2 = topoA.resolve("2222").resolve("worker.log"); + Path file3 = workerLogRoot.resolve("topoB").resolve("1111").resolve("worker.log"); + + Files.createDirectories(file1.getParent()); + Files.createDirectories(file2.getParent()); + Files.createDirectories(file3.getParent()); + Files.createFile(file1); + Files.createFile(file2); + Files.createFile(file3); + Files.createFile(fileOutsideDaemonRoot); + Files.createFile(daemonFile); + + Map stormConf = Utils.readStormConfig(); + StormMetricsRegistry metricsRegistry = new StormMetricsRegistry(); + return new LogviewerLogPageHandler(workerLogRoot.toString(), daemonLogRoot.toString(), + new WorkerLogs(stormConf, workerLogRoot, metricsRegistry), new ResourceAuthorizer(stormConf), metricsRegistry); + } } diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java index cb5116d5b8f..784fd02e800 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java @@ -69,16 +69,15 @@ public class LogviewerLogSearchHandlerTest { public static class SearchViaRestApi { + private final String pattern = "needle"; private final String expectedHost = "dev.null.invalid"; private final Integer expectedPort = 8888; private final String logviewerUrlPrefix = "http://" + expectedHost + ":" + expectedPort; /* - When we click a link to the logviewer, we expect the match line to - be somewhere near the middle of the page. So we subtract half of - the default page length from the offset at which we found the - match. + * When we click a link to the logviewer, we expect the match line to be somewhere near the middle of the page. So we subtract half + * of the default page length from the offset at which we found the match. */ private final Function expOffsetFn = arg -> (LogviewerConstant.DEFAULT_BYTES_PER_PAGE / 2 - arg); @@ -103,7 +102,7 @@ public void testLogviewerLinkCentersTheMatchInThePage() throws UnknownHostExcept String actualUrl = handler.urlToMatchCenteredInLogPage(new byte[42], new File(expectedFname).toPath(), 27526, 8888); assertEquals("http://" + expectedHost + ":" + expectedPort + "/api/v1/log?file=" + expectedFname - + "&start=1947&length=" + LogviewerConstant.DEFAULT_BYTES_PER_PAGE, actualUrl); + + "&start=1947&length=" + LogviewerConstant.DEFAULT_BYTES_PER_PAGE, actualUrl); } finally { Utils.setInstance(prevUtils); } @@ -124,7 +123,7 @@ public void testLogviewerLinkCentersTheMatchInThePageDaemon() throws UnknownHost String actualUrl = handler.urlToMatchCenteredInLogPageDaemonFile(new byte[42], new File(expectedFname).toPath(), 27526, 8888); assertEquals("http://" + expectedHost + ":" + expectedPort + "/api/v1/daemonlog?file=" + expectedFname - + "&start=1947&length=" + LogviewerConstant.DEFAULT_BYTES_PER_PAGE, actualUrl); + + "&start=1947&length=" + LogviewerConstant.DEFAULT_BYTES_PER_PAGE, actualUrl); } finally { Utils.setInstance(prevUtils); } @@ -151,34 +150,34 @@ public void testReturnsCorrectBeforeAndAfterContext() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(0, "", - " needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle ", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() +"resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" - )); + " needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle ", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" + )); matches.add(buildMatchData(7, "needle ", - "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle needle\n", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" - )); + "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle needle\n", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" + )); matches.add(buildMatchData(127, - "needle needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", - " needle\n", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" - )); + "needle needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + " needle\n", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" + )); matches.add(buildMatchData(134, - " needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle ", - "\n", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" - )); + " needle000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000needle ", + "\n", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" + )); expected.put("matches", matches); @@ -201,7 +200,7 @@ public void testAreallySmallLogFile() throws Exception { when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "small-worker.log.test"); + "small-worker.log.test"); Map expected = new HashMap<>(); expected.put("isDaemon", "no"); @@ -211,17 +210,17 @@ public void testAreallySmallLogFile() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(7, "000000 ", - " 000000\n", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + " 000000\n", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); expected.put("matches", matches); LogviewerLogSearchHandler handler = getSearchHandlerWithPort(expectedPort); Map searchResult = handler.substringSearch(file.toPath(), pattern); - + assertEquals(expected, searchResult); } finally { Utils.setInstance(prevUtils); @@ -238,7 +237,7 @@ public void testAreallySmallLogDaemonFile() throws InvalidRequestException, Unkn when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "small-worker.log.test"); + "small-worker.log.test"); Map expected = new HashMap<>(); expected.put("isDaemon", "yes"); @@ -248,9 +247,9 @@ public void testAreallySmallLogDaemonFile() throws InvalidRequestException, Unkn List> matches = new ArrayList<>(); matches.add(buildMatchData(7, "000000 ", - " 000000\n", - pattern, - "/api/v1/daemonlog?file=" + file.getName() + "&start=0&length=51200" + " 000000\n", + pattern, + "/api/v1/daemonlog?file=" + file.getName() + "&start=0&length=51200" )); expected.put("matches", matches); @@ -274,7 +273,7 @@ public void testNoOffsetReturnedWhenFileEndsOnBufferOffset() throws Exception { when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-3072.log.test"); + "test-3072.log.test"); Map expected = new HashMap<>(); expected.put("isDaemon", "no"); @@ -284,11 +283,11 @@ public void testNoOffsetReturnedWhenFileEndsOnBufferOffset() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(3066, - Seq.range(0, 128).map(x -> ".").collect(joining()), - "", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + Seq.range(0, 128).map(x -> ".").collect(joining()), + "", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); expected.put("matches", matches); @@ -315,7 +314,7 @@ public void testNextByteOffsetsAreCorrectForEachMatch() throws Exception { when(mockedUtil.hostname()).thenReturn(expectedHost); File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-worker.log.test"); + "test-worker.log.test"); LogviewerLogSearchHandler handler = getSearchHandlerWithPort(expectedPort); @@ -350,59 +349,59 @@ public void testNextByteOffsetsAreCorrectForEachMatch() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(5, - "Test ", - " is near the beginning of the file.\nThis file assumes a buffer size of 2048 bytes, a max search string size of 1024 bytes, and a", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "Test ", + " is near the beginning of the file.\nThis file assumes a buffer size of 2048 bytes, a max search string size of 1024 bytes, and a", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(2036, - "ng 146\npadding 147\npadding 148\npadding 149\npadding 150\npadding 151\npadding 152\npadding 153\nNear the end of a 1024 byte block, a ", - ".\nA needle that straddles a 1024 byte boundary should also be detected.\n\npadding 157\npadding 158\npadding 159\npadding 160\npadding", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "ng 146\npadding 147\npadding 148\npadding 149\npadding 150\npadding 151\npadding 152\npadding 153\nNear the end of a 1024 byte block, a ", + ".\nA needle that straddles a 1024 byte boundary should also be detected.\n\npadding 157\npadding 158\npadding 159\npadding 160\npadding", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(2046, - "ding 147\npadding 148\npadding 149\npadding 150\npadding 151\npadding 152\npadding 153\nNear the end of a 1024 byte block, a needle.\nA ", - " that straddles a 1024 byte boundary should also be detected.\n\npadding 157\npadding 158\npadding 159\npadding 160\npadding 161\npaddi", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "ding 147\npadding 148\npadding 149\npadding 150\npadding 151\npadding 152\npadding 153\nNear the end of a 1024 byte block, a needle.\nA ", + " that straddles a 1024 byte boundary should also be detected.\n\npadding 157\npadding 158\npadding 159\npadding 160\npadding 161\npaddi", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(3072, - "adding 226\npadding 227\npadding 228\npadding 229\npadding 230\npadding 231\npadding 232\npadding 233\npadding 234\npadding 235\n\n\nHere a ", - " occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: needleneedle\n\npa", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "adding 226\npadding 227\npadding 228\npadding 229\npadding 230\npadding 231\npadding 232\npadding 233\npadding 234\npadding 235\n\n\nHere a ", + " occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: needleneedle\n\npa", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(3190, - "\n\n\nHere a needle occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: ", - "needle\n\npadding 243\npadding 244\npadding 245\npadding 246\npadding 247\npadding 248\npadding 249\npadding 250\npadding 251\npadding 252\n", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "\n\n\nHere a needle occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: ", + "needle\n\npadding 243\npadding 244\npadding 245\npadding 246\npadding 247\npadding 248\npadding 249\npadding 250\npadding 251\npadding 252\n", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(3196, - "e a needle occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: needle", - "\n\npadding 243\npadding 244\npadding 245\npadding 246\npadding 247\npadding 248\npadding 249\npadding 250\npadding 251\npadding 252\npaddin", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "e a needle occurs just after a 1024 byte boundary. It should have the correct context.\n\nText with two adjoining matches: needle", + "\n\npadding 243\npadding 244\npadding 245\npadding 246\npadding 247\npadding 248\npadding 249\npadding 250\npadding 251\npadding 252\npaddin", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); matches.add(buildMatchData(6246, - "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\n", - "\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤¥\n\nneedle\n\n\n\nHere are four printable 3-byte UTF-8 characters: ऄअ", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\n", + "\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤¥\n\nneedle\n\n\n\nHere are four printable 3-byte UTF-8 characters: ऄअ", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); expected.put("matches", matches); @@ -426,7 +425,7 @@ public void testCorrectMatchOffsetIsReturnedWhenSkippingBytes() throws Exception when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-worker.log.test"); + "test-worker.log.test"); int startByteOffset = 3197; @@ -439,11 +438,11 @@ public void testCorrectMatchOffsetIsReturnedWhenSkippingBytes() throws Exception List> matches = new ArrayList<>(); matches.add(buildMatchData(6246, - "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\n", - "\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤¥\n\nneedle\n\n\n\nHere are four printable 3-byte UTF-8 characters: ऄअ", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\n", + "\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤¥\n\nneedle\n\n\n\nHere are four printable 3-byte UTF-8 characters: ऄअ", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); expected.put("matches", matches); @@ -468,7 +467,7 @@ public void testAnotherPatterns1() throws Exception { when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-worker.log.test"); + "test-worker.log.test"); String pattern = Seq.range(0, 1024).map(x -> "X").collect(joining()); @@ -481,16 +480,16 @@ public void testAnotherPatterns1() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(4075, - "\n\nThe following match of 1024 bytes completely fills half the byte buffer. It is a search substring of the maximum size......\n\n", - "\nThe following max-size match straddles a 1024 byte buffer.\nXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", + "\n\nThe following match of 1024 bytes completely fills half the byte buffer. It is a search substring of the maximum size......\n\n", + "\nThe following max-size match straddles a 1024 byte buffer.\nXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", pattern, "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + "&start=0&length=51200" )); matches.add(buildMatchData(5159, - "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\nThe following max-size match straddles a 1024 byte buffer.\n", - "\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\nneedle\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤", + "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\nThe following max-size match straddles a 1024 byte buffer.\n", + "\n\nHere are four non-ascii 1-byte UTF-8 characters: αβγδε\n\nneedle\n\nHere are four printable 2-byte UTF-8 characters: ¡¢£¤", pattern, "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + "&start=0&length=51200" @@ -518,7 +517,7 @@ public void testAnotherPatterns2() throws Exception { when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-worker.log.test"); + "test-worker.log.test"); String pattern = "𐄀𐄁𐄂"; Map expected = new HashMap<>(); @@ -530,11 +529,11 @@ public void testAnotherPatterns2() throws Exception { List> matches = new ArrayList<>(); matches.add(buildMatchData(7164, - "padding 372\npadding 373\npadding 374\npadding 375\n\nThe following tests multibyte UTF-8 Characters straddling the byte boundary: ", - "\n\nneedle", - pattern, - "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() - + "&start=0&length=51200" + "padding 372\npadding 373\npadding 374\npadding 375\n\nThe following tests multibyte UTF-8 Characters straddling the byte boundary: ", + "\n\nneedle", + pattern, + "/api/v1/log?file=test" + encodedFileSeparator() + "resources" + encodedFileSeparator() + file.getName() + + "&start=0&length=51200" )); expected.put("matches", matches); @@ -560,7 +559,7 @@ public void testReturnsZeroMatchesForUnseenPattern() throws UnknownHostException when(mockedUtil.hostname()).thenReturn(expectedHost); final File file = new File(String.join(File.separator, "src", "test", "resources"), - "test-worker.log.test"); + "test-worker.log.test"); Map expected = new HashMap<>(); expected.put("isDaemon", "no"); @@ -579,7 +578,7 @@ public void testReturnsZeroMatchesForUnseenPattern() throws UnknownHostException } private Map buildMatchData(int byteOffset, String beforeString, String afterString, - String matchString, String logviewerUrlPath) { + String matchString, String logviewerUrlPath) { Map match = new HashMap<>(); match.put("byteOffset", byteOffset); match.put("beforeString", beforeString); @@ -588,13 +587,14 @@ private Map buildMatchData(int byteOffset, String beforeString, match.put("logviewerURL", logviewerUrlPrefix + logviewerUrlPath); return match; } - + private String encodedFileSeparator() { return Utils.urlEncodeUtf8(File.separator); } } public static class FindNMatchesTest { + /** * find-n-matches looks through logs properly. */ @@ -602,9 +602,9 @@ public static class FindNMatchesTest { public void testFindNMatches() { List files = new ArrayList<>(); files.add(new File(String.join(File.separator, "src", "test", "resources"), - "logviewer-search-context-tests.log.test").toPath()); + "logviewer-search-context-tests.log.test").toPath()); files.add(new File(String.join(File.separator, "src", "test", "resources"), - "logviewer-search-context-tests.log.gz").toPath()); + "logviewer-search-context-tests.log.gz").toPath()); final LogviewerLogSearchHandler handler = getSearchHandler(); @@ -641,8 +641,7 @@ public void setUp() throws IOException { logFiles.add(Paths.get("src/test/resources/logviewer-search-context-tests.log.test")); logFiles.add(Paths.get("src/test/resources/logviewer-search-context-tests.log.gz")); - FileAttribute[] attrs = new FileAttribute[0]; - topoPath = Files.createTempDirectory("topoA", attrs).toAbsolutePath().normalize(); + topoPath = Files.createTempDirectory("topoA").toAbsolutePath().normalize(); new File(topoPath.toFile(), "6400").createNewFile(); new File(topoPath.toFile(), "6500").createNewFile(); new File(topoPath.toFile(), "6600").createNewFile(); @@ -676,7 +675,7 @@ public void testAllPortsAndSearchArchivedIsTrue() throws IOException { ArgumentCaptor search = ArgumentCaptor.forClass(String.class); verify(handler, times(4)).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); + offset.capture(), search.capture()); verify(handler, times(4)).logsForPort(isNull(), any()); // File offset and byte offset should always be zero when searching multiple workers (multiple ports). @@ -718,7 +717,7 @@ public void testAllPortsAndSearchArchivedIsFalse() throws IOException { ArgumentCaptor search = ArgumentCaptor.forClass(String.class); verify(handler, times(4)).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); + offset.capture(), search.capture()); verify(handler, times(4)).logsForPort(isNull(), any()); // File offset and byte offset should always be zero when searching multiple workers (multiple ports). @@ -760,8 +759,8 @@ public void testOnePortAndSearchArchivedIsTrueAndNotFileOffset() throws IOExcept ArgumentCaptor search = ArgumentCaptor.forClass(String.class); verify(handler, times(1)).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); - verify(handler, times(2)).logsForPort(isNull(), any()); + offset.capture(), search.capture()); + verify(handler).logsForPort(isNull(), any()); assertEquals(logFiles, files.getAllValues().get(0)); assertEquals(Integer.valueOf(20), numMatches.getAllValues().get(0)); @@ -783,8 +782,8 @@ public void testOnePortAndSearchArchivedIsTrueAndFileOffsetIs1() throws IOExcept ArgumentCaptor search = ArgumentCaptor.forClass(String.class); verify(handler, times(1)).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); - verify(handler, times(2)).logsForPort(isNull(), any()); + offset.capture(), search.capture()); + verify(handler).logsForPort(isNull(), any()); assertEquals(logFiles, files.getAllValues().get(0)); assertEquals(Integer.valueOf(20), numMatches.getAllValues().get(0)); @@ -806,8 +805,8 @@ public void testOnePortAndSearchArchivedIsFalseAndFileOffsetIs1() throws IOExcep ArgumentCaptor search = ArgumentCaptor.forClass(String.class); verify(handler, times(1)).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); - verify(handler, times(2)).logsForPort(isNull(), any()); + offset.capture(), search.capture()); + verify(handler).logsForPort(isNull(), any()); // File offset should be zero, since search-archived is false. assertEquals(Collections.singletonList(logFiles.get(0)), files.getAllValues().get(0)); @@ -824,7 +823,7 @@ public void testOnePortAndSearchArchivedIsTrueAndFileOffsetIs1AndByteOffsetIs100 handler.deepSearchLogsForTopology("", null, "search", "20", "6700", "1", "100", true, null, null); verify(handler, times(1)).findNMatches(anyList(), anyInt(), anyInt(), anyInt(), anyString()); - verify(handler, times(2)).logsForPort(isNull(), any()); + verify(handler, times(1)).logsForPort(isNull(), any()); } @Test @@ -841,14 +840,14 @@ public void testBadPortAndSearchArchivedIsFalseAndFileOffsetIs1() throws IOExcep // Called with a bad port (not in the config) No searching should be done. verify(handler, never()).findNMatches(files.capture(), numMatches.capture(), fileOffset.capture(), - offset.capture(), search.capture()); + offset.capture(), search.capture()); verify(handler, never()).logsForPort(anyString(), any()); } private LogviewerLogSearchHandler getStubbedSearchHandler() { Map stormConf = Utils.readStormConfig(); - LogviewerLogSearchHandler handler = new LogviewerLogSearchHandler(stormConf, topoPath, null, - new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); + LogviewerLogSearchHandler handler = new LogviewerLogSearchHandler(stormConf, topoPath, Paths.get(""), + new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); handler = spy(handler); doReturn(logFiles).when(handler).logsForPort(any(), any()); @@ -866,15 +865,15 @@ private LogviewerLogSearchHandler getStubbedSearchHandler() { private static LogviewerLogSearchHandler getSearchHandler() { Map stormConf = Utils.readStormConfig(); - return new LogviewerLogSearchHandler(stormConf, null, null, - new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); + return new LogviewerLogSearchHandler(stormConf, Paths.get(""), Paths.get(""), + new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); } private static LogviewerLogSearchHandler getSearchHandlerWithPort(int port) { Map stormConf = Utils.readStormConfig(); stormConf.put(DaemonConfig.LOGVIEWER_PORT, port); - return new LogviewerLogSearchHandler(stormConf, null, null, - new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); + return new LogviewerLogSearchHandler(stormConf, Paths.get(""), Paths.get(""), + new ResourceAuthorizer(stormConf), new StormMetricsRegistry()); } } diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java new file mode 100644 index 00000000000..96a287d0423 --- /dev/null +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java @@ -0,0 +1,165 @@ +/* + * 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.daemon.logviewer.handler; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.junit.Assert.assertThat; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import javax.ws.rs.core.Response; +import org.apache.commons.io.IOUtils; +import org.apache.storm.daemon.logviewer.utils.ResourceAuthorizer; +import org.apache.storm.metric.StormMetricsRegistry; +import org.apache.storm.testing.TmpPath; +import org.apache.storm.utils.Utils; +import org.junit.jupiter.api.Test; + +public class LogviewerProfileHandlerTest { + + @Test + public void testListDumpFiles() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.listDumpFiles("topoA", "localhost:1111", "user"); + Response topoBResponse = handler.listDumpFiles("topoB", "localhost:1111", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.OK.getStatusCode())); + String contentA = (String) topoAResponse.getEntity(); + assertThat(contentA, containsString("worker.jfr")); + assertThat(contentA, not(containsString("worker.bin"))); + assertThat(contentA, not(containsString("worker.txt"))); + String contentB = (String) topoBResponse.getEntity(); + assertThat(contentB, containsString("worker.txt")); + assertThat(contentB, not(containsString("worker.jfr"))); + assertThat(contentB, not(containsString("worker.bin"))); + } + } + + @Test + public void testListDumpFilesTraversalInTopoId() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response response = handler.listDumpFiles("../../", "localhost:logs", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(response.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testListDumpFilesTraversalInPort() throws Exception { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response response = handler.listDumpFiles("../", "localhost:../logs", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(response.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDownloadDumpFile() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.downloadDumpFile("topoA", "localhost:1111", "worker.jfr", "user"); + Response topoBResponse = handler.downloadDumpFile("topoB", "localhost:1111", "worker.txt", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.OK.getStatusCode())); + assertThat(topoAResponse.getEntity(), not(nullValue())); + assertThat(topoBResponse.getStatus(), is(Response.Status.OK.getStatusCode())); + assertThat(topoBResponse.getEntity(), not(nullValue())); + } + } + + @Test + public void testDownloadDumpFileTraversalInTopoId() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.downloadDumpFile("../../", "localhost:logs", "daemon-dump.bin", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + @Test + public void testDownloadDumpFileTraversalInPort() throws IOException { + try (TmpPath rootPath = new TmpPath()) { + + LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); + + Response topoAResponse = handler.downloadDumpFile("../", "localhost:../logs", "daemon-dump.bin", "user"); + + Utils.forceDelete(rootPath.toString()); + + assertThat(topoAResponse.getStatus(), is(Response.Status.NOT_FOUND.getStatusCode())); + } + } + + private LogviewerProfileHandler createHandlerTraversalTests(Path rootPath) throws IOException { + Path daemonLogRoot = rootPath.resolve("logs"); + Path fileOutsideDaemonRoot = rootPath.resolve("evil.bin"); + Path workerLogRoot = daemonLogRoot.resolve("workers-artifacts"); + Path daemonFile = daemonLogRoot.resolve("daemon-dump.bin"); + Path topoA = workerLogRoot.resolve("topoA"); + Path file1 = topoA.resolve("1111").resolve("worker.jfr"); + Path file2 = topoA.resolve("2222").resolve("worker.bin"); + Path file3 = workerLogRoot.resolve("topoB").resolve("1111").resolve("worker.txt"); + + Files.createDirectories(file1.getParent()); + Files.createDirectories(file2.getParent()); + Files.createDirectories(file3.getParent()); + Files.write(file1, "TopoA jfr".getBytes(StandardCharsets.UTF_8)); + Files.write(file3, "TopoB txt".getBytes(StandardCharsets.UTF_8)); + Files.createFile(file2); + Files.createFile(fileOutsideDaemonRoot); + Files.createFile(daemonFile); + + Map stormConf = Utils.readStormConfig(); + StormMetricsRegistry metricsRegistry = new StormMetricsRegistry(); + return new LogviewerProfileHandler(workerLogRoot.toString(), new ResourceAuthorizer(stormConf), metricsRegistry); + } + +} diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java index 4b09d6b8d68..8f5ebb856fb 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java @@ -27,13 +27,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.anyListOf; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyMap; -import static org.mockito.ArgumentMatchers.anySetOf; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -43,8 +37,8 @@ import java.nio.file.DirectoryStream; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.attribute.FileTime; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; @@ -55,7 +49,6 @@ import java.util.TreeSet; import java.util.function.Predicate; -import java.util.stream.Collectors; import org.apache.storm.daemon.supervisor.SupervisorUtils; import org.apache.storm.generated.LSWorkerHeartbeat; import org.apache.storm.metric.StormMetricsRegistry; @@ -66,7 +59,6 @@ import org.jooq.lambda.Seq; import org.junit.jupiter.api.Test; import org.mockito.internal.util.collections.Sets; -import org.slf4j.LoggerFactory; public class LogCleanerTest { @@ -81,7 +73,7 @@ public void testMkFileFilterForLogCleanup() throws IOException { conf.put(LOGVIEWER_CLEANUP_INTERVAL_SECS, 300); StormMetricsRegistry metricRegistry = new StormMetricsRegistry(); - WorkerLogs workerLogs = new WorkerLogs(conf, null, metricRegistry); + WorkerLogs workerLogs = new WorkerLogs(conf, Paths.get(""), metricRegistry); LogCleaner logCleaner = new LogCleaner(conf, workerLogs, new DirectoryCleaner(metricRegistry), null, metricRegistry); @@ -244,7 +236,7 @@ public void testGetDeadWorkerDirs() throws Exception { Map conf = Utils.readStormConfig(); StormMetricsRegistry metricRegistry = new StormMetricsRegistry(); - WorkerLogs stubbedWorkerLogs = new WorkerLogs(conf, null, metricRegistry) { + WorkerLogs stubbedWorkerLogs = new WorkerLogs(conf, Paths.get(""), metricRegistry) { @Override public SortedSet getLogDirs(Set logDirs, Predicate predicate) { TreeSet ret = new TreeSet<>(); @@ -282,7 +274,7 @@ public void testCleanupFn() throws IOException { Map conf = Utils.readStormConfig(); StormMetricsRegistry metricRegistry = new StormMetricsRegistry(); - WorkerLogs stubbedWorkerLogs = new WorkerLogs(conf, null, metricRegistry); + WorkerLogs stubbedWorkerLogs = new WorkerLogs(conf, Paths.get(""), metricRegistry); LogCleaner logCleaner = new LogCleaner(conf, stubbedWorkerLogs, new DirectoryCleaner(metricRegistry), null, metricRegistry) { @Override @@ -309,23 +301,4 @@ void cleanupEmptyTopoDirectory(Path dir) throws IOException { assertThat(Files.exists(dir2.getFile().toPath()), is(false)); } } - - private Path mkMockPath(File file) { - Path mockPath = mock(Path.class); - when(mockPath.toFile()).thenReturn(file); - return mockPath; - } - - private DirectoryStream mkDirectoryStream(List listOfPaths) { - return new DirectoryStream() { - @Override - public Iterator iterator() { - return listOfPaths.iterator(); - } - - @Override - public void close() throws IOException { - } - }; - } } diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizerTest.java index e6aada68211..25d5471bdc9 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/ResourceAuthorizerTest.java @@ -27,9 +27,9 @@ import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import java.nio.file.Paths; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -37,7 +37,8 @@ import org.apache.storm.DaemonConfig; import org.apache.storm.daemon.logviewer.testsupport.ArgumentsVerifier; import org.apache.storm.utils.Utils; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import org.mockito.Mockito; public class ResourceAuthorizerTest { @@ -171,10 +172,25 @@ public void testAuthorizedLogUserDisallowUserNotInNimbusAdminNorTopoUserNorLogsU verifyStubMethodsAreCalledProperly(authorizer); } + + /** + * disallow upward path traversal in filenames. + */ + @Test + public void testFailOnUpwardPathTraversal() { + Map stormConf = Utils.readStormConfig(); + + Map conf = new HashMap<>(stormConf); + + ResourceAuthorizer authorizer = new ResourceAuthorizer(conf); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> authorizer.isAuthorizedLogUser("user", Paths.get("some/../path").toString())); + } private void verifyStubMethodsAreCalledProperly(ResourceAuthorizer authorizer) { ArgumentsVerifier.verifyFirstCallArgsForSingleArgMethod( - captor -> verify(authorizer, times(2)).getLogUserGroupWhitelist(captor.capture()), + captor -> verify(authorizer).getLogUserGroupWhitelist(captor.capture()), String.class, "non-blank-fname"); ArgumentsVerifier.verifyFirstCallArgsForSingleArgMethod(