From ae5fb7f3301befc9f7490d69cf45e8294da0891e Mon Sep 17 00:00:00 2001
From: Tom Hvitved <tom@semmle.com>
Date: Fri, 23 Aug 2019 09:04:50 +0200
Subject: [PATCH 1/3] C#: Introduce `BarrierGuard`s

---
 change-notes/1.23/analysis-csharp.md          |   5 +-
 .../semmle/code/csharp/controlflow/Guards.qll | 182 ++++++++++--------
 .../dataflow/internal/DataFlowPublic.qll      |  49 ++++-
 .../csharp/security/dataflow/TaintedPath.qll  |  55 +++---
 .../csharp/security/dataflow/UrlRedirect.qll  |  21 +-
 .../code/csharp/security/dataflow/ZipSlip.qll |  44 +++--
 .../dataflow/callablereturnsarg/Common.qll    |  10 +-
 .../callablereturnsarg/TaintTracking.ql       |   2 +-
 .../library-tests/dataflow/local/Common.qll   |   6 +
 .../library-tests/dataflow/local/DataFlow.ql  |   3 +-
 .../dataflow/local/TaintTracking.ql           |   3 +-
 11 files changed, 223 insertions(+), 157 deletions(-)

diff --git a/change-notes/1.23/analysis-csharp.md b/change-notes/1.23/analysis-csharp.md
index e477ffd4c83a..c79da283ec7a 100644
--- a/change-notes/1.23/analysis-csharp.md
+++ b/change-notes/1.23/analysis-csharp.md
@@ -16,6 +16,9 @@ The following changes in version 1.23 affect C# analysis in all applications.
 ## Changes to QL libraries
 
 * The new class `NamespaceAccess` models accesses to namespaces, for example in `nameof` expressions.
+* The data-flow library now makes it easier to specify barriers/sanitizers
+  arising from guards by overriding the predicate
+  `isBarrierGuard`/`isSanitizerGuard` on data-flow and taint-tracking
+  configurations respectively.
 
 ## Changes to autobuilder
-
diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
index b5992f0326a0..114338482c80 100644
--- a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
+++ b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
@@ -13,6 +13,23 @@ private import semmle.code.csharp.controlflow.BasicBlocks
 private import semmle.code.csharp.controlflow.internal.Completion
 private import semmle.code.csharp.frameworks.System
 
