C#: Revert to using GuardedDataFlowNode in TaintedPath.qll

Author: hvitved

csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll

@@ -498,7 +498,7 @@ class GuardedExpr extends AccessOrCallExpr {
    * left-most qualifier, then so must the other (accessing the same SSA
    * variable).
    */
-  Expr getAGuard(Expr sub, AbstractValue v) {
+  Guard getAGuard(Expr sub, AbstractValue v) {
     result = g and
     sub = sub0 and
     v = v0
@@ -509,7 +509,7 @@ class GuardedExpr extends AccessOrCallExpr {
    * expression is guarded by a structurally equal expression having abstract
    * value `v`.
    */
-  predicate mustHaveValue(AbstractValue v) { exists(Expr e | e = this.getAGuard(e, v)) }
+  predicate mustHaveValue(AbstractValue v) { g = this.getAGuard(g, v) }
 
   /**
    * Holds if this expression is guarded by expression `cond`, which must
@@ -563,7 +563,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode {
    * left-most qualifier, then so must the other (accessing the same SSA
    * variable).
    */
-  Expr getAGuard(Expr sub, AbstractValue v) {
+  Guard getAGuard(Expr sub, AbstractValue v) {
     result = g and
     sub = sub0 and
     v = v0
@@ -574,12 +574,10 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode {
    * control flow node is guarded by a structurally equal expression having
    * abstract value `v`.
    */
-  predicate mustHaveValue(AbstractValue v) { exists(Expr e | e = this.getAGuard(e, v)) }
+  predicate mustHaveValue(AbstractValue v) { g = this.getAGuard(g, v) }
 }
 
 /**
- * DEPRECATED: Use `DataFlow::BarrierGuard` instead.
- *
  * A guarded data flow node. A guarded data flow node is like a guarded expression
  * (`GuardedExpr`), except control flow graph splitting is taken into account. That
  * is, one data flow node belonging to an expression may be guarded, while another
@@ -597,7 +595,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode {
  * In the example above, the node for `x.ToString()` is null-guarded in the
  * split `b == true`, but not in the split `b == false`.
  */
-deprecated class GuardedDataFlowNode extends DataFlow::ExprNode {
+class GuardedDataFlowNode extends DataFlow::ExprNode {
   private Guard g;
 
   private AccessOrCallExpr sub0;
@@ -621,7 +619,7 @@ deprecated class GuardedDataFlowNode extends DataFlow::ExprNode {
    * left-most qualifier, then so must the other (accessing the same SSA
    * variable).
    */
-  Expr getAGuard(Expr sub, AbstractValue v) {
+  Guard getAGuard(Expr sub, AbstractValue v) {
     result = g and
     sub = sub0 and
     v = v0
@@ -632,20 +630,16 @@ deprecated class GuardedDataFlowNode extends DataFlow::ExprNode {
    * data flow node is guarded by a structurally equal expression having
    * abstract value `v`.
    */
-  predicate mustHaveValue(AbstractValue v) { exists(Expr e | e = this.getAGuard(e, v)) }
+  predicate mustHaveValue(AbstractValue v) { g = this.getAGuard(g, v) }
 }
 
 /** An expression guarded by a `null` check. */
 class NullGuardedExpr extends GuardedExpr {
   NullGuardedExpr() { this.mustHaveValue(any(NullValue v | not v.isNull())) }
 }
 
-/**
- * DEPRECATED: Use `DataFlow::BarrierGuard` instead.
- *
- * A data flow node guarded by a `null` check.
- */
-deprecated class NullGuardedDataFlowNode extends GuardedDataFlowNode {
+/** A data flow node guarded by a `null` check. */
+class NullGuardedDataFlowNode extends GuardedDataFlowNode {
   NullGuardedDataFlowNode() { this.mustHaveValue(any(NullValue v | not v.isNull())) }
 }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll

@@ -190,31 +190,3 @@ class BarrierGuard extends Guard {
     )
   }
 }
-
-module BarrierGuards {
-  import AbstractValues
-
-  /** A simple guard that checks that this expression has an abstract value. */
-  abstract class ValueBarrierGuard extends BarrierGuard {
-    AbstractValue val;
-
-    ValueBarrierGuard() { this.controlsNode(_, this, val) }
-
-    /**
-     * Gets the abstract value that this expression is checked against.
-     *
-     * For example, in
-     *
-     * ```
-     * if (x == null)
-     *    ...
-     * ```
-     *
-     * `x == null` is checked against an abstract Boolean value (`BooleanValue`),
-     * and `x` is checked against an abstract nullness value (`NullValue`).
-     */
-    AbstractValue getCheckedValue() { result = val }
-
-    final override predicate checks(Expr e, AbstractValue v) { e = this and v = val }
-  }
-}

csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll

@@ -27,11 +27,6 @@ module TaintedPath {
    */
   abstract class Sanitizer extends DataFlow::ExprNode { }
 
-  /**
-   * A guard for uncontrolled data in path expression vulnerabilities.
-   */
-  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
-
   /**
    * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
    */
@@ -43,10 +38,6 @@ module TaintedPath {
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
-
-    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-      guard instanceof SanitizerGuard
-    }
   }
 
   /** A source of remote user input. */
@@ -102,31 +93,39 @@ module TaintedPath {
     }
   }
 
-  class NullBarrierGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
-    NullBarrierGuard() { val instanceof DataFlow::BarrierGuards::NullValue }
+  /**
+   * A weak guard that is insufficient to prevent path tampering.
+   */
+  private class WeakGuard extends Guard {
+    WeakGuard() {
+      // None of these are sufficient to guarantee that a string is safe.
+      exists(MethodCall mc, Method m | this = mc and mc.getTarget() = m |
+        m.getName() = "StartsWith" or
+        m.getName() = "EndsWith" or
+        m.getName() = "IsNullOrEmpty" or
+        m.getName() = "IsNullOrWhitespace" or
+        m = any(SystemIOFileClass f).getAMethod("Exists") or
+        m = any(SystemIODirectoryClass f).getAMethod("Exists")
+      )
+      or
+      // Checking against `null` has no bearing on path traversal.
+      this.controlsNode(_, _, any(AbstractValues::NullValue nv))
+    }
   }
 
   /**
    * A conditional involving the path, that is not considered to be a weak check.
    *
    * A weak check is one that is insufficient to prevent path tampering.
    */
-  class PathCheck extends SanitizerGuard {
+  class PathCheck extends Sanitizer {
     PathCheck() {
-      // None of these are sufficient to guarantee that a string is safe.
-      not this.(MethodCall).getTarget() = any(Method m |
-          m.getName() = "StartsWith" or
-          m.getName() = "EndsWith" or
-          m.getName() = "IsNullOrEmpty" or
-          m.getName() = "IsNullOrWhitespace" or
-          m = any(SystemIOFileClass f).getAMethod("Exists") or
-          m = any(SystemIODirectoryClass f).getAMethod("Exists")
-        ) and
-      // Checking against `null` has no bearing on path traversal.
-      not this instanceof NullBarrierGuard
+      // 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
+      )
     }
-
-    override predicate checks(Expr e, AbstractValue v) { this.controlsNode(_, e, v) }
   }
 
   /**

csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll

@@ -1,9 +1,5 @@
 import csharp
-private import DataFlow::BarrierGuards
-
-private class AntiNullBarrierGuard extends ValueBarrierGuard {
-  AntiNullBarrierGuard() { val.(NullValue).isNull() }
-}
+private import semmle.code.csharp.controlflow.Guards
 
 class Configuration extends DataFlow::Configuration {
   Configuration() { this = "Configuration" }
@@ -12,8 +8,10 @@ class Configuration extends DataFlow::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { any() }
 
-  override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof AntiNullBarrierGuard
+  override predicate isBarrier(DataFlow::Node node) {
+    exists(AbstractValues::NullValue nv | node.(GuardedDataFlowNode).mustHaveValue(nv) |
+      nv.isNull()
+    )
   }
 }
 

csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql

@@ -10,7 +10,7 @@ class TaintTrackingConfiguration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { c.isSink(sink) }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { c.isBarrierGuard(guard) }
+  override predicate isSanitizer(DataFlow::Node node) { c.isBarrier(node) }
 }
 
 from TaintTrackingConfiguration c, Parameter p, int outRefArg

csharp/ql/test/library-tests/dataflow/local/Common.qll

@@ -1,10 +1,6 @@
 import csharp
+import semmle.code.csharp.controlflow.Guards
 private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
-private import DataFlow::BarrierGuards
-
-private class NullBarrierGuard extends ValueBarrierGuard {
-  NullBarrierGuard() { val = any(NullValue nv | not nv.isNull()) }
-}
 
 class MyFlowSource extends DataFlow::Node {
   MyFlowSource() {
@@ -23,7 +19,3 @@ class MyFlowSource extends DataFlow::Node {
     )
   }
 }
-
-class MyNullGuardedDataFlowNode extends DataFlow::Node {
-  MyNullGuardedDataFlowNode() { this = any(NullBarrierGuard ng).getAGuardedNode() }
-}

csharp/ql/test/library-tests/dataflow/local/DataFlow.ql

@@ -3,7 +3,7 @@ import Common
 
 predicate step(DataFlow::Node pred, DataFlow::Node succ) {
   DataFlow::localFlowStep(pred, succ) and
-  not succ instanceof MyNullGuardedDataFlowNode
+  not succ instanceof NullGuardedDataFlowNode
 }
 
 from MyFlowSource source, DataFlow::Node sink, Access target

csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql

@@ -3,7 +3,7 @@ import Common
 
 predicate step(DataFlow::Node pred, DataFlow::Node succ) {
   TaintTracking::localTaintStep(pred, succ) and
-  not succ instanceof MyNullGuardedDataFlowNode
+  not succ instanceof NullGuardedDataFlowNode
 }
 
 from MyFlowSource source, DataFlow::Node sink, Access target