Merge pull request #266 from ncats/aw_a26_pathtraversal

WIP: path traversal mitigation attempt raised snyk analysis
ncats · Jul 2, 2024 · 9247c14 · 9247c14
2 parents edb481a + 93a2cb2
commit 9247c14
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 10 deletions.
diff --git a/gsrs-spring-boot-autoconfigure/src/main/java/gsrs/config/FilePathParserUtils.java b/gsrs-spring-boot-autoconfigure/src/main/java/gsrs/config/FilePathParserUtils.java
@@ -1,7 +1,10 @@
 package gsrs.config;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.time.format.DateTimeFormatter;
 
 import gov.nih.ncats.common.util.TimeUtil;
@@ -78,4 +81,26 @@ private static File getDefaultRootDir() {
         return null;
     }
 
+    public static void failOnBadPathResolution(String basePath, String passedPath)
+    throws InvalidPathException, IOException {
+        Path basePathObj;
+        Path resolvedPathObj;
+        try {
+            basePathObj = Paths.get(basePath);
+            resolvedPathObj = basePathObj.resolve(passedPath);
+//            System.out.println("getPath       : " + resolvedPathObj.toFile().getPath());
+//            System.out.println("getAbsPath    : " + resolvedPathObj.toFile().getAbsolutePath());
+//            System.out.println("getNormAbsPath: " + resolvedPathObj.normalize().toAbsolutePath());
+        } catch (InvalidPathException ipe) {
+            // Don't want to provide input path as it might get flagged.
+            throw new InvalidPathException("Relating to passedPath", "Exception while resolving path");
+        }
+        if (Paths.get(passedPath).isAbsolute()) {
+            throw new IOException("Absolute path not allowed.");
+        }
+        if (!resolvedPathObj.normalize().startsWith(basePathObj)) {
+            throw new IOException("Unexpected start of path.");
+        }
+    }
+
 }
diff --git a/gsrs-spring-boot-autoconfigure/src/main/java/gsrs/controller/ExportController.java b/gsrs-spring-boot-autoconfigure/src/main/java/gsrs/controller/ExportController.java
@@ -1,5 +1,6 @@
 package gsrs.controller;
 
+import gsrs.config.FilePathParserUtils;
 import gsrs.service.ExportService;
 import ix.ginas.exporters.ExportDir;
 import ix.ginas.exporters.ExportMetaData;
@@ -184,8 +185,6 @@ public ResponseEntity<Object> download(@PathVariable("id") @Pattern(regexp="^[a-
 
         File f = exportFile.get().getFile();
 
-
-
         Path path = Paths.get(f.getAbsolutePath());
         ByteArrayResource resource = new ByteArrayResource(Files.readAllBytes(path));
 

diff --git a/gsrs-spring-boot-autoconfigure/src/test/java/gsrs/config/TestFileParserHelper.java b/gsrs-spring-boot-autoconfigure/src/test/java/gsrs/config/TestFileParserHelper.java
@@ -1,13 +1,10 @@
 package gsrs.config;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
 import java.io.File;
 import java.io.IOException;
-
 import org.junit.jupiter.api.Test;
-
 import gsrs.config.FilePathParserUtils.FileParser;
+import static org.junit.jupiter.api.Assertions.*;
 
 
 public class TestFileParserHelper {
@@ -99,8 +96,6 @@ public void nullSuppliedGivenDefaultFilePathWithSetRelativeRootShouldBeEffective
 
         assertEquals(new File("test").toPath().normalize(), f.toPath().normalize());
     }
-
-
 
     @Test
     public void nullSuppliedGivenDefaultRelativeWithSetRootShouldBeAbsolute() throws IOException {
@@ -112,6 +107,70 @@ public void nullSuppliedGivenDefaultRelativeWithSetRootShouldBeAbsolute() throws
 
         assertEquals(new File("/tmp/test"), f);
     }
-
-
+
+    @Test
+    public void testFailOnBadPathResolutionNormal() throws IOException {
+        boolean hasException = false;
+        try {
+            // Should succeed since normalized path ok
+            FilePathParserUtils.failOnBadPathResolution("/tmp", "my-file.txt");
+        } catch(Exception e) {
+            System.out.println(e.getMessage());
+            hasException = true;
+        }
+        assertFalse(hasException);
+    }
+
+    @Test
+    public void testFailOnBadPathResolutionSingleDot1() throws IOException {
+        boolean hasException = false;
+        try {
+            // Should succeed since path resolves to /tmp/my-file.txt
+            FilePathParserUtils.failOnBadPathResolution("/tmp", "./my-file.txt");
+        } catch(Exception e) {
+            System.out.println(e.getMessage());
+            hasException = true;
+        }
+        assertFalse(hasException);
+    }
+
+    @Test
+    public void testFailOnBadPathResolutionDotDot1() throws IOException {
+        boolean hasException = false;
+        try {
+            // Should succeed since path resolves to /tmp/my-file.txt
+            FilePathParserUtils.failOnBadPathResolution("/tmp", "../tmp/my-file.txt");
+        } catch(Exception e) {
+            System.out.println(e.getMessage());
+            hasException = true;
+        }
+        assertFalse(hasException);
+    }
+
+    @Test
+    public void testFailOnBadPathResolutionDotDot2() throws IOException {
+        boolean hasException = false;
+        try {
+            // Should fail since path resolves to /etc/my-file.txt
+            FilePathParserUtils.failOnBadPathResolution("/tmp", "../etc/my-file.txt");
+        } catch(Exception e) {
+            System.out.println(e.getMessage());
+            hasException = true;
+        }
+        assertTrue(hasException);
+    }
+
+    @Test
+    public void testFailOnBadPathResolutionAbsolutePathException() throws IOException {
+        boolean hasException = false;
+        try {
+            // Should fail due to absolute path
+            FilePathParserUtils.failOnBadPathResolution("/tmp", "/my/abs/path");
+        } catch(Exception e) {
+            System.out.println(e.getMessage());
+            hasException = true;
+        }
+        assertTrue(hasException);
+    }
+
 }