github · smowton · Aug 15, 2022 · Jun 28, 2022 · Jun 28, 2022 · Jun 28, 2022
@@ -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))
+  }
+}
@@ -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()
+  }
+}
@@ -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>
@@ -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"
@@ -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());
+        }
+    }
+}
@@ -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>
@@ -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"
@@ -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());
+        }
+    }
+}
@@ -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>
@@ -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. 
@@ -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 |
@@ -0,0 +1 @@
+Security/CWE/CWE-023/PartialPathTraversal.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
+  }
+}