Merge pull request #9742 from smehta23/feat/SM/java_partial_path_traversal_vulnerability

[JAVA] Partial Path Traversal Vuln Query

java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll

@@ -0,0 +1,60 @@
+/** Provides classes to reason about partial path traversal vulnerabilities. */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.environment.SystemProperty
+
+private class MethodStringStartsWith extends Method {
+  MethodStringStartsWith() {
+    this.getDeclaringType() instanceof TypeString and
+    this.hasName("startsWith")
+  }
+}
+
+private class MethodFileGetCanonicalPath extends Method {
+  MethodFileGetCanonicalPath() {
+    this.getDeclaringType() instanceof TypeFile and
+    this.hasName("getCanonicalPath")
+  }
+}
+
+private class MethodAccessFileGetCanonicalPath extends MethodAccess {
+  MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath }
+}
+
+abstract private class FileSeparatorExpr extends Expr { }
+
+private class SystemPropFileSeparatorExpr extends FileSeparatorExpr {
+  SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") }
+}
+
+private class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral {
+  StringLiteralFileSeparatorExpr() {
+    this.getValue().matches("%/") or this.getValue().matches("%\\")
+  }
+}
+
+private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral {
+  CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }
+}
+
+private class FileSeparatorAppend extends AddExpr {
+  FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }
+}
+
+private predicate isSafe(Expr expr) {
+  DataFlow::localExprFlow(any(Expr e |
+      e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr
+    ), expr)
+}
+
+/**
+ * A method access that returns a boolean that incorrectly guards against Partial Path Traversal.
+ */
+class PartialPathTraversalMethodAccess extends MethodAccess {
+  PartialPathTraversalMethodAccess() {
+    this.getMethod() instanceof MethodStringStartsWith and
+    DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), this.getQualifier()) and
+    not isSafe(this.getArgument(0))
+  }
+}

java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll

@@ -0,0 +1,23 @@
+/** Provides taint tracking configurations to be used in partial path traversal queries. */
+
+import java
+import semmle.code.java.security.PartialPathTraversal
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.ExternalFlow
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
+
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to validate against path traversal, but is insufficient
+ * and remains vulnerable to Partial Path Traversal.
+ */
+class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration {
+  PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" }
+
+  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    any(PartialPathTraversalMethodAccess ma).getQualifier() = node.asExpr()
+  }
+}

java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp

@@ -0,0 +1,15 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>A common way to check that a user-supplied path <code>SUBDIR</code> falls inside a directory <code>DIR</code>
+is to use <code>getCanonicalPath()</code> to remove any path-traversal elements and then check that <code>DIR</code>
+is a prefix. However, if <code>DIR</code> is not slash-terminated, this can unexpectedly allow access to siblings of <code>DIR</code>.</p>
+
+<p>See also <code>java/partial-path-traversal-from-remote</code>, which is similar to this query but only flags instances with evidence of remote exploitability.</p>
+</overview>
+
+<include src="PartialPathTraversalRemainder.inc.qhelp"/>
+
+</qhelp>

java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql

@@ -0,0 +1,16 @@
+/**
+ * @name Partial path traversal vulnerability
+ * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 9.3
+ * @precision medium
+ * @id java/partial-path-traversal
+ * @tags security
+ *       external/cwe/cwe-023
+ */
+
+import semmle.code.java.security.PartialPathTraversal
+
+from PartialPathTraversalMethodAccess ma
+select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal"

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java

