From 3f1b519f975e06f8f62c18dc5d90307bd161287d Mon Sep 17 00:00:00 2001
From: Sebastian Sdorra <sebastian.sdorra@cloudogu.com>
Date: Mon, 17 Jan 2022 15:33:42 +0100
Subject: [PATCH] Fix path traversal vulnerability

---
 .../scm/plugin/PathWebResourceLoader.java     | 37 ++++++++---------
 .../scm/plugin/PathWebResourceLoaderTest.java | 41 ++++++++++++++++---
 2 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/scm-webapp/src/main/java/sonia/scm/plugin/PathWebResourceLoader.java b/scm-webapp/src/main/java/sonia/scm/plugin/PathWebResourceLoader.java
index 8dbd29ef03..fd8889f5b7 100644
--- a/scm-webapp/src/main/java/sonia/scm/plugin/PathWebResourceLoader.java
+++ b/scm-webapp/src/main/java/sonia/scm/plugin/PathWebResourceLoader.java
@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
+
 package sonia.scm.plugin;
 
 import org.slf4j.Logger;
@@ -38,37 +38,32 @@ import java.nio.file.Path;
  * @author Sebastian Sdorra
  * @since 2.0.0
  */
-public class PathWebResourceLoader implements WebResourceLoader
-{
+public class PathWebResourceLoader implements WebResourceLoader {
 
   private static final String SEPARATOR = "/";
 
+  private final Path directory;
+
   /**
    * the logger for PathWebResourceLoader
    */
-  private static final Logger LOG =
-    LoggerFactory.getLogger(PathWebResourceLoader.class);
+  private static final Logger LOG = LoggerFactory.getLogger(PathWebResourceLoader.class);
 
-  public PathWebResourceLoader(Path directory)
-  {
-    this.directory = directory;
+  public PathWebResourceLoader(Path directory) {
+    this.directory = directory.toAbsolutePath().normalize();
   }
 
   @Override
   public URL getResource(String path) {
     URL resource = null;
-    Path file = directory.resolve(filePath(path));
+    Path file = directory.resolve(filePath(path)).toAbsolutePath().normalize();
 
-    if (Files.exists(file) && ! Files.isDirectory(file))
-    {
+    if (isValidPath(file)) {
       LOG.trace("found path {} at {}", path, file);
 
-      try
-      {
+      try {
         resource = file.toUri().toURL();
-      }
-      catch (MalformedURLException ex)
-      {
+      } catch (MalformedURLException ex) {
         LOG.error("could not transform path to url", ex);
       }
     } else {
@@ -78,6 +73,12 @@ public class PathWebResourceLoader implements WebResourceLoader
     return resource;
   }
 
+  private boolean isValidPath(Path file) {
+    return Files.exists(file)
+      && !Files.isDirectory(file)
+      && file.startsWith(directory);
+  }
+
   private String filePath(String path) {
     if (path.startsWith(SEPARATOR)) {
       return path.substring(1);
@@ -85,8 +86,4 @@ public class PathWebResourceLoader implements WebResourceLoader
     return path;
   }
 
-  //~--- fields ---------------------------------------------------------------
-
-  /** Field description */
-  private final Path directory;
 }
diff --git a/scm-webapp/src/test/java/sonia/scm/plugin/PathWebResourceLoaderTest.java b/scm-webapp/src/test/java/sonia/scm/plugin/PathWebResourceLoaderTest.java
index ce9ca8d497..b8075bd2f5 100644
--- a/scm-webapp/src/test/java/sonia/scm/plugin/PathWebResourceLoaderTest.java
+++ b/scm-webapp/src/test/java/sonia/scm/plugin/PathWebResourceLoaderTest.java
@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
+
 package sonia.scm.plugin;
 
 //~--- non-JDK imports --------------------------------------------------------
@@ -42,11 +42,10 @@ import java.net.URL;
  *
  * @author Sebastian Sdorra
  */
-public class PathWebResourceLoaderTest extends WebResourceLoaderTestBase
-{
+public class PathWebResourceLoaderTest extends WebResourceLoaderTestBase {
 
   @Test
-  public void testGetNullForDirectories() throws IOException {
+  public void shouldReturnNullForDirectories() throws IOException {
     File directory = temp.newFolder();
     assertTrue(new File(directory, "awesome").mkdir());
 
@@ -56,7 +55,7 @@ public class PathWebResourceLoaderTest extends WebResourceLoaderTestBase
 
 
   @Test
-  public void testGetResource() throws IOException {
+  public void shouldReturnResource() throws IOException {
     File directory = temp.newFolder();
     URL url = file(directory, "myresource").toURI().toURL();
 
@@ -68,4 +67,36 @@ public class PathWebResourceLoaderTest extends WebResourceLoaderTestBase
     assertNull(resourceLoader.getResource("other"));
   }
 
+  @Test
+  public void shouldNotReturnPathsWithAbsolutePath() throws IOException {
+    File base = temp.newFolder();
+
+    File one = new File(base, "one");
+    assertTrue(one.mkdirs());
+    File two = new File(base, "two");
+    assertTrue(two.mkdirs());
+
+    File secret = new File(two, "secret");
+    assertTrue(secret.createNewFile());
+
+    WebResourceLoader resourceLoader = new PathWebResourceLoader(one.toPath());
+    assertNull(resourceLoader.getResource(secret.getAbsolutePath()));
+    assertNull(resourceLoader.getResource("/" + secret.getAbsolutePath()));
+  }
+
+  @Test
+  public void shouldNotReturnPathsWithPathTraversal() throws IOException {
+    File base = temp.newFolder();
+
+    File one = new File(base, "one");
+    assertTrue(one.mkdirs());
+    File two = new File(base, "two");
+    assertTrue(two.mkdirs());
+
+    File secret = new File(two, "secret");
+    assertTrue(secret.createNewFile());
+
+    WebResourceLoader resourceLoader = new PathWebResourceLoader(one.toPath());
+    assertNull(resourceLoader.getResource("../two/secret"));
+  }
 }