sanitize path in {History/Annotation}Controller (#3794)

* sanitize path

* extract the check to toFile()

* use InvalidPathException

* remove unused exception

* remove unused import

* add warning suppression

* fix tests

* add more coverage

* update javadoc

* try to escape

* use relative path

* refactor

Author: Vladimir Kotal

opengrok-web/pom.xml

@@ -199,6 +199,11 @@ Portions Copyright (c) 2018, 2020, Chris Fraire <[email protected]>.
             <groupId>org.jetbrains</groupId>
             <artifactId>annotations</artifactId>
         </dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-params</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/AnnotationController.java

@@ -44,6 +44,8 @@
 
 import static org.opengrok.web.util.FileUtil.toFile;
 
+// No need to have PATH configurable.
+@SuppressWarnings("java:S1075")
 @Path(AnnotationController.PATH)
 public class AnnotationController {
 

opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/HistoryController.java

@@ -33,28 +33,31 @@
 import jakarta.ws.rs.core.Context;
 import jakarta.ws.rs.core.MediaType;
 import org.jetbrains.annotations.TestOnly;
-import org.opengrok.indexer.configuration.RuntimeEnvironment;
 import org.opengrok.indexer.history.History;
 import org.opengrok.indexer.history.HistoryEntry;
 import org.opengrok.indexer.history.HistoryException;
 import org.opengrok.indexer.history.HistoryGuru;
 import org.opengrok.indexer.web.messages.JSONable;
 import org.opengrok.web.api.v1.filter.CorsEnable;
 import org.opengrok.web.api.v1.filter.PathAuthorized;
+import org.opengrok.web.util.NoPathParameterException;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.SortedSet;
 
+import static org.opengrok.web.util.FileUtil.toFile;
+
+// No need to have PATH configurable.
+@SuppressWarnings("java:S1075")
 @Path(HistoryController.PATH)
 public final class HistoryController {
 
-    private static final RuntimeEnvironment env = RuntimeEnvironment.getInstance();
-
     private static final int MAX_RESULTS = 1000;
 
     public static final String PATH = "/history";
@@ -204,10 +207,11 @@ public HistoryDTO get(@Context HttpServletRequest request,
                           @QueryParam("withFiles") final boolean withFiles,
                           @QueryParam("max") @DefaultValue(MAX_RESULTS + "") final int maxEntries,
                           @QueryParam("start") @DefaultValue(0 + "") final int startIndex)
-            throws HistoryException {
+            throws HistoryException, IOException, NoPathParameterException {
+
+        File file = toFile(path);
 
-        History history = HistoryGuru.getInstance().getHistory(new File(env.getSourceRootFile(), path),
-                withFiles, true);
+        History history = HistoryGuru.getInstance().getHistory(file, withFiles, true);
         if (history == null) {
             return null;
         }

opengrok-web/src/main/java/org/opengrok/web/util/FileUtil.java

@@ -18,14 +18,16 @@
  */
 
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opengrok.web.util;
 
 import org.opengrok.indexer.configuration.RuntimeEnvironment;
 
 import java.io.File;
 import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
 
 public class FileUtil {
 
@@ -38,17 +40,25 @@ private FileUtil() {
     /**
      * @param path path relative to source root
      * @return file object corresponding to the file under source root
-     * @throws FileNotFoundException if the file constructed from the {@code path} parameter and source root does not exist
+     * @throws FileNotFoundException if the file constructed from the {@code path} parameter and source root
+     * does not exist
+     * @throws InvalidPathException if the file constructed from the {@code path} parameter and source root
+     * leads outside source root
      * @throws NoPathParameterException if the {@code path} parameter is null
      */
-    public static File toFile(String path) throws NoPathParameterException, FileNotFoundException {
+    public static File toFile(String path) throws NoPathParameterException, IOException {
         if (path == null) {
             throw new NoPathParameterException("Missing path parameter");
         }
 
         File file = new File(env.getSourceRootFile(), path);
-        if (!file.isFile()) {
-            throw new FileNotFoundException("File " + file.toString() + "not found");
+
+        if (!file.getCanonicalPath().startsWith(env.getSourceRootPath() + File.separator)) {
+            throw new InvalidPathException(path, "File points to outside of source root");
+        }
+
+        if (!file.exists()) {
+            throw new FileNotFoundException("File " + file + " not found");
         }
 
         return file;

opengrok-web/src/test/java/org/opengrok/web/util/FileUtilTest.java

@@ -18,31 +18,75 @@
  */
 
 /*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2020, Chris Fraire <[email protected]>.
  */
 package org.opengrok.web.util;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.opengrok.indexer.configuration.RuntimeEnvironment;
 
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.io.File;
 import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.UUID;
 
 /**
  * Represents a container for tests of {@link FileUtil}.
  */
-public class FileUtilTest {
+class FileUtilTest {
+
+    RuntimeEnvironment env = RuntimeEnvironment.getInstance();
 
     @Test
-    public void shouldThrowOnNullArgument() {
+    void shouldThrowOnNullArgument() {
         assertThrows(NoPathParameterException.class, () -> FileUtil.toFile(null),
                 "toFile(null)");
     }
 
+    private String setSourceRoot(String id) throws IOException {
+        String origRoot = env.getSourceRootPath();
+        Path dir = Files.createTempDirectory(id);
+        dir.toFile().deleteOnExit();
+        env.setSourceRoot(dir.toString());
+        assertTrue(env.getSourceRootFile().isDirectory());
+        return origRoot;
+    }
+
     @Test
-    public void shouldThrowOnMissingFile() {
-        assertThrows(FileNotFoundException.class, () -> FileUtil.toFile(
-                UUID.randomUUID().toString()), "toFile(randomUUID)");
+    void shouldThrowOnInvalidFile() throws IOException {
+        String origRoot = setSourceRoot("shouldThrowOnInvalidFile");
+        String rndPath = ".." + File.separator + UUID.randomUUID();
+        assertThrows(InvalidPathException.class, () -> FileUtil.toFile(rndPath),
+                "toFile(randomUUID)");
+        env.setSourceRoot(origRoot);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void shouldThrowOnMissingFile(boolean isPresent) throws IOException, NoPathParameterException {
+        String origRoot = setSourceRoot("shouldThrowOnMissingFile");
+        if (isPresent) {
+            String fileName = "existent";
+            Path filePath = Paths.get(env.getSourceRootPath(), fileName);
+            Files.createFile(filePath);
+            filePath.toFile().deleteOnExit();
+            assertTrue(filePath.toFile().exists());
+            assertNotNull(FileUtil.toFile(fileName));
+        } else {
+            assertThrows(FileNotFoundException.class, () -> FileUtil.toFile("nonexistent"),
+                    "toFile(nonexistent)");
+        }
+        env.setSourceRoot(origRoot);
     }
 }