Merge pull request #237 from microsoft/fix-fps-in-tainted-path

C#: Fix FPs (and a small FN) in `cs/path-injection`

Author: MathiasVP

csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll

@@ -52,6 +52,40 @@ private class GetFullPathStep extends PathNormalizationStep {
   }
 }
 
+/** Holds if `e` may evaluate to an absolute path. */
+bindingset[e]
+pragma[inline_late]
+private predicate isAbsolute(Expr e) {
+  exists(Expr absolute | DataFlow::localExprFlow(absolute, e) |
+    exists(Call call | absolute = call |
+      call.getARuntimeTarget()
+          .hasFullyQualifiedName(["System.Web.HttpServerUtilityBase", "System.Web.HttpRequest"],
+            "MapPath")
+      or
+      call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Path", "GetFullPath")
+      or
+      call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Directory", "GetCurrentDirectory")
+    )
+    or
+    exists(PropertyRead read | absolute = read |
+      read.getTarget().hasFullyQualifiedName("System", "Environment", "CurrentDirectory")
+    )
+  )
+}
+
+private class PathCombineStep extends PathNormalizationStep {
+  override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(Call call |
+      // The result of `Path.Combine(x, y)` is an absolute path when `x` is an
+      // absolute path.
+      call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Path", "Combine") and
+      isAbsolute(call.getArgument(0)) and
+      n1.asExpr() = call.getArgument(1) and
+      n2.asExpr() = call
+    )
+  }
+}
+
 /**
  * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
  */
@@ -64,10 +98,9 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig {
     source instanceof Source and state = NotNormalized()
   }
 
-  predicate isSink(DataFlow::Node sink, FlowState state) {
-    sink instanceof Sink and
-    exists(state)
-  }
+  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) { none() }
 
   predicate isAdditionalFlowStep(DataFlow::Node n1, FlowState s1, DataFlow::Node n2, FlowState s2) {
     any(PathNormalizationStep step).isAdditionalFlowStep(n1, n2) and
@@ -78,7 +111,7 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig {
   predicate isBarrier(DataFlow::Node node, FlowState state) { node.(Sanitizer).isBarrier(state) }
 
   predicate isBarrierOut(DataFlow::Node node, FlowState state) {
-    isAdditionalFlowStep(node, state, _, _)
+    isAdditionalFlowStep(_, state, node, _)
   }
 }
 

csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs

@@ -61,6 +61,15 @@ public void ProcessRequest(HttpContext ctx)
         {
             File.ReadAllText(fullPath); // GOOD
         }
+
+        // This test ensures that we can flow through `Path.GetFullPath` and still get a result.
+        ctx.Response.Write(File.ReadAllText(path)); // BAD
+
+        string absolutePath = ctx.Request.MapPath("~MyTempFile");
+        string fullPath2 = Path.Combine(absolutePath, path);
+        if (fullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) {
+            File.ReadAllText(fullPath2); // GOOD
+        }
     }
 
     public bool IsReusable

csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected

@@ -6,18 +6,21 @@
 | TaintedPath.cs:36:25:36:31 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:36:25:36:31 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value |
 | TaintedPath.cs:38:49:38:55 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:38:49:38:55 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value |
 | TaintedPath.cs:51:26:51:29 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:51:26:51:29 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value |
+| TaintedPath.cs:66:45:66:48 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:66:45:66:48 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value |
 edges
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:12:50:12:53 | access to local variable path | provenance |  |
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:17:51:17:54 | access to local variable path | provenance |  |
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:25:30:25:33 | access to local variable path | provenance |  |
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:31:30:31:33 | access to local variable path | provenance |  |
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | provenance |  |
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:51:26:51:29 | access to local variable path | provenance |  |
+| TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:59:44:59:47 | access to local variable path : String | provenance |  |
 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:10:16:10:19 | access to local variable path : String | provenance |  |
 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:10:23:10:53 | access to indexer : String | provenance | MaD:1 |
 | TaintedPath.cs:10:23:10:53 | access to indexer : String | TaintedPath.cs:10:16:10:19 | access to local variable path : String | provenance |  |
 | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:36:25:36:31 | access to local variable badPath | provenance |  |
 | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:38:49:38:55 | access to local variable badPath | provenance |  |
+| TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:66:45:66:48 | access to local variable path | provenance |  |
 models
 | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
 nodes
@@ -32,4 +35,6 @@ nodes
 | TaintedPath.cs:36:25:36:31 | access to local variable badPath | semmle.label | access to local variable badPath |
 | TaintedPath.cs:38:49:38:55 | access to local variable badPath | semmle.label | access to local variable badPath |
 | TaintedPath.cs:51:26:51:29 | access to local variable path | semmle.label | access to local variable path |
+| TaintedPath.cs:59:44:59:47 | access to local variable path : String | semmle.label | access to local variable path : String |
+| TaintedPath.cs:66:45:66:48 | access to local variable path | semmle.label | access to local variable path |
 subpaths