Merge pull request #21001 from aschackmull/guards/generalise-validationwrapper

Guards: Generalise ValidationWrapper to support GuardValue-based BarrierGuards

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll

@@ -1051,12 +1051,12 @@ module BarrierGuardWithIntParam<guardChecksNodeSig/4 guardChecksNode> {
   }
 
   private predicate guardChecksInstr(
-    IRGuards::Guards_v1::Guard g, IRGuards::GuardsInput::Expr instr, boolean branch,
+    IRGuards::Guards_v1::Guard g, IRGuards::GuardsInput::Expr instr, IRGuards::GuardValue gv,
     int indirectionIndex
   ) {
     exists(Node node |
       nodeHasInstruction(node, instr, indirectionIndex) and
-      guardChecksNode(g, node, branch, indirectionIndex)
+      guardChecksNode(g, node, gv.asBooleanValue(), indirectionIndex)
     )
   }
 
@@ -1064,8 +1064,8 @@ module BarrierGuardWithIntParam<guardChecksNodeSig/4 guardChecksNode> {
     DataFlowIntegrationInput::Guard g, SsaImpl::Definition def, IRGuards::GuardValue val,
     int indirectionIndex
   ) {
-    IRGuards::Guards_v1::ValidationWrapperWithState<int, guardChecksInstr/4>::guardChecksDef(g, def,
-      val, indirectionIndex)
+    IRGuards::Guards_v1::ParameterizedValidationWrapper<int, guardChecksInstr/4>::guardChecksDef(g,
+      def, val, indirectionIndex)
   }
 
   Node getABarrierNode(int indirectionIndex) {

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -374,6 +374,29 @@ class ContentSet instanceof Content {
   }
 }
 
+/**
+ * Holds if the guard `g` validates the expression `e` upon evaluating to `gv`.
+ *
+ * The expression `e` is expected to be a syntactic part of the guard `g`.
+ * For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
+ * the argument `x`.
+ */
+signature predicate valueGuardChecksSig(Guard g, Expr e, GuardValue gv);
+
+/**
+ * Provides a set of barrier nodes for a guard that validates an expression.
+ *
+ * This is expected to be used in `isBarrier`/`isSanitizer` definitions
+ * in data flow and taint tracking.
+ */
+module BarrierGuardValue<valueGuardChecksSig/3 guardChecks> {
+  /** Gets a node that is safely guarded by the given guard check. */
+  Node getABarrierNode() {
+    SsaFlow::asNode(result) =
+      SsaImpl::DataFlowIntegration::BarrierGuard<guardChecks/3>::getABarrierNode()
+  }
+}
+
 /**
  * Holds if the guard `g` validates the expression `e` upon evaluating to `branch`.
  *
@@ -390,9 +413,10 @@ signature predicate guardChecksSig(Guard g, Expr e, boolean branch);
  * in data flow and taint tracking.
  */
 module BarrierGuard<guardChecksSig/3 guardChecks> {
+  private predicate guardChecks0(Guard g, Expr e, GuardValue gv) {
+    guardChecks(g, e, gv.asBooleanValue())
+  }
+
   /** Gets a node that is safely guarded by the given guard check. */
-  Node getABarrierNode() {
-    SsaFlow::asNode(result) =
-      SsaImpl::DataFlowIntegration::BarrierGuard<guardChecks/3>::getABarrierNode()
-  }
+  Node getABarrierNode() { result = BarrierGuardValue<guardChecks0/3>::getABarrierNode() }
 }

java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll

@@ -564,12 +564,14 @@ private module Cached {
       DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
     }
 
-    signature predicate guardChecksSig(Guards::Guard g, Expr e, boolean branch);
+    signature predicate guardChecksSig(Guards::Guard g, Expr e, Guards::GuardValue gv);
 
     cached // nothing is actually cached
     module BarrierGuard<guardChecksSig/3 guardChecks> {
-      private predicate guardChecksAdjTypes(Guards::Guards_v3::Guard g, Expr e, boolean branch) {
-        guardChecks(g, e, branch)
+      private predicate guardChecksAdjTypes(
+        Guards::Guards_v3::Guard g, Expr e, Guards::GuardValue gv
+      ) {
+        guardChecks(g, e, gv)
       }
 
       private predicate guardChecksWithWrappers(

java/ql/test/library-tests/dataflow/ssa/A.java

@@ -4,7 +4,13 @@ public class A {
 
   boolean isSafe(Object o) { return o == null; }
 
-  void foo() {
+  void assertSafe(Object o) { if (o != null) throw new RuntimeException(); }
+
+  private boolean wrapIsSafe(Object o) { return isSafe(o); }
+
+  private void wrapAssertSafe(Object o) { assertSafe(o); }
+
+  void test1() {
     Object x = source();
     if (!isSafe(x)) {
       x = null;
@@ -21,4 +27,23 @@ public class A {
     }
     sink(x);
   }
+
+  void test2() {
+    Object x = source();
+    assertSafe(x);
+    sink(x);
+  }
+
+  void test3() {
+    Object x = source();
+    if (wrapIsSafe(x)) {
+      sink(x);
+    }
+  }
+
+  void test4() {
+    Object x = source();
+    wrapAssertSafe(x);
+    sink(x);
+  }
 }

java/ql/test/library-tests/dataflow/ssa/test.ql

@@ -10,6 +10,14 @@ private predicate isSafe(Guard g, Expr checked, boolean branch) {
   )
 }
 
+private predicate assertSafe(Guard g, Expr checked, GuardValue gv) {
+  exists(MethodCall mc | g = mc |
+    mc.getMethod().hasName("assertSafe") and
+    checked = mc.getAnArgument() and
+    gv.getDualValue().isThrowsException()
+  )
+}
+
 module TestConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source.asExpr().(MethodCall).getMethod().hasName("source")
@@ -21,6 +29,8 @@ module TestConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) {
     node = DataFlow::BarrierGuard<isSafe/3>::getABarrierNode()
+    or
+    node = DataFlow::BarrierGuardValue<assertSafe/3>::getABarrierNode()
   }
 }
 

shared/controlflow/codeql/controlflow/Guards.qll

@@ -1280,39 +1280,38 @@ module Make<
       }
     }
 