@@ -0,0 +1,7 @@
+public class PartialPathTraversalBad {
+    public void example(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+}

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp

@@ -0,0 +1,16 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>A common way to check that a user-supplied path <code>SUBDIR</code> falls inside a directory <code>DIR</code>
+is to use <code>getCanonicalPath()</code> to remove any path-traversal elements and then check that <code>DIR</code>
+is a prefix. However, if <code>DIR</code> is not slash-terminated, this can unexpectedly allow accessing siblings of <code>DIR</code>.</p>
+
+<p>See also <code>java/partial-path-traversal</code>, which is similar to this query, 
+but may also flag non-remotely-exploitable instances of partial path traversal vulnerabilities.</p>
+</overview>
+
+<include src="PartialPathTraversalRemainder.inc.qhelp"/>
+
+</qhelp>

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Partial path traversal vulnerability from remote
+ * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.3
+ * @precision high
+ * @id java/partial-path-traversal-from-remote
+ * @tags security
+ *       external/cwe/cwe-023
+ */
+
+import semmle.code.java.security.PartialPathTraversalQuery
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink
+where any(PartialPathTraversalFromRemoteConfig config).hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Partial Path Traversal Vulnerability due to insufficient guard against path traversal from user-supplied data"

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java

@@ -0,0 +1,7 @@
+public class PartialPathTraversalGood {
+    public void example(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().toPath().startsWith(parent.getCanonicalPath().toPath())) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+}

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<recommendation>
+
+<p>If the user should only access items within a certain directory <code>DIR</code>, ensure that <code>DIR</code> is slash-terminated
+before checking that <code>DIR</code> is a prefix of the user-provided path, <code>SUBDIR</code>. Note, Java's <code>getCanonicalPath()</code>
+returns a <b>non</b>-slash-terminated path string, so a slash must be added to <code>DIR</code> if that method is used.</p>
+
+</recommendation>
+<example>
+
+<p> 
+
+
+In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath()</code> 
+is a prefix of <code>dir.getCanonicalPath()</code>. However, <code>parent.getCanonicalPath()</code> is 
+not slash-terminated.  This means that users that supply <code>dir</code> may be also allowed to access siblings of <code>parent</code> 
+and not just children of <code>parent</code>, which is a security issue.
+
+</p>
+
+<sample src="PartialPathTraversalBad.java" />
+
+<p>
+
+In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath() + File.separator </code> 
+is a prefix of <code>dir.getCanonicalPath()</code>. Because <code>parent.getCanonicalPath().toPath()</code> is 
+indeed slash-terminated, the user supplying <code>dir</code> can only access children of 
+<code>parent</code>, as desired.
+
+</p>
+
+<sample src="PartialPathTraversalGood.java" />
+
+</example>
+<references>
+
+<li>OWASP:
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Partial Path Traversal</a>.</li>
+<li>CVE-2022-23457:
+<a href="https://github.com/ESAPI/esapi-java-legacy/blob/develop/documentation/GHSL-2022-008_The_OWASP_Enterprise_Security_API.md"> ESAPI Vulnerability Report</a>.</li>
+
+</references>
+
+
+</qhelp>

java/ql/src/change-notes/2022-07-01-partial-path-traversal.md

@@ -0,0 +1,5 @@
+---
+category: newQuery
+---
+* A new query `java/partial-path-traversal` finds partial path traversal vulnerabilities resulting from incorrectly using 
+`String#startsWith` to compare canonical paths. 

java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected

@@ -0,0 +1,16 @@
+| PartialPathTraversalTest.java:10:14:10:73 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:17:9:17:72 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:29:14:29:58 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:35:14:35:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:42:14:42:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:49:14:49:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:53:14:53:65 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:61:14:61:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:64:14:64:65 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:75:14:75:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:94:14:94:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:102:14:102:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:105:14:105:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:173:14:173:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:191:18:191:87 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |
+| PartialPathTraversalTest.java:209:14:209:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal |

java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-023/PartialPathTraversal.ql

java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql

@@ -0,0 +1,17 @@
+import java
+import TestUtilities.InlineFlowTest
+import semmle.code.java.security.PartialPathTraversalQuery
+
+class TestRemoteSource extends RemoteFlowSource {
+  TestRemoteSource() { this.asParameter().hasName(["dir", "path"]) }
+
+  override string getSourceType() { result = "TestSource" }
+}
+
+class Test extends InlineFlowTest {
+  override DataFlow::Configuration getValueFlowConfig() { none() }
+
+  override TaintTracking::Configuration getTaintFlowConfig() {
+    result instanceof PartialPathTraversalFromRemoteConfig
+  }
+}

java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java

@@ -0,0 +1,239 @@
+import java.io.IOException;
+import java.io.File;
+import java.io.InputStream;
+import static java.io.File.separatorChar;
+import java.nio.file.Files;
+
+
+public class PartialPathTraversalTest {
+    public void esapiExample(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    @SuppressWarnings("ResultOfMethodCallIgnored")
+    void foo1(File dir, File parent) throws IOException {
+        (dir.getCanonicalPath()).startsWith((parent.getCanonicalPath())); // $hasTaintFlow
+    }
+
+    void foo2(File dir, File parent) throws IOException {
+        dir.getCanonicalPath();
+        if ("potato".startsWith(parent.getCanonicalPath())) {
+            System.out.println("Hello!");
+        }
+    }
+
+    void foo3(File dir, File parent) throws IOException {
+        String parentPath = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentPath)) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo4(File dir) throws IOException {
+        if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo5(File dir, File parent) throws IOException {
+        String canonicalPath = dir.getCanonicalPath();
+        if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo6(File dir, File parent) throws IOException {
+        String canonicalPath = dir.getCanonicalPath();
+        if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+        String canonicalPath2 = dir.getCanonicalPath();
+        if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo7(File dir, File parent) throws IOException {
+        String canonicalPath = dir.getCanonicalPath();
+        String canonicalPath2 = dir.getCanonicalPath();
+        if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+        if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    File getChild() {
+        return null;
+    }
+
+    void foo8(File parent) throws IOException {
+        String canonicalPath = getChild().getCanonicalPath();
+        if (!canonicalPath.startsWith(parent.getCanonicalPath())) { 
+            throw new IOException("Invalid directory: " + getChild().getCanonicalPath());
+        }
+    }
+
+    void foo9(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo10(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separatorChar)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo11(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo12(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        String parentCanonical2 = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+        if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo13(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath() + File.separatorChar;
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo14(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath() + separatorChar;
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) { 
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo15(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath() + File.separatorChar;
+        String parentCanonical2 = parent.getCanonicalPath() + File.separatorChar;
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+        if (!dir.getCanonicalPath().startsWith(parentCanonical2)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo16(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath() + File.separator;
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    @SuppressWarnings({
+            "IfStatementWithIdenticalBranches",
+            "MismatchedStringCase",
+            "UnusedAssignment",
+            "ResultOfMethodCallIgnored"
+    })
+    void foo17(File dir, File parent, boolean branch) throws IOException {
+        String parentCanonical = null;
+        "test ".startsWith("somethingElse");
+        if (branch) {
+            parentCanonical = parent.getCanonicalPath() + File.separatorChar;
+        } else {
+            parentCanonical = parent.getCanonicalPath() + File.separatorChar;
+        }
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo18(File dir, File parent, boolean branch) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        if (branch) {
+            parentCanonical = parent.getCanonicalPath() + File.separatorChar;
+        }
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo19(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath() + "/potato";
+        if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    private File cacheDir;
+
+    InputStream foo20(String... path) {
+        StringBuilder sb = new StringBuilder();
+        sb.append(cacheDir.getAbsolutePath());
+        for (String p : path) {
+            sb.append(File.separatorChar);
+            sb.append(p);
+        }
+        sb.append(".gz");
+        String filePath = sb.toString();
+        File encodedFile = new File(filePath);
+        try {
+            if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) { // $hasTaintFlow 
+                return null;
+            }
+            return Files.newInputStream(encodedFile.toPath());
+        } catch (Exception e) {
+            return null;
+        }
+    }
+
+    void foo21(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentCanonical + File.separator)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo22(File dir, File dir2, File parent, boolean conditional) throws IOException {
+        String canonicalPath = conditional ? dir.getCanonicalPath() : dir2.getCanonicalPath();
+        if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo23(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentCanonical + "/")) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    void foo24(File dir, File parent) throws IOException {
+        String parentCanonical = parent.getCanonicalPath();
+        if (!dir.getCanonicalPath().startsWith(parentCanonical + '/')) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+
+    public void doesNotFlag() {
+        "hello".startsWith("goodbye");
+    }
+
+    public void doesNotFlagBackslash(File file) throws IOException {
+        // https://github.com/jenkinsci/jenkins/blob/be3cf6bffe7aa2fe2307c424fa418519f3bbd73b/core/src/main/java/hudson/util/jna/Kernel32Utils.java#L77-L77
+        if (!file.getCanonicalPath().startsWith("\\\\")) {
+            throw new RuntimeException("Boom");
+        }
+    }
+
+}