+/** An expression whose value may control the execution of another element. */
+class Guard extends Expr {
+  Guard() { isGuard(this, _) }
+
+  /**
+   * Holds if `cfn` is guarded by this expression having value `v`, where `sub` is
+   * a sub expression of this expression that is structurally equal to the expression
+   * belonging to `cfn`.
+   *
+   * In case `cfn` or `sub` access an SSA variable in their left-most qualifier, then
+   * so must the other (accessing the same SSA variable).
+   */
+  predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AccessOrCallExpr sub, AbstractValue v) {
+    isGuardedByNode(cfn, this, sub, v)
+  }
+}
+
 /** An abstract value. */
 abstract class AbstractValue extends TAbstractValue {
   /** Holds if the `s` branch out of `cfe` is taken iff `e` has this value. */
@@ -533,7 +550,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode {
 
   private AbstractValue v0;
 
-  GuardedControlFlowNode() { isGuardedByNode(this, g, sub0, v0) }
+  GuardedControlFlowNode() { g.controlsNode(this, sub0, v0) }
 
   /**
    * Gets an expression that guards this control flow node. That is, this control
@@ -561,6 +578,8 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode {
 }
 
 /**
+ * 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
@@ -578,7 +597,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`.
  */
-class GuardedDataFlowNode extends DataFlow::ExprNode {
+deprecated class GuardedDataFlowNode extends DataFlow::ExprNode {
   private Guard g;
 
   private AccessOrCallExpr sub0;
@@ -587,7 +606,7 @@ class GuardedDataFlowNode extends DataFlow::ExprNode {
 
   GuardedDataFlowNode() {
     exists(ControlFlow::Nodes::ElementNode cfn | exists(this.getExprAtNode(cfn)) |
-      isGuardedByNode(cfn, g, sub0, v0)
+      g.controlsNode(cfn, sub0, v0)
     )
   }
 
@@ -621,8 +640,12 @@ class NullGuardedExpr extends GuardedExpr {
   NullGuardedExpr() { this.mustHaveValue(any(NullValue v | not v.isNull())) }
 }
 
-/** A data flow node guarded by a `null` check. */
-class NullGuardedDataFlowNode extends GuardedDataFlowNode {
+/**
+ * DEPRECATED: Use `DataFlow::BarrierGuard` instead.
+ *
+ * A data flow node guarded by a `null` check.
+ */
+deprecated class NullGuardedDataFlowNode extends GuardedDataFlowNode {
   NullGuardedDataFlowNode() { this.mustHaveValue(any(NullValue v | not v.isNull())) }
 }
 
@@ -761,72 +784,49 @@ module Internal {
     e = any(BinaryArithmeticOperation bao | result = bao.getAnOperand())
   }
 
-  /** An expression whose value may control the execution of another element. */
-  class Guard extends Expr {
-    private AbstractValue val;
+  /** Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */
+  private predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) {
+    exists(ControlFlowElement cfe, ConditionalSuccessor s, AbstractValue v0, Guard g0 |
+      cfe.controlsBlock(bb, s)
+    |
+      v0.branch(cfe, s, g0) and
+      impliesSteps(g0, v0, g, v)
+    )
+  }
 
-    Guard() {
-      this.getType() instanceof BoolType and
-      not this instanceof BoolLiteral and
-      not this instanceof SwitchCaseExpr and
-      not this instanceof PatternExpr and
-      val = TBooleanValue(_)
-      or
-      this instanceof DereferenceableExpr and
-      val = TNullValue(_)
-      or
-      val.branch(_, _, this)
+  /**
+   * Holds if control flow node `cfn` only is reached when guard `g` evaluates to `v`,
+   * because of an assertion.
+   */
+  private predicate guardAssertionControlsNode(Guard g, ControlFlow::Node cfn, AbstractValue v) {
+    exists(Assertion a, Guard g0, AbstractValue v0 |
+      asserts(a, g0, v0) and
+      impliesSteps(g0, v0, g, v)
+    |
+      a.strictlyDominates(cfn.getBasicBlock())
       or
-      asserts(_, this, val)
-    }
-
-    /** Gets an abstract value that this guard may have. */
-    AbstractValue getAValue() { result = val }
-
-    /** Holds if basic block `bb` only is reached when this guard has abstract value `v`. */
-    predicate controls(BasicBlock bb, AbstractValue v) {
-      exists(ControlFlowElement cfe, ConditionalSuccessor s, AbstractValue v0, Guard g |
-        cfe.controlsBlock(bb, s)
-      |
-        v0.branch(cfe, s, g) and
-        impliesSteps(g, v0, this, v)
-      )
-    }
-
-    /**
-     * Holds if control flow node `cfn` only is reached when this guard evaluates to `v`,
-     * because of an assertion.
-     */
-    predicate assertionControlsNode(ControlFlow::Node cfn, AbstractValue v) {
-      exists(Assertion a, Guard g, AbstractValue v0 |
-        asserts(a, g, v0) and
-        impliesSteps(g, v0, this, v)
-      |
-        a.strictlyDominates(cfn.getBasicBlock())
-        or
-        exists(BasicBlock bb, int i, int j | bb.getNode(i) = a.getAControlFlowNode() |
-          bb.getNode(j) = cfn and
-          j > i
-        )
+      exists(BasicBlock bb, int i, int j | bb.getNode(i) = a.getAControlFlowNode() |
+        bb.getNode(j) = cfn and
+        j > i
       )
-    }
+    )
+  }
 
-    /**
-     * Holds if control flow element `cfe` only is reached when this guard evaluates to `v`,
-     * because of an assertion.
-     */
-    predicate assertionControlsElement(ControlFlowElement cfe, AbstractValue v) {
-      forex(ControlFlow::Node cfn | cfn = cfe.getAControlFlowNode() |
-        this.assertionControlsNode(cfn, v)
-      )
-    }
+  /**
+   * Holds if control flow element `cfe` only is reached when guard `g` evaluates to `v`,
+   * because of an assertion.
+   */
+  private predicate guardAssertionControlsElement(Guard g, ControlFlowElement cfe, AbstractValue v) {
+    forex(ControlFlow::Node cfn | cfn = cfe.getAControlFlowNode() |
+      guardAssertionControlsNode(g, cfn, v)
+    )
+  }
 
-    /** Same as `this.getAChildExpr*()`, but avoids `fastTC`. */
-    Expr getAChildExprStar() {
-      result = this
-      or
-      result = this.getAChildExprStar().getAChildExpr()
-    }
+  /** Same as `this.getAChildExpr*()`, but avoids `fastTC`. */
+  private Expr getAChildExprStar(Guard g) {
+    result = g
+    or
+    result = getAChildExprStar(g).getAChildExpr()
   }
 
   /**
@@ -1273,8 +1273,24 @@ module Internal {
       private import semmle.code.csharp.Caching
 
       cached
-      predicate isCustomNullCheck(Call call, Expr arg, BooleanValue v, boolean isNull) {
+      predicate isGuard(Expr e, AbstractValue val) {
         Stages::ControlFlowStage::forceCachingInSameStage() and
+        e.getType() instanceof BoolType and
+        not e instanceof BoolLiteral and
+        not e instanceof SwitchCaseExpr and
+        not e instanceof PatternExpr and
+        val = TBooleanValue(_)
+        or
+        e instanceof DereferenceableExpr and
+        val = TNullValue(_)
+        or
+        val.branch(_, _, e)
+        or
+        asserts(_, e, val)
+      }
+
+      cached
+      predicate isCustomNullCheck(Call call, Expr arg, BooleanValue v, boolean isNull) {
         exists(Callable callable, Parameter p |
           arg = call.getArgumentForParameter(any(Parameter p0 | p0.getSourceDeclaration() = p)) and
           call.getTarget().getSourceDeclaration() = callable and
@@ -1355,7 +1371,7 @@ module Internal {
           )
         )
         or
-        v1 = g1.getAValue() and
+        isGuard(g1, v1) and
         v1 = any(MatchValue mv |
             mv.isMatch() and
             g2 = g1 and
@@ -1379,20 +1395,20 @@ module Internal {
         v2 = v1
         or
         g2 = g1.(AssignExpr).getRValue() and
-        v1 = g1.getAValue() and
+        isGuard(g1, v1) and
         v2 = v1
         or
         g2 = g1.(Assignment).getLValue() and
-        v1 = g1.getAValue() and
+        isGuard(g1, v1) and
         v2 = v1
         or
         g2 = g1.(CastExpr).getExpr() and
-        v1 = g1.getAValue() and
+        isGuard(g1, v1) and
         v2 = v1.(NullValue)
         or
         exists(PreSsa::Definition def | def.getDefinition().getSource() = g2 |
           g1 = def.getARead() and
-          v1 = g1.getAValue() and
+          isGuard(g1, v1) and
           v2 = v1
         )
         or
@@ -1473,10 +1489,10 @@ module Internal {
     pragma[noinline]
     private predicate candidateAux(AccessOrCallExpr e, Declaration target, BasicBlock bb) {
       target = e.getTarget() and
-      exists(Guard g | e = g.getAChildExprStar() |
-        g.controls(bb, _)
+      exists(Guard g | e = getAChildExprStar(g) |
+        guardControls(g, bb, _)
         or
-        g.assertionControlsNode(bb.getANode(), _)
+        guardAssertionControlsNode(g, bb.getANode(), _)
       )
     }
   }
@@ -1492,7 +1508,7 @@ module Internal {
     ) {
       Stages::GuardsStage::forceCachingInSameStage() and
       cfn = guarded.getAControlFlowNode() and
-      g.controls(cfn.getBasicBlock(), v) and
+      guardControls(g, cfn.getBasicBlock(), v) and
       exists(ConditionOnExprComparisonConfig c | c.same(sub, guarded))
     }
 
@@ -1504,7 +1520,7 @@ module Internal {
         isGuardedByNode0(cfn, guarded, g, sub, v)
       )
       or
-      g.assertionControlsElement(guarded, v) and
+      guardAssertionControlsElement(g, guarded, v) and
       exists(ConditionOnExprComparisonConfig c | c.same(sub, guarded))
     }
 
@@ -1513,7 +1529,7 @@ module Internal {
       AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub, AbstractValue v
     ) {
       isGuardedByExpr1(guarded, g, sub, v) and
-      sub = g.getAChildExprStar() and
+      sub = getAChildExprStar(g) and
       forall(Ssa::Definition def | def = sub.getAnSsaQualifier(_) |
         def = guarded.getAnSsaQualifier(_)
       )
@@ -1525,7 +1541,7 @@ module Internal {
     ) {
       isGuardedByNode0(guarded, _, g, sub, v)
       or
-      g.assertionControlsNode(guarded, v) and
+      guardAssertionControlsNode(g, guarded, v) and
       exists(ConditionOnExprComparisonConfig c | c.same(sub, guarded.getElement()))
     }
 
@@ -1542,7 +1558,7 @@ module Internal {
       ControlFlow::Nodes::ElementNode guarded, Guard g, AccessOrCallExpr sub, AbstractValue v
     ) {
       isGuardedByNode1(guarded, g, sub, v) and
-      sub = g.getAChildExprStar() and
+      sub = getAChildExprStar(g) and
       forall(Ssa::Definition def | def = sub.getAnSsaQualifier(_) | isGuardedByNode2(guarded, def))
     }
 
@@ -1560,7 +1576,7 @@ module Internal {
       forex(ControlFlow::Node cfn | cfn = g1.getAControlFlowNode() |
         exists(Ssa::ExplicitDefinition def | def.getADefinition().getSource() = g2 |
           g1 = def.getAReadAtNode(cfn) and
-          v1 = g1.getAValue() and
+          isGuard(g1, v1) and
           v2 = v1
         )
       )
@@ -1578,7 +1594,7 @@ module Internal {
   predicate preImpliesSteps(Guard g1, AbstractValue v1, Guard g2, AbstractValue v2) {
     g1 = g2 and
     v1 = v2 and
-    v1 = g1.getAValue()
+    isGuard(g1, v1)
     or
     exists(Expr mid, AbstractValue vMid | preImpliesSteps(g1, v1, mid, vMid) |
       preImpliesStep(mid, vMid, g2, v2)
@@ -1595,7 +1611,7 @@ module Internal {
   predicate impliesSteps(Guard g1, AbstractValue v1, Guard g2, AbstractValue v2) {
     g1 = g2 and
     v1 = v2 and
-    v1 = g1.getAValue()
+    isGuard(g1, v1)
     or
     exists(Expr mid, AbstractValue vMid | impliesSteps(g1, v1, mid, vMid) |
       impliesStep(mid, vMid, g2, v2)
diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
index 0b16b66630fc..b689bf72b559 100644
--- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
+++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
@@ -178,12 +178,51 @@ abstract class NonLocalJumpNode extends Node {
  *
  * It is important that all extending classes in scope are disjoint.
  */
-class BarrierGuard extends Internal::Guard {
-  /** NOT YET SUPPORTED. Holds if this guard validates `e` upon evaluating to `v`. */
-  abstract deprecated predicate checks(Expr e, AbstractValue v);
+class BarrierGuard extends Guard {
+  /** Holds if this guard validates `e` upon evaluating to `v`. */
+  abstract predicate checks(Expr e, AbstractValue v);
 
   /** Gets a node guarded by this guard. */
-  final Node getAGuardedNode() {
-    none() // stub
+  final ExprNode getAGuardedNode() {
+    exists(Expr e, AbstractValue v |
+      this.checks(e, v) and
+      this.controlsNode(result.getControlFlowNode(), e, v)
+    )
+  }
+}
+
+module BarrierGuards {
+  /** A simple guard that checks that this expression has an abstract value. */
+  class ValueBarrierGuard extends BarrierGuard {
+    private AbstractValue v0;
+
+    ValueBarrierGuard() { this.controlsNode(_, this, v0) }
+
+    /**
+     * 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 = v0 }
+
+    final override predicate checks(Expr e, AbstractValue v) { e = this and v = v0 }
+  }
+
+  /** A guard that checks if this expression is non-`null`. */
+  class NullGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
+    NullGuard() { this.getCheckedValue() = any(AbstractValues::NullValue nv | not nv.isNull()) }
+  }
+
+  /** A guard that checks if this expression is `null`. */
+  class AntiNullGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
+    AntiNullGuard() { this.getCheckedValue().(AbstractValues::NullValue).isNull() }
   }
 }
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
index dd718ddef104..9f38add80cae 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
@@ -27,6 +27,11 @@ module TaintedPath {
    */
   abstract class Sanitizer extends DataFlow::ExprNode { }
 
+  /**
+   * A guard for uncontrolled data in path expression vulnerabilities.
+   */
+  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+
   /**
    * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
    */
@@ -38,6 +43,10 @@ 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 BarrierGuard
+    }
   }
 
   /** A source of remote user input. */
@@ -93,44 +102,28 @@ module TaintedPath {
     }
   }
 
-  /**
-   * Holds if this expression is part of a check that is insufficient to prevent path tampering.
-   */
-  private predicate inWeakCheck(Expr e) {
-    // None of these are sufficient to guarantee that a string is safe.
-    exists(MethodCall m, Method def | m.getTarget() = def |
-      m.getQualifier() = e and
-      (
-        def.getName() = "StartsWith" or
-        def.getName() = "EndsWith"
-      )
-      or
-      m.getArgument(0) = e and
-      (
-        def.getName() = "IsNullOrEmpty" or
-        def.getName() = "IsNullOrWhitespace" or
-        def = any(SystemIOFileClass f).getAMethod("Exists") or
-        def = any(SystemIODirectoryClass f).getAMethod("Exists")
-      )
-    )
-    or
-    // Checking against `null` has no bearing on path traversal.
-    exists(EqualityOperation b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
-  }
-
   /**
    * 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 Sanitizer {
+  class PathCheck extends BarrierGuard {
     PathCheck() {
-      // This expression is structurally replicated in a dominating guard which is not a "weak" check
-      exists(Expr e, AbstractValues::BooleanValue v |
-        exists(this.(GuardedDataFlowNode).getAGuard(e, v)) and
-        not inWeakCheck(e)
-      )
+      // 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 DataFlow::BarrierGuards::NullGuard and
+      not this instanceof DataFlow::BarrierGuards::AntiNullGuard
     }
+
+    override predicate checks(Expr e, AbstractValue v) { this.controlsNode(_, e, v) }
   }
 
   /**
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
index bbc7dc78b227..bcc3e40e6069 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
@@ -27,6 +27,11 @@ module UrlRedirect {
    */
   abstract class Sanitizer extends DataFlow::ExprNode { }
 
+  /**
+   * A guard for unvalidated URL redirect vulnerabilities.
+   */
+  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+
   /**
    * A taint-tracking configuration for reasoning about unvalidated URL redirect vulnerabilities.
    */
@@ -38,6 +43,10 @@ module UrlRedirect {
     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 BarrierGuard
+    }
   }
 
   /** A source of remote user input. */
@@ -98,12 +107,12 @@ module UrlRedirect {
   /**
    * A URL argument to a call to `UrlHelper.isLocalUrl()` that is a sanitizer for URL redirects.
    */
-  class IsLocalUrlSanitizer extends Sanitizer {
-    IsLocalUrlSanitizer() {
-      exists(MethodCall mc, AbstractValues::BooleanValue v | mc.getTarget().hasName("IsLocalUrl") |
-        mc = this.(GuardedDataFlowNode).getAGuard(mc.getArgument(0), v) and
-        v.getValue() = true
-      )
+  class IsLocalUrlSanitizer extends BarrierGuard, MethodCall {
+    IsLocalUrlSanitizer() { this.getTarget().hasName("IsLocalUrl") }
+
+    override predicate checks(Expr e, AbstractValue v) {
+      e = this.getArgument(0) and
+      v.(AbstractValues::BooleanValue).getValue() = true
     }
   }
 
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
index d5b9f24d0b1d..8d43f3a710c8 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
@@ -22,6 +22,11 @@ module ZipSlip {
    */
   abstract class Sanitizer extends DataFlow::ExprNode { }
 
+  /**
+   * A guard for unsafe zip extraction.
+   */
+  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+
   /** A taint tracking configuration for Zip Slip */
   class TaintTrackingConfiguration extends TaintTracking::Configuration {
     TaintTrackingConfiguration() { this = "ZipSlipTaintTracking" }
@@ -31,6 +36,10 @@ module ZipSlip {
     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 BarrierGuard
+    }
   }
 
   /** An access to the `FullName` property of a `ZipArchiveEntry`. */
@@ -120,27 +129,26 @@ module ZipSlip {
   }
 
   /**
-   * An expression which is guarded by a call to `String.StartsWith`.
-   *
-   * A call to the method `String.StartsWith` can indicate the the tainted path value is being
+   * A call to `String.StartsWith()` that indicates that the tainted path value is being
    * validated to ensure that it occurs within a permitted output path.
    */
-  class StringCheckSanitizer extends Sanitizer {
-    StringCheckSanitizer() {
-      exists(MethodCall mc, Expr startsWithQualifier, AbstractValues::BooleanValue v |
-        mc = this.(GuardedDataFlowNode).getAGuard(startsWithQualifier, v)
-      |
-        v.getValue() = true and
-        mc.getTarget().hasQualifiedName("System.String", "StartsWith") and
-        mc.getQualifier() = startsWithQualifier and
-        // A StartsWith check against Path.Combine is not sufficient, because the ".." elements have
-        // not yet been resolved.
-        not exists(MethodCall combineCall |
-          combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
-          DataFlow::localFlow(DataFlow::exprNode(combineCall),
-            DataFlow::exprNode(startsWithQualifier))
-        )
+  class StringCheckGuard extends BarrierGuard, MethodCall {
+    private Expr q;
+
+    StringCheckGuard() {
+      this.getTarget().hasQualifiedName("System.String", "StartsWith") and
+      this.getQualifier() = q and
+      // A StartsWith check against Path.Combine is not sufficient, because the ".." elements have
+      // not yet been resolved.
+      not exists(MethodCall combineCall |
+        combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
+        DataFlow::localFlow(DataFlow::exprNode(combineCall), DataFlow::exprNode(q))
       )
     }
+
+    override predicate checks(Expr e, AbstractValue v) {
+      e = q and
+      v.(AbstractValues::BooleanValue).getValue() = true
+    }
   }
 }
diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
index b491756df636..294d7aef99a7 100644
--- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
+++ b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
@@ -8,14 +8,8 @@ class Configuration extends DataFlow::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { any() }
 
-  override predicate isBarrier(DataFlow::Node node) {
-    exists(EQExpr eq, Expr e, AbstractValues::BooleanValue v |
-      eq = node.(GuardedDataFlowNode).getAGuard(e, v)
-    |
-      v.getValue() = true and
-      eq.getAnOperand() = e and
-      eq.getAnOperand() instanceof NullLiteral
-    )
+  override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof DataFlow::BarrierGuards::AntiNullGuard
   }
 }
 
diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql
index f1004b76160f..d83ab7cc9732 100644
--- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql
+++ b/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 isSanitizer(DataFlow::Node node) { c.isBarrier(node) }
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { c.isBarrierGuard(guard) }
 }
 
 from TaintTrackingConfiguration c, Parameter p, int outRefArg
diff --git a/csharp/ql/test/library-tests/dataflow/local/Common.qll b/csharp/ql/test/library-tests/dataflow/local/Common.qll
index 298c02c38fa4..8c4587303d5a 100644
--- a/csharp/ql/test/library-tests/dataflow/local/Common.qll
+++ b/csharp/ql/test/library-tests/dataflow/local/Common.qll
@@ -18,3 +18,9 @@ class MyFlowSource extends DataFlow::Node {
     )
   }
 }
+
+class MyNullGuardedDataFlowNode extends DataFlow::Node {
+  MyNullGuardedDataFlowNode() {
+    this = any(DataFlow::BarrierGuards::NullGuard ng).getAGuardedNode()
+  }
+}
diff --git a/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql b/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql
index 4fec5b6c67a0..a563b7aaadfd 100644
--- a/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql
+++ b/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql
@@ -1,10 +1,9 @@
 import csharp
 import Common
-import semmle.code.csharp.controlflow.Guards
 
 predicate step(DataFlow::Node pred, DataFlow::Node succ) {
   DataFlow::localFlowStep(pred, succ) and
-  not succ instanceof NullGuardedDataFlowNode
+  not succ instanceof MyNullGuardedDataFlowNode
 }
 
 from MyFlowSource source, DataFlow::Node sink, Access target
diff --git a/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql b/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql
index faf328c17ea1..e6fd5298345b 100644
--- a/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql
+++ b/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql
@@ -1,10 +1,9 @@
 import csharp
 import Common
-import semmle.code.csharp.controlflow.Guards
 
 predicate step(DataFlow::Node pred, DataFlow::Node succ) {
   TaintTracking::localTaintStep(pred, succ) and
-  not succ instanceof NullGuardedDataFlowNode
+  not succ instanceof MyNullGuardedDataFlowNode
 }
 
 from MyFlowSource source, DataFlow::Node sink, Access target

From 751985dcf282a377603f7945dd669e9ad897feed Mon Sep 17 00:00:00 2001
From: Tom Hvitved <tom@semmle.com>
Date: Fri, 23 Aug 2019 15:21:28 +0200
Subject: [PATCH 2/3] C#: Address review comments

---
 .../semmle/code/csharp/dataflow/Nullness.qll  |  2 +-
 .../dataflow/internal/DataFlowPublic.qll      | 22 ++++++-------------
 .../csharp/security/dataflow/TaintedPath.qll  | 13 ++++++-----
 .../csharp/security/dataflow/UrlRedirect.qll  |  6 ++---
 .../code/csharp/security/dataflow/ZipSlip.qll |  6 ++---
 .../dataflow/callablereturnsarg/Common.qll    |  8 +++++--
 .../library-tests/dataflow/local/Common.qll   |  9 +++++---
 7 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll b/csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll
index b269e13e5acd..b0f4cfc04326 100644
--- a/csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll
+++ b/csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll
@@ -127,7 +127,7 @@ private predicate dereferenceAt(BasicBlock bb, int i, Ssa::Definition def, Deref
 private predicate exprImpliesSsaDef(
   Expr e, G::AbstractValue vExpr, Ssa::Definition def, G::AbstractValue vDef
 ) {
-  exists(G::Internal::Guard g | G::Internal::impliesSteps(e, vExpr, g, vDef) |
+  exists(G::Guard g | G::Internal::impliesSteps(e, vExpr, g, vDef) |
     g = def.getARead()
     or
     g = def.(Ssa::ExplicitDefinition).getADefinition().getTargetAccess()
diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
index b689bf72b559..37949cafbe1c 100644
--- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
+++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
@@ -192,11 +192,13 @@ class BarrierGuard extends Guard {
 }
 
 module BarrierGuards {
+  import AbstractValues
+
   /** A simple guard that checks that this expression has an abstract value. */
-  class ValueBarrierGuard extends BarrierGuard {
-    private AbstractValue v0;
+  abstract class ValueBarrierGuard extends BarrierGuard {
+    AbstractValue val;
 
-    ValueBarrierGuard() { this.controlsNode(_, this, v0) }
+    ValueBarrierGuard() { this.controlsNode(_, this, val) }
 
     /**
      * Gets the abstract value that this expression is checked against.
@@ -211,18 +213,8 @@ module BarrierGuards {
      * `x == null` is checked against an abstract Boolean value (`BooleanValue`),
      * and `x` is checked against an abstract nullness value (`NullValue`).
      */
-    AbstractValue getCheckedValue() { result = v0 }
-
-    final override predicate checks(Expr e, AbstractValue v) { e = this and v = v0 }
-  }
-
-  /** A guard that checks if this expression is non-`null`. */
-  class NullGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
-    NullGuard() { this.getCheckedValue() = any(AbstractValues::NullValue nv | not nv.isNull()) }
-  }
+    AbstractValue getCheckedValue() { result = val }
 
-  /** A guard that checks if this expression is `null`. */
-  class AntiNullGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
-    AntiNullGuard() { this.getCheckedValue().(AbstractValues::NullValue).isNull() }
+    final override predicate checks(Expr e, AbstractValue v) { e = this and v = val }
   }
 }
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
index 9f38add80cae..23c6e1d840f7 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
@@ -30,7 +30,7 @@ module TaintedPath {
   /**
    * A guard for uncontrolled data in path expression vulnerabilities.
    */
-  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities.
@@ -45,7 +45,7 @@ module TaintedPath {
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
     override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-      guard instanceof BarrierGuard
+      guard instanceof SanitizerGuard
     }
   }
 
@@ -102,12 +102,16 @@ module TaintedPath {
     }
   }
 
+  class NullBarrierGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
+    NullBarrierGuard() { val instanceof DataFlow::BarrierGuards::NullValue }
+  }
+
   /**
    * 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 BarrierGuard {
+  class PathCheck extends SanitizerGuard {
     PathCheck() {
       // None of these are sufficient to guarantee that a string is safe.
       not this.(MethodCall).getTarget() = any(Method m |
@@ -119,8 +123,7 @@ module TaintedPath {
           m = any(SystemIODirectoryClass f).getAMethod("Exists")
         ) and
       // Checking against `null` has no bearing on path traversal.
-      not this instanceof DataFlow::BarrierGuards::NullGuard and
-      not this instanceof DataFlow::BarrierGuards::AntiNullGuard
+      not this instanceof NullBarrierGuard
     }
 
     override predicate checks(Expr e, AbstractValue v) { this.controlsNode(_, e, v) }
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
index bcc3e40e6069..ff501a1c0966 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/UrlRedirect.qll
@@ -30,7 +30,7 @@ module UrlRedirect {
   /**
    * A guard for unvalidated URL redirect vulnerabilities.
    */
-  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A taint-tracking configuration for reasoning about unvalidated URL redirect vulnerabilities.
@@ -45,7 +45,7 @@ module UrlRedirect {
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
     override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-      guard instanceof BarrierGuard
+      guard instanceof SanitizerGuard
     }
   }
 
@@ -107,7 +107,7 @@ module UrlRedirect {
   /**
    * A URL argument to a call to `UrlHelper.isLocalUrl()` that is a sanitizer for URL redirects.
    */
-  class IsLocalUrlSanitizer extends BarrierGuard, MethodCall {
+  class IsLocalUrlSanitizer extends SanitizerGuard, MethodCall {
     IsLocalUrlSanitizer() { this.getTarget().hasName("IsLocalUrl") }
 
     override predicate checks(Expr e, AbstractValue v) {
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
index 8d43f3a710c8..30531b57f9eb 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
+++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll
@@ -25,7 +25,7 @@ module ZipSlip {
   /**
    * A guard for unsafe zip extraction.
    */
-  abstract class BarrierGuard extends DataFlow::BarrierGuard { }
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /** A taint tracking configuration for Zip Slip */
   class TaintTrackingConfiguration extends TaintTracking::Configuration {
@@ -38,7 +38,7 @@ module ZipSlip {
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
     override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-      guard instanceof BarrierGuard
+      guard instanceof SanitizerGuard
     }
   }
 
@@ -132,7 +132,7 @@ module ZipSlip {
    * A call to `String.StartsWith()` that indicates that the tainted path value is being
    * validated to ensure that it occurs within a permitted output path.
    */
-  class StringCheckGuard extends BarrierGuard, MethodCall {
+  class StringCheckGuard extends SanitizerGuard, MethodCall {
     private Expr q;
 
     StringCheckGuard() {
diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
index 294d7aef99a7..2d06be9bf4fa 100644
--- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
+++ b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
@@ -1,5 +1,9 @@
 import csharp
-import semmle.code.csharp.controlflow.Guards
+private import DataFlow::BarrierGuards
+
+private class AntiNullBarrierGuard extends ValueBarrierGuard {
+  AntiNullBarrierGuard() { val.(NullValue).isNull() }
+}
 
 class Configuration extends DataFlow::Configuration {
   Configuration() { this = "Configuration" }
@@ -9,7 +13,7 @@ class Configuration extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) { any() }
 
   override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof DataFlow::BarrierGuards::AntiNullGuard
+    guard instanceof AntiNullBarrierGuard
   }
 }
 
diff --git a/csharp/ql/test/library-tests/dataflow/local/Common.qll b/csharp/ql/test/library-tests/dataflow/local/Common.qll
index 8c4587303d5a..3de1332f680b 100644
--- a/csharp/ql/test/library-tests/dataflow/local/Common.qll
+++ b/csharp/ql/test/library-tests/dataflow/local/Common.qll
@@ -1,5 +1,10 @@
 import csharp
 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() {
@@ -20,7 +25,5 @@ class MyFlowSource extends DataFlow::Node {
 }
 
 class MyNullGuardedDataFlowNode extends DataFlow::Node {
-  MyNullGuardedDataFlowNode() {
-    this = any(DataFlow::BarrierGuards::NullGuard ng).getAGuardedNode()
-  }
+  MyNullGuardedDataFlowNode() { this = any(NullBarrierGuard ng).getAGuardedNode() }
 }

From 6e7ef6664281d87037291678af60f001c75b7f06 Mon Sep 17 00:00:00 2001
From: Tom Hvitved <tom@semmle.com>
Date: Tue, 27 Aug 2019 15:38:29 +0200
Subject: [PATCH 3/3] C#: Revert to using `GuardedDataFlowNode` in
 `TaintedPath.qll`

---
 .../semmle/code/csharp/controlflow/Guards.qll | 24 ++++-----
 .../dataflow/internal/DataFlowPublic.qll      | 28 -----------
 .../csharp/security/dataflow/TaintedPath.qll  | 49 +++++++++----------
 .../dataflow/callablereturnsarg/Common.qll    | 12 ++---
 .../callablereturnsarg/TaintTracking.ql       |  2 +-
 .../library-tests/dataflow/local/Common.qll   | 10 +---
 .../library-tests/dataflow/local/DataFlow.ql  |  2 +-
 .../dataflow/local/TaintTracking.ql           |  2 +-
 8 files changed, 42 insertions(+), 87 deletions(-)

diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
index 114338482c80..4b6afbeaf189 100644
--- a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
+++ b/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,7 +630,7 @@ 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. */
@@ -640,12 +638,8 @@ 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())) }
 }
 
diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
index 37949cafbe1c..d5ba0101f21b 100644
--- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
+++ b/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 }
-  }
-}
diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
index 23c6e1d840f7..78a0ca2c3bf6 100644
--- a/csharp/ql/src/semmle/code/csharp/security/dataflow/TaintedPath.qll
+++ b/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,8 +93,24 @@ 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))
+    }
   }
 
   /**
@@ -111,22 +118,14 @@ module TaintedPath {
    *
    * 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) }
   }
 
   /**
diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
index 2d06be9bf4fa..4b2626c73b3c 100644
--- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll
+++ b/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()
+    )
   }
 }
 
diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql
index d83ab7cc9732..f1004b76160f 100644
--- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/TaintTracking.ql
+++ b/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
diff --git a/csharp/ql/test/library-tests/dataflow/local/Common.qll b/csharp/ql/test/library-tests/dataflow/local/Common.qll
index 3de1332f680b..5c65005407db 100644
--- a/csharp/ql/test/library-tests/dataflow/local/Common.qll
+++ b/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() }
-}
diff --git a/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql b/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql
index a563b7aaadfd..fbe1a746d16d 100644
--- a/csharp/ql/test/library-tests/dataflow/local/DataFlow.ql
+++ b/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
diff --git a/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql b/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql
index e6fd5298345b..2ded6977c20b 100644
--- a/csharp/ql/test/library-tests/dataflow/local/TaintTracking.ql
+++ b/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