Correct the data model and update qldoc

java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.qhelp

@@ -5,24 +5,24 @@
 
 
 <overview>
-<p>External Control of File Name or Path, also called File Path Injection, is a vulnerability that a file path
-being accessed is composed using data from outside the application (such as the HTTP request, the database, or
+<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, the database, or
 the filesystem). 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 the file path. In order to prevent File
+<p>Unsanitized user-provided data must not be used to construct the file path. 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 (e.g., must come within <code>user_content/</code>) or disallowed
-(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
-or URL encoding are used to evade these checks.
+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, 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" />

java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql

@@ -13,36 +13,26 @@
 
 import java
 import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.PathCreation
 import JFinalController
 import PathSanitizer
 import DataFlow::PathGraph
 
-/**
- * A sink that represents a file read operation.
- */
-private class ReadFileSinkModels extends SinkModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        "java.io;FileInputStream;false;FileInputStream;;;Argument[0];read-file",
-        "java.io;File;false;File;;;Argument[0];read-file"
-      ]
-  }
-}
-
-/**
- * A sink that represents a file creation or access, such as a file read, write, copy or move operation.
- */
-private class FileAccessSink extends DataFlow::Node {
-  FileAccessSink() { sinkNode(this, "create-file") or sinkNode(this, "read-file") }
-}
-
 class InjectFilePathConfig extends TaintTracking::Configuration {
   InjectFilePathConfig() { this = "InjectFilePathConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource or
+    isGetAttributeFromRemoteSource(source.asExpr())
+  }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof FileAccessSink }
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(PathCreation p).getAnInput()
+  }
+
+  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

java/ql/src/experimental/Security/CWE/CWE-073/JFinalController.qll

@@ -1,31 +1,80 @@
 import java
+import semmle.code.java.dataflow.TaintTracking2
 import semmle.code.java.dataflow.FlowSources
 
