Make sure DirectoryPathTree does not allow reading outside the root dir

quarkusio · Feb 16, 2022 · 9d726e7 · 9d726e7
1 parent 748fd26
commit 9d726e7
Show file tree

Hide file tree

Showing 9 changed files with 367 additions and 0 deletions.
diff --git a/independent-projects/bootstrap/app-model/pom.xml b/independent-projects/bootstrap/app-model/pom.xml
@@ -50,6 +50,11 @@
             <artifactId>junit-jupiter</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <scope>test</scope>
+        </dependency>
 
         <dependency>
             <groupId>io.quarkus</groupId>

diff --git a/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/ArchivePathTree.java b/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/ArchivePathTree.java
@@ -49,8 +49,24 @@ public void walk(PathVisitor visitor) {
         }
     }
 
+    private String ensureRelative(String path) {
+        if (path == null || path.isEmpty()) {
+            return path;
+        }
+        if (DirectoryPathTree.isWindowsAbsolutePath(path)) {
+            // We are allowing absolute paths on Linux but interpreting the root as the root of the tree.
+            // However, Windows absolute paths, including the disk part, are not expected.
+            throw new IllegalArgumentException(path + " does not appear to be a path relative to the root of the path tree");
+        }
+        if (path.charAt(0) == '/') {
+            return path.substring(1);
+        }
+        return path;
+    }
+
     @Override
     protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean manifestEnabled) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return func.apply(null);
         }
@@ -73,6 +89,7 @@ protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean
 
     @Override
     public void accept(String relativePath, Consumer<PathVisit> consumer) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             consumer.accept(null);
             return;
@@ -97,6 +114,7 @@ public void accept(String relativePath, Consumer<PathVisit> consumer) {
 
     @Override
     public boolean contains(String relativePath) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return false;
         }

diff --git a/...endent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java b/...endent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java
@@ -2,6 +2,7 @@
 
 import java.io.IOException;
 import java.io.Serializable;
+import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -10,11 +11,25 @@
 import java.util.Objects;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.regex.Pattern;
 
 public class DirectoryPathTree extends PathTreeWithManifest implements OpenPathTree, Serializable {
 
     private static final long serialVersionUID = 2255956884896445059L;
 
+    private static final boolean USE_WINDOWS_ABSOLUTE_PATH_PATTERN = !FileSystems.getDefault().getSeparator().equals("/");
+
+    private static volatile Pattern windowsAbsolutePathPattern;
+
+    private static Pattern windowsAbsolutePathPattern() {
+        return windowsAbsolutePathPattern == null ? windowsAbsolutePathPattern = Pattern.compile("[a-zA-Z]:\\\\.*")
+                : windowsAbsolutePathPattern;
+    }
+
+    static boolean isWindowsAbsolutePath(String path) {
+        return USE_WINDOWS_ABSOLUTE_PATH_PATTERN ? windowsAbsolutePathPattern().matcher(path).matches() : false;
+    }
+
     private Path dir;
     private PathFilter pathFilter;
 
@@ -55,8 +70,33 @@ public void walk(PathVisitor visitor) {
         PathTreeVisit.walk(dir, dir, pathFilter, getMultiReleaseMapping(), visitor);
     }
 
+    private String ensureRelative(String path) {
+        if (path == null || path.isEmpty()) {
+            return path;
+        }
+        if (isWindowsAbsolutePath(path)) {
+            // We are allowing absolute paths on Linux but interpreting the root as the root of the tree.
+            // However, Windows absolute paths, including the disk part, are not expected.
+            throw new IllegalArgumentException(
+                    path + " does not appear to be a path relative to the root of the path tree " + dir);
+        }
+        // this is to disallow reading outside the path tree root
+        if (path.contains("..")) {
+            final Path absolutePath = dir.resolve(path).normalize().toAbsolutePath();
+            if (absolutePath.startsWith(dir)) {
+                return dir.relativize(absolutePath).toString();
+            }
+            return null;
+        }
+        if (path.charAt(0) == '/') {
+            return path.substring(1);
+        }
+        return path;
+    }
+
     @Override
     protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean manifestEnabled) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return func.apply(null);
         }
@@ -69,6 +109,7 @@ protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean
 
     @Override
     public void accept(String relativePath, Consumer<PathVisit> consumer) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             consumer.accept(null);
             return;
@@ -83,6 +124,7 @@ public void accept(String relativePath, Consumer<PathVisit> consumer) {
 
     @Override
     public boolean contains(String relativePath) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return false;
         }
