github · atorralba · Feb 16, 2022 · Jan 23, 2022 · Feb 8, 2022 · Feb 9, 2022
@@ -0,0 +1,21 @@
+// BAD: no file download validation
+HttpServletRequest request = getRequest();
+String path = request.getParameter("path");
+String filePath = "/pages/" + path;
+HttpServletResponse resp = getResponse();
+File file = new File(filePath);
+resp.getOutputStream().write(file.readContent());
+
+// BAD: no file upload validation
+String savePath = getPara("dir");
+File file = getFile("fileParam").getFile();
+FileInputStream fis = new FileInputStream(file);
+String filePath = "/files/" + savePath;
+FileOutputStream fos = new FileOutputStream(filePath);
+
+// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
+// (alternatively use `Path.normalize` instead of checking for `..`)
+if (!filePath.contains("..") && filePath.hasPrefix("/pages")) { ... }
+// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
+// (alternatively use `URLDecoder.decode` before `hasPrefix`)
+if (filePath.hasPrefix("/files") && !filePath.contains("%")) { ... }
@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>External Control of File Name or Path, also called File Path Injection, is a vulnerability by which
+a file path is created using data from outside the application (such as the HTTP request). It allows
+an attacker to traverse through the filesystem and access arbitrary files.</p>
+</overview>
+
+<recommendation>
+<p>Unsanitized user-provided data must not be used to construct file paths. In order to prevent File
+Path Injection, it is recommended to avoid concatenating user input directly into the file path. Instead, 
+user input should be checked against allowed or disallowed paths (for example, the path must be within
+<code>/user_content/</code> or must not be within <code>/internal</code>), ensuring that neither path
+traversal using <code>../</code> nor URL encoding is used to evade these checks.
+</p>
+</recommendation>
+
+<example>
+<p>The following examples show the bad case and the good case respectively.
+The <code>BAD</code> methods show an HTTP request parameter being used directly to construct a file path
+without validating the input, which may cause file leakage. In the <code>GOOD</code> method, the file path
+is validated.
+</p>
+<sample src="FilePathInjection.java" />
+</example>
+
+<references>
+<li>OWASP:
+  <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
+</li>
+<li>Veracode:
+  <a href="https://www.veracode.com/security/dotnet/cwe-73">External Control of File Name or Path Flaw</a>.
+</li>
+</references>
+</qhelp>
@@ -0,0 +1,43 @@
+/**
+ * @name File Path Injection
+ * @description Loading files based on unvalidated user-input may cause file information disclosure
+ *              and uploading files with unvalidated file types to an arbitrary directory may lead to
+ *              Remote Command Execution (RCE).
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/file-path-injection
+ * @tags security
+ *       external/cwe-073
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.PathCreation
+import JFinalController
+import experimental.semmle.code.java.PathSanitizer
+import DataFlow::PathGraph
+
+class InjectFilePathConfig extends TaintTracking::Configuration {
+  InjectFilePathConfig() { this = "InjectFilePathConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(PathCreation p).getAnInput() and
+    not sink instanceof NormalizedPathNode
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof PathTraversalBarrierGuard
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, InjectFilePathConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "External control of file name or path due to $@.",
+  source.getNode(), "user-provided value"
@@ -0,0 +1,83 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+
+/** The class `com.jfinal.core.Controller`. */
+class JFinalController extends RefType {
+  JFinalController() { this.hasQualifiedName("com.jfinal.core", "Controller") }
+}
+
+/** The method `getSessionAttr` of `JFinalController`. */
+class GetSessionAttributeMethod extends Method {
+  GetSessionAttributeMethod() {
+    this.getName() = "getSessionAttr" and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/** The method `setSessionAttr` of `JFinalController`. */
+class SetSessionAttributeMethod extends Method {
+  SetSessionAttributeMethod() {
+    this.getName() = "setSessionAttr" and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/** A request attribute getter method of `JFinalController`. */
+class GetRequestAttributeMethod extends Method {
+  GetRequestAttributeMethod() {
+    this.getName().matches("getAttr%") and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/** A request attribute setter method of `JFinalController`. */
+class SetRequestAttributeMethod extends Method {
+  SetRequestAttributeMethod() {
+    this.getName() = ["set", "setAttr"] and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/**
+ * Value step from a setter call to a corresponding getter call relating to a
+ * session or request attribute.
+ */
+private class SetToGetAttributeStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(MethodAccess gma, MethodAccess sma |
+      (
+        gma.getMethod() instanceof GetSessionAttributeMethod and
+        sma.getMethod() instanceof SetSessionAttributeMethod
+        or
+        gma.getMethod() instanceof GetRequestAttributeMethod and
+        sma.getMethod() instanceof SetRequestAttributeMethod
+      ) and
+      gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() =
+        sma.getArgument(0).(CompileTimeConstantExpr).getStringValue()
+    |
+      pred.asExpr() = sma.getArgument(1) and
+      succ.asExpr() = gma
+    )
+  }
+}
+
+/** Remote flow source models relating to `JFinal`. */
+private class JFinalControllerSource extends SourceModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
+          ";;;ReturnValue;remote",
+        "com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
+        "com.jfinal.core;Controller;true;getHeader;;;ReturnValue;remote",
+        "com.jfinal.core;Controller;true;getKv;;;ReturnValue;remote",
+        "com.jfinal.core;Controller;true;getPara" +
+          [
+            "", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
+            "ValuesToLong"
+          ] + ";;;ReturnValue;remote",
+        "com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
+          ";;;ReturnValue;remote"
+      ]
+  }
+}
@@ -0,0 +1,193 @@
+import java
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.dataflow.FlowSources
+
+/** A barrier guard that protects against path traversal vulnerabilities. */
+abstract class PathTraversalBarrierGuard extends DataFlow::BarrierGuard { }
+
+/**
+ * A guard that considers safe a string being exactly compared to a trusted value.
+ */
+private class ExactStringPathMatchGuard extends PathTraversalBarrierGuard instanceof MethodAccess {
+  ExactStringPathMatchGuard() {
+    super.getMethod().getDeclaringType() instanceof TypeString and
+    super.getMethod().getName() = ["equals", "equalsIgnoreCase"]
+  }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = super.getQualifier() and
+    branch = true
+  }
+}
+
+private class AllowListGuard extends Guard instanceof MethodAccess {
+  AllowListGuard() {
+    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
+    not isDisallowedWord(super.getAnArgument())
+  }
+
+  Expr getCheckedExpr() { result = super.getQualifier() }
+}
+
+/**
+ * A guard that considers a path safe because it is checked against an allowlist of partial trusted values.
+ * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
+ * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
+ */
+private class AllowListBarrierGuard extends PathTraversalBarrierGuard instanceof AllowListGuard {
+  override predicate checks(Expr e, boolean branch) {
+    e = super.getCheckedExpr() and
+    branch = true and
+    (
+      // Either a path normalization sanitizer comes before the guard,
+      exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
+      or
+      // or a check like `!path.contains("..")` comes before the guard
+      exists(PathTraversalGuard previousGuard |
+        DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
+        previousGuard.controls(this.getBasicBlock().(ConditionBlock), false)
+      )
+    )
+  }
+}
+
+/**
+ * A guard that considers a path safe because it is checked for `..` components, having previously
+ * been checked for a trusted prefix.
+ */
+private class DotDotCheckBarrierGuard extends PathTraversalBarrierGuard instanceof PathTraversalGuard {
+  override predicate checks(Expr e, boolean branch) {
+    e = super.getCheckedExpr() and
+    branch = false and
+    // The same value has previously been checked against a list of allowed prefixes:
+    exists(AllowListGuard previousGuard |
+      DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
+      previousGuard.controls(this.getBasicBlock().(ConditionBlock), true)
+    )
+  }
+}
+
+private class BlockListGuard extends Guard instanceof MethodAccess {
+  BlockListGuard() {
+    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
+    isDisallowedWord(super.getAnArgument())
+  }
+
+  Expr getCheckedExpr() { result = super.getQualifier() }
+}
+
+/**
+ * A guard that considers a string safe because it is checked against a blocklist of known dangerous values.
+ * This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`)
+ * or a sanitizer (`UrlDecodeSanitizer`).
+ */
+private class BlockListBarrierGuard extends PathTraversalBarrierGuard instanceof BlockListGuard {
+  override predicate checks(Expr e, boolean branch) {
+    e = super.getCheckedExpr() and
+    branch = false and
+    (
+      // Either `e` has been URL decoded:
+      exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
+      or
+      // or `e` has previously been checked for URL encoding sequences:
+      exists(UrlEncodingGuard previousGuard |
+        DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
+        previousGuard.controls(this.getBasicBlock(), false)
+      )
+    )
+  }
+}
+
+/**
+ * A guard that considers a string safe because it is checked for URL encoding sequences,
+ * having previously been checked against a block-list of forbidden values.
+ */
+private class URLEncodingBarrierGuard extends PathTraversalBarrierGuard instanceof UrlEncodingGuard {
+  override predicate checks(Expr e, boolean branch) {
+    e = super.getCheckedExpr() and
+    branch = false and
+    exists(BlockListGuard previousGuard |
+      DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
+      previousGuard.controls(this.getBasicBlock(), false)
+    )
+  }
+}
+
+/**
+ * Holds if `ma` is a call to a method that checks a partial string match.
+ */
+private predicate isStringPartialMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().getName() =
+    ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
+}
+
+/**
+ * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match.
+ */
+private predicate isPathPartialMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypePath and
+  ma.getMethod().getName() = "startsWith"
+}
+
+private predicate isDisallowedWord(CompileTimeConstantExpr word) {
+  word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
+}
+
+/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
+class PathTraversalGuard extends Guard instanceof MethodAccess {
+  Expr checked;
+
+  PathTraversalGuard() {
+    super.getMethod().getDeclaringType() instanceof TypeString and
+    super.getMethod().hasName(["contains", "indexOf"]) and
+    super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
+  }
+
+  Expr getCheckedExpr() { result = super.getQualifier() }
+}
+
+/** A complementary sanitizer that protects against path traversal using path normalization. */
+private class PathNormalizeSanitizer extends MethodAccess {
+  PathNormalizeSanitizer() {
+    this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
+    this.getMethod().hasName("normalize")
+  }
+}
+
+/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */
+private class UrlEncodingGuard extends Guard instanceof MethodAccess {
+  UrlEncodingGuard() {
+    super.getMethod().getDeclaringType() instanceof TypeString and
+    super.getMethod().hasName(["contains", "indexOf"]) and
+    super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
+  }
+
+  Expr getCheckedExpr() { result = super.getQualifier() }
+}
+
+/** A complementary sanitizer that protects against double URL encoding using URL decoding. */
+private class UrlDecodeSanitizer extends MethodAccess {
+  UrlDecodeSanitizer() {
+    this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
+    this.getMethod().hasName("decode")
+  }
+}
+
+/** A node with path normalization. */
+class NormalizedPathNode extends DataFlow::Node {
+  NormalizedPathNode() {
+    TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma))
+  }
+}
+
+/** Data model related to `java.nio.file.Path`. */
+private class PathDataModel extends SummaryModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;taint",
+        "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint"
+      ]
+  }
+}