-/** The class `com.jfinal.config.Routes`. */
-class JFinalRoutes extends RefType {
-  JFinalRoutes() { this.hasQualifiedName("com.jfinal.config", "Routes") }
-}
-
-/** The method `add` of the class `Routes`. */
-class AddJFinalRoutes extends Method {
-  AddJFinalRoutes() {
-    this.getDeclaringType() instanceof JFinalRoutes and
-    this.getName() = "add"
-  }
-}
-
 /** 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
+  }
+}
+
+/** The request attribute getter method of `JFinalController`. */
+class GetRequestAttributeMethod extends Method {
+  GetRequestAttributeMethod() {
+    this.getName().matches("getAttr%") and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/** The request attribute setter method of `JFinalController`. */
+class SetRequestAttributeMethod extends Method {
+  SetRequestAttributeMethod() {
+    this.getName() = ["set", "setAttr", "setAttrs"] and
+    this.getDeclaringType().getASupertype*() instanceof JFinalController
+  }
+}
+
+/** Taint configuration of flow from remote source to attribute setter methods. */
+class SetRemoteAttributeConfig extends TaintTracking2::Configuration {
+  SetRemoteAttributeConfig() { this = "SetRemoteAttributeConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof SetSessionAttributeMethod and
+      sink.asExpr() = ma.getArgument(1)
+    )
+  }
+}
+
+/** Holds if the result of an attribute getter call is from a method invocation of remote attribute setter. */
+predicate isGetAttributeFromRemoteSource(Expr expr) {
+  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
+    expr = gma and
+    gma.getArgument(0).(CompileTimeConstantExpr).getStringValue() =
+      sma.getArgument(0).(CompileTimeConstantExpr).getStringValue() and
+    exists(SetRemoteAttributeConfig cc | cc.hasFlowToExpr(sma.getArgument(1)))
+  )
+}
+
 /** Source model of remote flow source with `JFinal`. */
 private class JFinalControllerSource extends SourceModelCsv {
   override predicate row(string row) {
     row =
       [
-        "com.jfinal.core;Controller;true;getAttr" + ["", "ForInt", "ForStr"] +
-          ";;;ReturnValue;remote",
         "com.jfinal.core;Controller;true;getCookie" + ["", "Object", "Objects", "ToInt", "ToLong"] +
           ";;;ReturnValue;remote",
         "com.jfinal.core;Controller;true;getFile" + ["", "s"] + ";;;ReturnValue;remote",
@@ -36,7 +85,6 @@ private class JFinalControllerSource extends SourceModelCsv {
             "", "Map", "ToBoolean", "ToDate", "ToInt", "ToLong", "Values", "ValuesToInt",
             "ValuesToLong"
           ] + ";;;ReturnValue;remote",
-        "com.jfinal.core;Controller;true;getSession" + ["", "Attr"] + ";;;ReturnValue;remote",
         "com.jfinal.core;Controller;true;get" + ["", "Int", "Long", "Boolean", "Date"] +
           ";;;ReturnValue;remote"
       ]

java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected

@@ -1,23 +1,20 @@
 edges
 | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath |
-| FilePathInjection.java:61:50:61:58 | file : File | FilePathInjection.java:66:30:66:33 | file |
-| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath |
-| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath : String |
-| FilePathInjection.java:92:15:92:32 | new File(...) : File | FilePathInjection.java:100:19:100:22 | file : File |
-| FilePathInjection.java:92:24:92:31 | filePath : String | FilePathInjection.java:92:15:92:32 | new File(...) : File |
-| FilePathInjection.java:100:19:100:22 | file : File | FilePathInjection.java:61:50:61:58 | file : File |
+| FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath |
+| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath |
+| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath |
 nodes
 | FilePathInjection.java:20:21:20:34 | getPara(...) : String | semmle.label | getPara(...) : String |
 | FilePathInjection.java:25:47:25:59 | finalFilePath | semmle.label | finalFilePath |
-| FilePathInjection.java:61:50:61:58 | file : File | semmle.label | file : File |
-| FilePathInjection.java:66:30:66:33 | file | semmle.label | file |
-| FilePathInjection.java:88:17:88:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| FilePathInjection.java:92:15:92:32 | new File(...) : File | semmle.label | new File(...) : File |
-| FilePathInjection.java:92:24:92:31 | filePath | semmle.label | filePath |
-| FilePathInjection.java:92:24:92:31 | filePath : String | semmle.label | filePath : String |
-| FilePathInjection.java:100:19:100:22 | file : File | semmle.label | file : File |
+| FilePathInjection.java:40:21:40:34 | getPara(...) : String | semmle.label | getPara(...) : String |
+| FilePathInjection.java:43:25:43:37 | finalFilePath | semmle.label | finalFilePath |
+| FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | semmle.label | getSessionAttr(...) : String |
+| FilePathInjection.java:71:47:71:59 | finalFilePath | semmle.label | finalFilePath |
+| FilePathInjection.java:111:17:111:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| FilePathInjection.java:115:24:115:31 | filePath | semmle.label | filePath |
 subpaths
 #select
 | FilePathInjection.java:25:47:25:59 | finalFilePath | FilePathInjection.java:20:21:20:34 | getPara(...) : String | FilePathInjection.java:25:47:25:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:20:21:20:34 | getPara(...) | user-provided value |
-| FilePathInjection.java:66:30:66:33 | file | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:66:30:66:33 | file | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value |
-| FilePathInjection.java:92:24:92:31 | filePath | FilePathInjection.java:88:17:88:44 | getParameter(...) : String | FilePathInjection.java:92:24:92:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:88:17:88:44 | getParameter(...) | user-provided value |
+| FilePathInjection.java:43:25:43:37 | finalFilePath | FilePathInjection.java:40:21:40:34 | getPara(...) : String | FilePathInjection.java:43:25:43:37 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:40:21:40:34 | getPara(...) | user-provided value |
+| FilePathInjection.java:71:47:71:59 | finalFilePath | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) : String | FilePathInjection.java:71:47:71:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:65:29:65:55 | getSessionAttr(...) | user-provided value |
+| FilePathInjection.java:115:24:115:31 | filePath | FilePathInjection.java:111:17:111:44 | getParameter(...) : String | FilePathInjection.java:115:24:115:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:111:17:111:44 | getParameter(...) | user-provided value |

java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.java

@@ -58,6 +58,29 @@ public class FilePathInjection extends Controller {
 		}
 	}
 
+	// BAD: Upload file to user specified path without validation
+	public void uploadFile3() throws IOException {
+		String savePath = getPara("dir");
+		setSessionAttr("uploadDir", savePath);
+		String sessionUploadDir = getSessionAttr("uploadDir");
+
+		File file = getFile("fileParam").getFile();
+		String finalFilePath = BASE_PATH + sessionUploadDir;
+
+		FileInputStream fis = new FileInputStream(file);
+		FileOutputStream fos = new FileOutputStream(finalFilePath);
+		int i = 0;
+
+		do {
+			byte[] buf = new byte[1024];
+			i = fis.read(buf);
+			fos.write(buf);
+		} while (i != -1);
+
+		fis.close();
+		fos.close();
+	}
+
 	private void readFile(HttpServletResponse resp, File file) {
 		OutputStream os = null;
 		FileInputStream fis = null;

java/ql/test/stubs/jfinal-4.9.15/com/jfinal/core/Controller.java

@@ -41,6 +41,14 @@ public abstract class Controller {
 		return null;
 	}
 
+	public Controller setAttr(String name, Object value) {
+		return null;
+	}
+
+	public Controller setAttrs(Map<String, Object> attrMap) {
+		return null;
+	}
+
 	public Enumeration<String> getAttrNames() {
 		return null;
 	}
@@ -137,6 +145,10 @@ public abstract class Controller {
 		return null;
 	}
 
+	public Controller setSessionAttr(String key, Object value) {
+		return null;
+	}
+
 	public String getCookie(String name, String defaultValue) {
 		return null;
 	}
@@ -245,6 +257,10 @@ public abstract class Controller {
 		return null;
 	}
 
+	public Controller set(String attributeName, Object attributeValue) {
+		return null;
+	}
+
 	public String get(String name) {
 		return null;
 	}