From e5eed2302f40bbf57dcd5f3b8a2d33281faf9831 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 30 Oct 2024 14:56:55 +0100 Subject: [PATCH 1/3] Data flow: Track call contexts in `parameterFlow` --- .../dataflow/internal/DataFlowImplCommon.qll | 140 +++++++++++++----- 1 file changed, 100 insertions(+), 40 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index 81f9946126db..cd2af1634051 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -628,6 +628,8 @@ module MakeImplCommon Lang> { override string toString() { exists(DataFlowCall call | this = TReturn(_, call) | result = "CcReturn(" + call + ")") } + + predicate isReturn(DataFlowCallable c, DataFlowCall call) { this = TReturn(c, call) } } pragma[nomagic] @@ -678,6 +680,8 @@ module MakeImplCommon Lang> { class CcNoCall = CallContextNoCall; + class CcReturn = CallContextReturn; + Cc ccNone() { result instanceof CallContextAny } CcCall ccSomeCall() { result instanceof CallContextSomeCall } @@ -1338,6 +1342,7 @@ module MakeImplCommon Lang> { * or summarized as a single read step with before and after types recorded * in the `ReadStepTypesOption` parameter. * - Types are checked using the `compatibleTypes()` relation. + * - Call contexts are taken into account. */ private module Final { /** @@ -1348,8 +1353,12 @@ module MakeImplCommon Lang> { * If a read step was taken, then `read` captures the `Content`, the * container type, and the content type. */ - predicate parameterValueFlow(ParamNode p, Node node, ReadStepTypesOption read, string model) { - parameterValueFlow0(p, node, read, model) and + predicate parameterValueFlow( + ParamNode p, Node node, ReadStepTypesOption read, string model, + CachedCallContextSensitivity::CcNoCall ctx + ) { + parameterValueFlow0(p, node, read, model, ctx) and + Cand::cand(p, node) and if node instanceof CastingNode then // normal flow through @@ -1369,16 +1378,18 @@ module MakeImplCommon Lang> { pragma[nomagic] private predicate parameterValueFlow0( - ParamNode p, Node node, ReadStepTypesOption read, string model + ParamNode p, Node node, ReadStepTypesOption read, string model, + CachedCallContextSensitivity::CcNoCall ctx ) { p = node and Cand::cand(p, _) and read = TReadStepTypesNone() and - model = "" + model = "" and + CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx) or // local flow exists(Node mid, string model1, string model2 | - parameterValueFlow(p, mid, read, model1) and + parameterValueFlow(p, mid, read, model1, ctx) and simpleLocalFlowStep(mid, node, model2) and validParameterAliasStep(mid, node) and model = mergeModels(model1, model2) @@ -1386,50 +1397,109 @@ module MakeImplCommon Lang> { or // read exists(Node mid | - parameterValueFlow(p, mid, TReadStepTypesNone(), model) and + parameterValueFlow(p, mid, TReadStepTypesNone(), model, ctx) and readStepWithTypes(mid, read.getContainerType(), read.getContent(), node, read.getContentType()) and Cand::parameterValueFlowReturnCand(p, _, true) and compatibleTypesFilter(getNodeDataFlowType(p), read.getContainerType()) ) or - parameterValueFlow0_0(TReadStepTypesNone(), p, node, read, model) + parameterValueFlow0_0(TReadStepTypesNone(), p, node, read, model, ctx) + } + + bindingset[ctx1, ctx2] + pragma[inline_late] + private CachedCallContextSensitivity::CcNoCall mergeContexts( + CachedCallContextSensitivity::CcNoCall ctx1, CachedCallContextSensitivity::CcNoCall ctx2 + ) { + if CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx1) + then result = ctx2 + else + if CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx2) + then result = ctx1 + else + // check that `ctx1` is compatible with `ctx2` for at least _some_ outer call, + // and then (arbitrarily) continue with `ctx2` + exists(DataFlowCall someOuterCall, DataFlowCallable callable | + someOuterCall = + CachedCallContextSensitivity::viableImplCallContextReducedReverse(callable, ctx1) and + someOuterCall = + CachedCallContextSensitivity::viableImplCallContextReducedReverse(callable, ctx2) and + result = ctx2 + ) } pragma[nomagic] private predicate parameterValueFlow0_0( ReadStepTypesOption mustBeNone, ParamNode p, Node node, ReadStepTypesOption read, - string model + string model, CachedCallContextSensitivity::CcNoCall ctx ) { - // flow through: no prior read - exists(ArgNode arg, string model1, string model2 | - parameterValueFlowArg(p, arg, mustBeNone, model1) and - argumentValueFlowsThrough(arg, read, node, model2) and - model = mergeModels(model1, model2) - ) - or - // flow through: no read inside method - exists(ArgNode arg, string model1, string model2 | - parameterValueFlowArg(p, arg, read, model1) and - argumentValueFlowsThrough(arg, mustBeNone, node, model2) and - model = mergeModels(model1, model2) + exists( + DataFlowCall call, DataFlowCallable callable, ArgNode arg, string model1, string model2, + CachedCallContextSensitivity::CcNoCall ctx1, CachedCallContextSensitivity::CcNoCall ctx2 + | + model = mergeModels(model1, model2) and + ( + // call may restrict the set of call sites that can be returned to + ctx2.(CachedCallContextSensitivity::CcReturn).isReturn(callable, call) + or + // call does not restrict the set of call sites that can be returned to + not exists(CachedCallContextSensitivity::CcReturn ret | ret.isReturn(callable, call)) and + CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx2) + ) and + ctx = mergeContexts(ctx1, ctx2) + | + // flow through: no prior read + parameterValueFlowArg(p, arg, mustBeNone, model1, ctx1) and + argumentValueFlowsThrough(call, callable, arg, read, node, model2) + or + // flow through: no read inside method + parameterValueFlowArg(p, arg, read, model1, ctx1) and + argumentValueFlowsThrough(call, callable, arg, mustBeNone, node, model2) ) } pragma[nomagic] private predicate parameterValueFlowArg( - ParamNode p, ArgNode arg, ReadStepTypesOption read, string model + ParamNode p, ArgNode arg, ReadStepTypesOption read, string model, + CachedCallContextSensitivity::CcNoCall ctx ) { - parameterValueFlow(p, arg, read, model) and + parameterValueFlow(p, arg, read, model, ctx) and Cand::argumentValueFlowsThroughCand(arg, _, _) } pragma[nomagic] private predicate argumentValueFlowsThrough0( - DataFlowCall call, ArgNode arg, ReturnKind kind, ReadStepTypesOption read, string model + DataFlowCall call, DataFlowCallable callable, ArgNode arg, ReturnKind kind, + ReadStepTypesOption read, string model ) { - exists(ParamNode param | viableParamArg(call, param, arg) | - parameterValueFlowReturn(param, kind, read, model) + exists(ParamNode param, CachedCallContextSensitivity::CcNoCall ctx | + viableParamArg(call, param, arg) and + parameterValueFlowReturn(param, kind, read, model, ctx) and + callable = nodeGetEnclosingCallable(param) + | + CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx) + or + call = CachedCallContextSensitivity::viableImplCallContextReducedReverse(callable, ctx) + ) + } + + pragma[nomagic] + private predicate argumentValueFlowsThrough( + DataFlowCall call, DataFlowCallable callable, ArgNode arg, ReadStepTypesOption read, + Node out, string model + ) { + exists(ReturnKind kind | + argumentValueFlowsThrough0(call, callable, arg, kind, read, model) and + out = getAnOutNode(call, kind) + | + // normal flow through + read = TReadStepTypesNone() and + compatibleTypesFilter(getNodeDataFlowType(arg), getNodeDataFlowType(out)) + or + // getter + compatibleTypesFilter(getNodeDataFlowType(arg), read.getContainerType()) and + compatibleTypesFilter(read.getContentType(), getNodeDataFlowType(out)) ) } @@ -1445,18 +1515,7 @@ module MakeImplCommon Lang> { predicate argumentValueFlowsThrough( ArgNode arg, ReadStepTypesOption read, Node out, string model ) { - exists(DataFlowCall call, ReturnKind kind | - argumentValueFlowsThrough0(call, arg, kind, read, model) and - out = getAnOutNode(call, kind) - | - // normal flow through - read = TReadStepTypesNone() and - compatibleTypesFilter(getNodeDataFlowType(arg), getNodeDataFlowType(out)) - or - // getter - compatibleTypesFilter(getNodeDataFlowType(arg), read.getContainerType()) and - compatibleTypesFilter(read.getContentType(), getNodeDataFlowType(out)) - ) + argumentValueFlowsThrough(_, _, arg, read, out, model) } /** @@ -1479,10 +1538,11 @@ module MakeImplCommon Lang> { * container type, and the content type. */ private predicate parameterValueFlowReturn( - ParamNode p, ReturnKind kind, ReadStepTypesOption read, string model + ParamNode p, ReturnKind kind, ReadStepTypesOption read, string model, + CachedCallContextSensitivity::CcNoCall ctx ) { exists(ReturnNode ret | - parameterValueFlow(p, ret, read, model) and + parameterValueFlow(p, ret, read, model, ctx) and kind = ret.getKind() ) } @@ -1498,7 +1558,7 @@ module MakeImplCommon Lang> { * node `n`, in the same callable, using only value-preserving steps. */ private predicate parameterValueFlowsToPreUpdate(ParamNode p, PostUpdateNode n) { - parameterValueFlow(p, n.getPreUpdateNode(), TReadStepTypesNone(), _) + parameterValueFlow(p, n.getPreUpdateNode(), TReadStepTypesNone(), _, _) } cached From 5f9b8c05bdf63578aaaeba5cd0be8c56f7748948 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 30 Oct 2024 16:01:32 +0100 Subject: [PATCH 2/3] Java: Update expected test output --- java/ql/test/library-tests/dataflow/getter/getter.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/test/library-tests/dataflow/getter/getter.expected b/java/ql/test/library-tests/dataflow/getter/getter.expected index b5af3f91a593..9a36107f1983 100644 --- a/java/ql/test/library-tests/dataflow/getter/getter.expected +++ b/java/ql/test/library-tests/dataflow/getter/getter.expected @@ -1,6 +1,5 @@ | A.java:5:12:5:15 | this | A.java:5:12:5:19 | this.foo | A.java:2:7:2:9 | foo | | A.java:21:13:21:13 | a | A.java:21:13:21:22 | getFoo(...) | A.java:2:7:2:9 | foo | | A.java:23:9:23:9 | a | A.java:23:9:23:19 | aGetter(...) | A.java:2:7:2:9 | foo | -| A.java:24:9:24:10 | a2 | A.java:24:9:24:23 | notAGetter(...) | A.java:2:7:2:9 | foo | | A.java:45:12:45:38 | maybeIdWrap(...) | A.java:45:12:45:42 | maybeIdWrap(...).foo | A.java:2:7:2:9 | foo | | A.java:49:12:49:38 | maybeIdWrap(...) | A.java:49:12:49:42 | maybeIdWrap(...).foo | A.java:2:7:2:9 | foo | From 3f56fc9e894d7c92aa7b2cb594cae0ae2a300a30 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 20 Nov 2024 12:58:30 +0100 Subject: [PATCH 3/3] Address review comments --- .../dataflow/internal/DataFlowImplCommon.qll | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index cd2af1634051..e477913a59bd 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -628,8 +628,6 @@ module MakeImplCommon Lang> { override string toString() { exists(DataFlowCall call | this = TReturn(_, call) | result = "CcReturn(" + call + ")") } - - predicate isReturn(DataFlowCallable c, DataFlowCall call) { this = TReturn(c, call) } } pragma[nomagic] @@ -1435,27 +1433,19 @@ module MakeImplCommon Lang> { string model, CachedCallContextSensitivity::CcNoCall ctx ) { exists( - DataFlowCall call, DataFlowCallable callable, ArgNode arg, string model1, string model2, - CachedCallContextSensitivity::CcNoCall ctx1, CachedCallContextSensitivity::CcNoCall ctx2 + ArgNode arg, string model1, string model2, CachedCallContextSensitivity::CcNoCall ctx1, + CachedCallContextSensitivity::CcNoCall ctx2 | model = mergeModels(model1, model2) and - ( - // call may restrict the set of call sites that can be returned to - ctx2.(CachedCallContextSensitivity::CcReturn).isReturn(callable, call) - or - // call does not restrict the set of call sites that can be returned to - not exists(CachedCallContextSensitivity::CcReturn ret | ret.isReturn(callable, call)) and - CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx2) - ) and ctx = mergeContexts(ctx1, ctx2) | // flow through: no prior read parameterValueFlowArg(p, arg, mustBeNone, model1, ctx1) and - argumentValueFlowsThrough(call, callable, arg, read, node, model2) + argumentValueFlowsThrough(arg, read, node, model2, ctx2) or // flow through: no read inside method parameterValueFlowArg(p, arg, read, model1, ctx1) and - argumentValueFlowsThrough(call, callable, arg, mustBeNone, node, model2) + argumentValueFlowsThrough(arg, mustBeNone, node, model2, ctx2) ) } @@ -1470,27 +1460,32 @@ module MakeImplCommon Lang> { pragma[nomagic] private predicate argumentValueFlowsThrough0( - DataFlowCall call, DataFlowCallable callable, ArgNode arg, ReturnKind kind, - ReadStepTypesOption read, string model + DataFlowCall call, ArgNode arg, ReturnKind kind, ReadStepTypesOption read, string model, + CachedCallContextSensitivity::CcNoCall outerCtx ) { - exists(ParamNode param, CachedCallContextSensitivity::CcNoCall ctx | + exists( + ParamNode param, DataFlowCallable callable, + CachedCallContextSensitivity::CcNoCall innerCtx + | viableParamArg(call, param, arg) and - parameterValueFlowReturn(param, kind, read, model, ctx) and - callable = nodeGetEnclosingCallable(param) + parameterValueFlowReturn(param, kind, read, model, innerCtx) and + callable = nodeGetEnclosingCallable(param) and + outerCtx = CachedCallContextSensitivity::getCallContextReturn(callable, call) | - CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(ctx) + CachedCallContextSensitivity::viableImplNotCallContextReducedReverse(innerCtx) or - call = CachedCallContextSensitivity::viableImplCallContextReducedReverse(callable, ctx) + call = + CachedCallContextSensitivity::viableImplCallContextReducedReverse(callable, innerCtx) ) } pragma[nomagic] private predicate argumentValueFlowsThrough( - DataFlowCall call, DataFlowCallable callable, ArgNode arg, ReadStepTypesOption read, - Node out, string model + ArgNode arg, ReadStepTypesOption read, Node out, string model, + CachedCallContextSensitivity::CcNoCall ctx ) { - exists(ReturnKind kind | - argumentValueFlowsThrough0(call, callable, arg, kind, read, model) and + exists(DataFlowCall call, ReturnKind kind | + argumentValueFlowsThrough0(call, arg, kind, read, model, ctx) and out = getAnOutNode(call, kind) | // normal flow through @@ -1515,7 +1510,7 @@ module MakeImplCommon Lang> { predicate argumentValueFlowsThrough( ArgNode arg, ReadStepTypesOption read, Node out, string model ) { - argumentValueFlowsThrough(_, _, arg, read, out, model) + argumentValueFlowsThrough(arg, read, out, model, _) } /**