Merge pull request #312 from microsoft/fix-merge-conflict-inTaintedPathQuery-2

C#: Fix an incorrect merge conflict resolution.

Author: dilanbhalla

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

@@ -52,40 +52,6 @@ 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.
  */
@@ -108,12 +74,12 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig {
     s2 = Normalized()
   }
 
+  predicate isBarrier(DataFlow::Node node, FlowState state) { node.(Sanitizer).isBarrier(state) }
+
   predicate isBarrierOut(DataFlow::Node node, FlowState state) {
     isAdditionalFlowStep(_, state, node, _)
   }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
-
   predicate observeDiffInformedIncrementalMode() { any() }
 }
 

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

@@ -68,7 +68,12 @@ public void ProcessRequest(HttpContext ctx)
         string absolutePath = ctx.Request.MapPath("~MyTempFile");
         string fullPath2 = Path.Combine(absolutePath, path);
         if (fullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) {
-            File.ReadAllText(fullPath2); // GOOD
+            File.ReadAllText(fullPath2); // BAD
+        }
+
+        string normalizedFullPath2 = Path.GetFullPath(fullPath2);
+        if (normalizedFullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) {
+            File.ReadAllText(normalizedFullPath2); // GOOD
         }
     }
 

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

@@ -7,6 +7,7 @@
 | 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 |
+| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | 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 |  |
@@ -21,8 +22,13 @@ edges
 | 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 |  |
+| TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:69:55:69:58 | access to local variable path : String | provenance |  |
+| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | provenance |  |
+| TaintedPath.cs:69:28:69:59 | call to method Combine : String | TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | provenance |  |
+| TaintedPath.cs:69:55:69:58 | access to local variable path : String | TaintedPath.cs:69:28:69:59 | call to method Combine : String | provenance | MaD:2 |
 models
 | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
+| 2 | Summary: System.IO; Path; false; Combine; (System.String,System.String); ; Argument[1]; ReturnValue; taint; manual |
 nodes
 | TaintedPath.cs:10:16:10:19 | access to local variable path : String | semmle.label | access to local variable path : String |
 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
@@ -37,4 +43,8 @@ nodes
 | 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 |
+| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | semmle.label | access to local variable fullPath2 : String |
+| TaintedPath.cs:69:28:69:59 | call to method Combine : String | semmle.label | call to method Combine : String |
+| TaintedPath.cs:69:55:69:58 | access to local variable path : String | semmle.label | access to local variable path : String |
+| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | semmle.label | access to local variable fullPath2 |
 subpaths