-    signature predicate guardChecksSig(Guard g, Expr e, boolean branch);
+    signature predicate guardChecksSig(Guard g, Expr e, GuardValue gv);
 
     bindingset[this]
-    signature class StateSig;
+    signature class ParamSig;
 
-    private module WithState<StateSig State> {
-      signature predicate guardChecksSig(Guard g, Expr e, boolean branch, State state);
+    private module WithParam<ParamSig P> {
+      signature predicate guardChecksSig(Guard g, Expr e, GuardValue gv, P par);
     }
 
     /**
      * Extends a `BarrierGuard` input predicate with wrapped invocations.
      */
     module ValidationWrapper<guardChecksSig/3 guardChecks0> {
-      private predicate guardChecksWithState(Guard g, Expr e, boolean branch, Unit state) {
-        guardChecks0(g, e, branch) and exists(state)
+      private predicate guardChecksWithParam(Guard g, Expr e, GuardValue gv, Unit par) {
+        guardChecks0(g, e, gv) and exists(par)
       }
 
-      private module StatefulWrapper = ValidationWrapperWithState<Unit, guardChecksWithState/4>;
+      private module ParameterizedWrapper =
+        ParameterizedValidationWrapper<Unit, guardChecksWithParam/4>;
 
       /**
        * Holds if the guard `g` validates the SSA definition `def` upon evaluating to `val`.
        */
       predicate guardChecksDef(Guard g, SsaDefinition def, GuardValue val) {
-        StatefulWrapper::guardChecksDef(g, def, val, _)
+        ParameterizedWrapper::guardChecksDef(g, def, val, _)
       }
     }
 
     /**
      * Extends a `BarrierGuard` input predicate with wrapped invocations.
      */
-    module ValidationWrapperWithState<
-      StateSig State, WithState<State>::guardChecksSig/4 guardChecks0>
-    {
+    module ParameterizedValidationWrapper<ParamSig P, WithParam<P>::guardChecksSig/4 guardChecks0> {
       private import WrapperGuard
 
       /**
@@ -1321,12 +1320,12 @@ module Make<
        * parameter has been validated by the given guard.
        */
       private predicate validReturnInValidationWrapper(
-        ReturnExpr ret, ParameterPosition ppos, GuardValue retval, State state
+        ReturnExpr ret, ParameterPosition ppos, GuardValue retval, P par
       ) {
         exists(NonOverridableMethod m, SsaParameterInit param, Guard guard, GuardValue val |
           m.getAReturnExpr() = ret and
           param.getParameter() = m.getParameter(ppos) and
-          guardChecksDef(guard, param, val, state)
+          guardChecksDef(guard, param, val, par)
         |
           guard.valueControls(ret.getBasicBlock(), val) and
           relevantReturnExprValue(m, ret, retval)
@@ -1341,7 +1340,7 @@ module Make<
        * that the argument has been validated by the given guard.
        */
       private NonOverridableMethod validationWrapper(
-        ParameterPosition ppos, GuardValue retval, State state
+        ParameterPosition ppos, GuardValue retval, P par
       ) {
         forex(ReturnExpr ret |
           result.getAReturnExpr() = ret and
@@ -1350,12 +1349,12 @@ module Make<
             disjointValues(notRetval, retval)
           )
         |
-          validReturnInValidationWrapper(ret, ppos, retval, state)
+          validReturnInValidationWrapper(ret, ppos, retval, par)
         )
         or
         exists(SsaParameterInit param, BasicBlock bb, Guard guard, GuardValue val |
           param.getParameter() = result.getParameter(ppos) and
-          guardChecksDef(guard, param, val, state) and
+          guardChecksDef(guard, param, val, par) and
           guard.valueControls(bb, val) and
           normalExitBlock(bb) and
           retval = TException(false)
@@ -1365,12 +1364,12 @@ module Make<
       /**
        * Holds if the guard `g` validates the expression `e` upon evaluating to `val`.
        */
-      private predicate guardChecks(Guard g, Expr e, GuardValue val, State state) {
-        guardChecks0(g, e, val.asBooleanValue(), state)
+      private predicate guardChecks(Guard g, Expr e, GuardValue val, P par) {
+        guardChecks0(g, e, val, par)
         or
         exists(NonOverridableMethodCall call, ParameterPosition ppos, ArgumentPosition apos |
           g = call and
-          call.getMethod() = validationWrapper(ppos, val, state) and
+          call.getMethod() = validationWrapper(ppos, val, par) and
           call.getArgument(apos) = e and
           parameterMatch(pragma[only_bind_out](ppos), pragma[only_bind_out](apos))
         )
@@ -1379,9 +1378,9 @@ module Make<
       /**
        * Holds if the guard `g` validates the SSA definition `def` upon evaluating to `val`.
        */
-      predicate guardChecksDef(Guard g, SsaDefinition def, GuardValue val, State state) {
+      predicate guardChecksDef(Guard g, SsaDefinition def, GuardValue val, P par) {
         exists(Expr e |
-          guardChecks(g, e, val, state) and
+          guardChecks(g, e, val, par) and
           guardReadsSsaVar(e, def)
         )
       }