@@ -92,6 +134,7 @@ public boolean contains(String relativePath) {
 
     @Override
     public Path getPath(String relativePath) {
+        relativePath = ensureRelative(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return null;
         }

diff --git a/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/PathFilter.java b/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/PathFilter.java
@@ -13,6 +13,9 @@ public class PathFilter implements Serializable {
     private static final long serialVersionUID = -5712472676677054175L;
 
     public static boolean isVisible(PathFilter filter, String path) {
+        if (path == null) {
+            return false;
+        }
         if (filter == null) {
             return true;
         }

diff --git a/...nt-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java b/...nt-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java
@@ -0,0 +1,166 @@
+package io.quarkus.paths;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.EnabledOnOs;
+import org.junit.jupiter.api.condition.OS;
+
+public class DirectoryPathTreeTest {
+
+    private static final String BASE_DIR = "paths/directory-path-tree";
+
+    private static volatile Path baseDir;
+
+    @BeforeAll
+    public static void staticInit() throws Exception {
+        final URL url = Thread.currentThread().getContextClassLoader().getResource(BASE_DIR);
+        if (url == null) {
+            throw new IllegalStateException("Failed to locate " + BASE_DIR + " on the classpath");
+        }
+        baseDir = Path.of(url.toURI()).toAbsolutePath();
+        if (!Files.exists(baseDir)) {
+            throw new IllegalStateException("Failed to locate " + baseDir);
+        }
+    }
+
+    @Test
+    public void acceptExistingPath() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        tree.accept("README.md", visit -> {
+            assertThat(visit).isNotNull();
+            assertThat(visit.getRelativePath("/")).isEqualTo("README.md");
+            assertThat(visit.getPath()).exists();
+            assertThat(visit.getRoot()).isEqualTo(root);
+            try {
+                assertThat(Files.readString(visit.getPath())).isEqualTo("test readme");
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);
+            }
+        });
+    }
+
+    @Test
+    public void acceptNonExistentPath() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        tree.accept("non-existent", visit -> {
+            assertThat(visit).isNull();
+        });
+    }
+
+    @Test
+    public void acceptLegalAbsolutePath() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        tree.accept("/README.md", visit -> {
+            assertThat(visit).isNotNull();
+            assertThat(visit.getRelativePath("/")).isEqualTo("README.md");
+            assertThat(visit.getPath()).exists();
+            assertThat(visit.getRoot()).isEqualTo(root);
+            try {
+                assertThat(Files.readString(visit.getPath())).isEqualTo("test readme");
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);
+            }
+        });
+    }
+
+    @Test
+    @DisabledOnOs(OS.WINDOWS)
+    public void acceptIllegalAbsolutePathOnLinux() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        final Path absolute = root.getParent().resolve("external.txt");
+        assertThat(absolute).exists();
+        tree.accept(absolute.toString(), visit -> {
+            assertThat(visit).isNull();
+        });
+    }
+
+    @Test
+    @EnabledOnOs(OS.WINDOWS)
+    public void acceptIllegalAbsolutePathOnWindows() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        final Path absolute = root.getParent().resolve("external.txt");
+        assertThat(absolute).exists();
+        try {
+            tree.accept(absolute.toString(), visit -> {
+                fail("Windows absolute paths are not allowed");
+            });
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void acceptExistingRelativeNonNormalizedPath() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        tree.accept("../root/./other/../README.md", visit -> {
+            assertThat(visit).isNotNull();
+            assertThat(visit.getRelativePath("/")).isEqualTo("README.md");
+            assertThat(visit.getPath()).exists();
+            assertThat(visit.getRoot()).isEqualTo(root);
+            try {
+                assertThat(Files.readString(visit.getPath())).isEqualTo("test readme");
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);
+            }
+        });
+    }
+
+    @Test
+    public void acceptNonExistentRelativeNonNormalizedPath() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        tree.accept("../root/./README.md/../non-existent.txt", visit -> {
+            assertThat(visit).isNull();
+        });
+    }
+
+    @Test
+    public void walk() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+
+        final Set<String> visited = new HashSet<>();
+        final PathVisitor visitor = new PathVisitor() {
+            @Override
+            public void visitPath(PathVisit visit) {
+                visited.add(visit.getRelativePath("/"));
+            }
+        };
+        tree.walk(visitor);
+
+        assertThat(visited).isEqualTo(Set.of(
+                "",
+                "README.md",
+                "src",
+                "src/main",
+                "src/main/java",
+                "src/main/java/Main.java"));
+    }
+
+    /**
+     * Returns a path relative to src/test/resources/paths/directory-path-tree/
+     * 
+     * @param relative relative path
+     * @return Path instance
+     */
+    private Path resolveTreeRoot(String relative) {
+        return baseDir.resolve(relative);
+    }
+}