Merge pull request #106 from microsoft/tainted-path-barrier-with-state

C#: Make `StartsWith` and `EndsWith` sanitizers on normalized paths

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

@@ -4,6 +4,7 @@
  */
 
 import csharp
+private import codeql.util.Unit
 private import semmle.code.csharp.controlflow.Guards
 private import semmle.code.csharp.security.dataflow.flowsinks.FlowSinks
 private import semmle.code.csharp.security.dataflow.flowsources.FlowSources
@@ -24,23 +25,67 @@ abstract class Sink extends ApiSinkExprNode { }
 /**
  * A sanitizer for uncontrolled data in path expression vulnerabilities.
  */
-abstract class Sanitizer extends DataFlow::ExprNode { }
+abstract class Sanitizer extends DataFlow::ExprNode {
+  /** Holds if this is a sanitizer when the flow state is `state`. */
+  predicate isBarrier(TaintedPathConfig::FlowState state) { any() }
+}
+
+/** A path normalization step. */
+private class PathNormalizationStep extends Unit {
+  /**
+   * Holds if the flow step from `n1` to `n2` transforms the path into an
+   * absolute path.
+   *
+   * For example, the argument-to-return-value step through a call
+   * to `System.IO.Path.GetFullPath` is a normalization step.
+   */
+  abstract predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2);
+}
+
+private class GetFullPathStep extends PathNormalizationStep {
+  override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(Call call |
+      call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Path", "GetFullPath") and
+      n1.asExpr() = call.getArgument(0) and
+      n2.asExpr() = call
+    )
+  }
+}
 
 /**
  * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
  */
-private module TaintedPathConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source instanceof Source }
+private module TaintedPathConfig implements DataFlow::StateConfigSig {
+  newtype FlowState =
+    additional NotNormalized() or
+    additional Normalized()
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    source instanceof Source and state = NotNormalized()
+  }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    sink instanceof Sink and
+    exists(state)
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node n1, FlowState s1, DataFlow::Node n2, FlowState s2) {
+    any(PathNormalizationStep step).isAdditionalFlowStep(n1, n2) and
+    s1 = NotNormalized() and
+    s2 = Normalized()
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState state) { node.(Sanitizer).isBarrier(state) }
+
+  predicate isBarrierOut(DataFlow::Node node, FlowState state) {
+    isAdditionalFlowStep(node, state, _, _)
+  }
 }
 
 /**
  * A taint-tracking module for uncontrolled data in path expression vulnerabilities.
  */
-module TaintedPath = TaintTracking::Global<TaintedPathConfig>;
+module TaintedPath = TaintTracking::GlobalWithState<TaintedPathConfig>;
 
 /**
  * DEPRECATED: Use `ThreatModelSource` instead.
@@ -99,7 +144,7 @@ class StreamWriterTaintedPathSink extends Sink {
 }
 
 /**
- * A weak guard that is insufficient to prevent path tampering.
+ * A weak guard that may be insufficient to prevent path tampering.
  */
 private class WeakGuard extends Guard {
   WeakGuard() {
@@ -118,6 +163,14 @@ private class WeakGuard extends Guard {
     or
     this.(LogicalOperation).getAnOperand() instanceof WeakGuard
   }
+
+  predicate isBarrier(TaintedPathConfig::FlowState state) {
+    state = TaintedPathConfig::Normalized() and
+    exists(Method m | this.(MethodCall).getTarget() = m |
+      m.getName() = "StartsWith" or
+      m.getName() = "EndsWith"
+    )
+  }
 }
 
 /**
@@ -126,12 +179,17 @@ private class WeakGuard extends Guard {
  * A weak check is one that is insufficient to prevent path tampering.
  */
 class PathCheck extends Sanitizer {
+  Guard g;
+
   PathCheck() {
-    // This expression is structurally replicated in a dominating guard which is not a "weak" check
-    exists(Guard g, AbstractValues::BooleanValue v |
-      g = this.(GuardedDataFlowNode).getAGuard(_, v) and
-      not g instanceof WeakGuard
-    )
+    // This expression is structurally replicated in a dominating guard
+    exists(AbstractValues::BooleanValue v | g = this.(GuardedDataFlowNode).getAGuard(_, v))
+  }
+
+  override predicate isBarrier(TaintedPathConfig::FlowState state) {
+    g.(WeakGuard).isBarrier(state)
+    or
+    not g instanceof WeakGuard
   }
 }
 

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

@@ -55,6 +55,12 @@ public class TaintedPathHandler : IHttpHandler
 
         // GOOD: A simple type.
         File.ReadAllText(int.Parse(path).ToString());
+
+        string fullPath = Path.GetFullPath(path);
+        if (fullPath.StartsWith("C:\\Foo"))
+        {
+            File.ReadAllText(fullPath); // GOOD
+        }
     }
 
     public bool IsReusable