diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll index 285e0dc8419..8dc3513b444 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll @@ -1051,12 +1051,12 @@ module BarrierGuardWithIntParam { } 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 { DataFlowIntegrationInput::Guard g, SsaImpl::Definition def, IRGuards::GuardValue val, int indirectionIndex ) { - IRGuards::Guards_v1::ValidationWrapperWithState::guardChecksDef(g, def, - val, indirectionIndex) + IRGuards::Guards_v1::ParameterizedValidationWrapper::guardChecksDef(g, + def, val, indirectionIndex) } Node getABarrierNode(int indirectionIndex) { diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index 00e7d15ee8b..d96834a5533 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/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 { + /** Gets a node that is safely guarded by the given guard check. */ + Node getABarrierNode() { + SsaFlow::asNode(result) = + SsaImpl::DataFlowIntegration::BarrierGuard::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 { - /** Gets a node that is safely guarded by the given guard check. */ - Node getABarrierNode() { - SsaFlow::asNode(result) = - SsaImpl::DataFlowIntegration::BarrierGuard::getABarrierNode() + 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() { result = BarrierGuardValue::getABarrierNode() } } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll index 624f82fd341..323f14f550f 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll +++ b/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 { - 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( diff --git a/java/ql/test/library-tests/dataflow/ssa/A.java b/java/ql/test/library-tests/dataflow/ssa/A.java index 79303697d64..d7047bec276 100644 --- a/java/ql/test/library-tests/dataflow/ssa/A.java +++ b/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); + } } diff --git a/java/ql/test/library-tests/dataflow/ssa/test.ql b/java/ql/test/library-tests/dataflow/ssa/test.ql index cb601dab5eb..0bea0aa71a1 100644 --- a/java/ql/test/library-tests/dataflow/ssa/test.ql +++ b/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::getABarrierNode() + or + node = DataFlow::BarrierGuardValue::getABarrierNode() } } diff --git a/shared/controlflow/codeql/controlflow/Guards.qll b/shared/controlflow/codeql/controlflow/Guards.qll index c0d07278b9a..b313afcdb6b 100644 --- a/shared/controlflow/codeql/controlflow/Guards.qll +++ b/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 { - signature predicate guardChecksSig(Guard g, Expr e, boolean branch, State state); + private module WithParam { + signature predicate guardChecksSig(Guard g, Expr e, GuardValue gv, P par); } /** * Extends a `BarrierGuard` input predicate with wrapped invocations. */ module ValidationWrapper { - 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; + private module ParameterizedWrapper = + ParameterizedValidationWrapper; /** * 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::guardChecksSig/4 guardChecks0> - { + module ParameterizedValidationWrapper::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) ) }