Make reporting locations consistent with PathCreation; add test

Author: smowton

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

@@ -50,7 +50,21 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   }
 }
 
+/**
+ * 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 sink, source, sink, "$@ flows to here and is used in a path.", source.getNode(),
-  "User-provided value"
+select getReportingNode(sink.getNode()), source, sink, "$@ flows to here and is used in a path.",
+  source.getNode(), "User-provided value"

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,10 +21,13 @@ 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 |
 | Test.java:27:11:27:25 | get(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:27:21:27:24 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
 | 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,20 +11,21 @@
 import java.nio.file.Paths;
 import java.nio.file.FileSystems;
 
+import org.apache.commons.io.output.LockableFileWriter;
 
 class Test {
 	void doGet1(InetAddress address)
 		throws IOException {
 			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 @@ void doGet1(InetAddress address)
 				file = new File(temp);
 			}
 	}
-	
+
 	void doGet2(InetAddress address)
 		throws IOException {
 			String temp = address.getHostName();
@@ -44,7 +44,7 @@ void doGet2(InetAddress address)
 			if(isSafe(temp))
 				file = new File(temp);
 	}
-	
+
 	void doGet3(InetAddress address)
 		throws IOException {
 			String temp = address.getHostName();
@@ -66,7 +66,7 @@ boolean isSafe(String pathSpec) {
 			return false;
 		return true;
 	}
-	
+
 	boolean isSortOfSafe(String pathSpec) {
 		// no file separators
 		if (pathSpec.contains(File.separator))
@@ -82,4 +82,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
             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