Merge pull request #9956 from github/smowton/feature/tainted-path-query-mad

Make java/path-injection recognise create-file MaD sinks

java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -15,6 +15,7 @@
 
 import java
 import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.security.PathCreation
 import DataFlow::PathGraph
 import TaintedPathCommon
@@ -34,7 +35,12 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getAnInput() and not guarded(e))
+    (
+      sink.asExpr() = any(PathCreation p).getAnInput()
+      or
+      sinkNode(sink, "create-file")
+    ) and
+    not guarded(sink.asExpr())
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -44,9 +50,21 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, PathCreation p, TaintedPathConfig conf
-where
-  sink.getNode().asExpr() = p.getAnInput() and
-  conf.hasFlowPath(source, sink)
-select p, source, sink, "$@ flows to here and is used in a path.", source.getNode(),
-  "User-provided value"
+/**
+ * Gets the data-flow node at which to report a path ending at `sink`.
+ *
+ * Previously this query flagged alerts exclusively at `PathCreation` sites,
+ * so to avoid perturbing existing alerts, where a `PathCreation` exists we
+ * continue to report there; otherwise we report directly at `sink`.
+ */
+DataFlow::Node getReportingNode(DataFlow::Node sink) {
+  any(TaintedPathConfig c).hasFlowTo(sink) and
+  if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
+  then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
+  else result = sink
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, TaintedPathConfig conf
+where conf.hasFlowPath(source, sink)
+select getReportingNode(sink.getNode()), source, sink, "$@ flows to here and is used in a path.",
+  source.getNode(), "User-provided value"

java/ql/src/change-notes/2022-08-03-tainted-path-mad.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/path-injection` now recognises vulnerable APIs defined using the `SinkModelCsv` class with the `create-file` type. Out of the box this includes Apache Commons-IO functions, as well as any user-defined sinks.

java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected

@@ -8,6 +8,7 @@ edges
 | Test.java:79:74:79:97 | getInputStream(...) : ServletInputStream | Test.java:79:52:79:98 | new InputStreamReader(...) : InputStreamReader |
 | Test.java:80:31:80:32 | br : BufferedReader | Test.java:80:31:80:43 | readLine(...) : String |
 | Test.java:80:31:80:43 | readLine(...) : String | Test.java:82:67:82:81 | ... + ... |
+| Test.java:88:17:88:37 | getHostName(...) : String | Test.java:90:26:90:29 | temp |
 nodes
 | Test.java:19:18:19:38 | getHostName(...) : String | semmle.label | getHostName(...) : String |
 | Test.java:24:20:24:23 | temp | semmle.label | temp |
@@ -20,6 +21,8 @@ nodes
 | Test.java:80:31:80:32 | br : BufferedReader | semmle.label | br : BufferedReader |
 | Test.java:80:31:80:43 | readLine(...) : String | semmle.label | readLine(...) : String |
 | Test.java:82:67:82:81 | ... + ... | semmle.label | ... + ... |
+| Test.java:88:17:88:37 | getHostName(...) : String | semmle.label | getHostName(...) : String |
+| Test.java:90:26:90:29 | temp | semmle.label | temp |
 subpaths
 #select
 | Test.java:24:11:24:24 | new File(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:24:20:24:23 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
@@ -27,3 +30,4 @@ subpaths
 | Test.java:30:11:30:48 | getPath(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:30:44:30:47 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
 | Test.java:34:12:34:25 | new File(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:34:21:34:24 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
 | Test.java:82:52:82:88 | new FileWriter(...) | Test.java:79:74:79:97 | getInputStream(...) : ServletInputStream | Test.java:82:67:82:81 | ... + ... | $@ flows to here and is used in a path. | Test.java:79:74:79:97 | getInputStream(...) | User-provided value |
+| Test.java:90:26:90:29 | temp | Test.java:88:17:88:37 | getHostName(...) : String | Test.java:90:26:90:29 | temp | $@ flows to here and is used in a path. | Test.java:88:17:88:37 | getHostName(...) | User-provided value |

java/ql/test/query-tests/security/CWE-022/semmle/tests/Test.java

@@ -2,7 +2,6 @@
 // http://cwe.mitre.org/data/definitions/22.html
 package test.cwe22.semmle.tests;
 
-
 import javax.servlet.http.*;
 import javax.servlet.ServletException;
 
@@ -12,6 +11,7 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.FileSystems;
 
+import org.apache.commons.io.output.LockableFileWriter;
 
 class Test {
 	void doGet1(InetAddress address)
@@ -19,13 +19,13 @@ class Test {
 			String temp = address.getHostName();
 			File file;
 			Path path;
-			
+
 			// BAD: construct a file path with user input
 			file = new File(temp);
-			
+
 			// BAD: construct a path with user input
 			path = Paths.get(temp);
-					
+
 			// BAD: construct a path with user input
 			path = FileSystems.getDefault().getPath(temp);
 
@@ -34,7 +34,7 @@ class Test {
 				file = new File(temp);
 			}
 	}
-	
+
 	void doGet2(InetAddress address)
 		throws IOException {
 			String temp = address.getHostName();
@@ -44,7 +44,7 @@ class Test {
 			if(isSafe(temp))
 				file = new File(temp);
 	}
-	
+
 	void doGet3(InetAddress address)
 		throws IOException {
 			String temp = address.getHostName();
@@ -66,7 +66,7 @@ class Test {
 			return false;
 		return true;
 	}
-	
+
 	boolean isSortOfSafe(String pathSpec) {
 		// no file separators
 		if (pathSpec.contains(File.separator))
@@ -82,4 +82,11 @@ class Test {
             BufferedWriter bw = new BufferedWriter(new FileWriter("dir/"+filename, true));
         }
     }
+
+	void doGet4(InetAddress address)
+	throws IOException {
+		String temp = address.getHostName();
+		// BAD: open a file based on user input, using a MaD-documented API
+		new LockableFileWriter(temp);
+	}
 }

java/ql/test/query-tests/security/CWE-022/semmle/tests/options

@@ -1 +1 @@
-// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4
+// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4:${testdir}/../../../../../stubs/apache-commons-io-2.6

java/ql/test/stubs/apache-commons-io-2.6/org/apache/commons/io/output/LockableFileWriter.java

@@ -0,0 +1,7 @@
+package org.apache.commons.io.output;
+
+public class LockableFileWriter {
+
+  public LockableFileWriter(String filename) { }
+
+}