From 9c4a378dc7ef6cc99aa9f9f3b0e3938a6e190135 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 18 Jun 2024 10:42:03 +0200 Subject: [PATCH] refactor2 --- .../codeql/dataflow/internal/DataFlowImpl.qll | 105 ++++++++++-------- .../dataflow/internal/DataFlowImplCommon.qll | 40 +++---- .../CWE-078/CommandInjection.expected | 14 --- 3 files changed, 77 insertions(+), 82 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 3974233dbb177..f7f88e777f55f 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -1021,6 +1021,10 @@ module MakeImpl Lang> { callEdgeReturn(call, c, _, _, _, _, _) } + predicate storeMayReachRead(NodeEx storeSource, Content c, NodeEx readTarget) { none() } + + predicate providesStoreReadMatching() { none() } + additional predicate stats( boolean fwd, int nodes, int fields, int conscand, int states, int tuples, int calledges ) { @@ -1320,6 +1324,10 @@ module MakeImpl Lang> { predicate relevantCallEdgeIn(DataFlowCall call, DataFlowCallable c); predicate relevantCallEdgeOut(DataFlowCall call, DataFlowCallable c); + + predicate storeMayReachRead(NodeEx storeSource, Content c, NodeEx readTarget); + + default predicate providesStoreReadMatching() { any() } } private module MkStage { @@ -1425,8 +1433,6 @@ module MakeImpl Lang> { predicate typecheckStore(Typ typ, DataFlowType contentType); default predicate enableTypeFlow() { any() } - - default predicate enableStoreReadMatching() { any() } } module Stage implements StageSig { @@ -1482,43 +1488,49 @@ module MakeImpl Lang> { private class NodeExAlias = NodeEx; + final private class ApFinal = Ap; + private module StoreReadMatchingInput implements StoreReadMatchingInputSig { class NodeEx = NodeExAlias; - final private class ApApproxFinal = PrevStage::Ap; - - class Ap extends ApApproxFinal { - Content getHead() { result = PrevStage::getAHeadContent(this) } + class Ap extends ApFinal { + Content getHead() { result = getAHeadContent(this) } } - predicate revFlow(NodeEx node, Ap ap) { - enableStoreReadMatching() and PrevStage::revFlowAp(node, ap) - } + predicate nodeApRange(NodeEx node, Ap ap) { revFlowAp(node, ap) } - predicate localValueStep(NodeEx node1, NodeEx node2) { localValueStep(node1, node2, _) } + predicate localValueStep(NodeEx node1, NodeEx node2) { + exists(FlowState state, Ap ap, ApOption returnAp | + revFlow(node1, pragma[only_bind_into](state), _, pragma[only_bind_into](returnAp), + pragma[only_bind_into](ap)) and + revFlow(node2, pragma[only_bind_into](state), _, pragma[only_bind_into](returnAp), + pragma[only_bind_into](ap)) and + localValueStep(node1, node2, _) + ) + } predicate jumpValueStep(NodeEx node1, NodeEx node2) { jumpStepEx(node1, node2) } - predicate callEdgeArgParam(NodeEx arg, NodeEx param, Ap ap) { - PrevStage::callEdgeArgParam(_, _, arg, param, true, ap) + predicate callEdgeArgParam(NodeEx arg, NodeEx param) { + callEdgeArgParam(_, _, arg, param, true, _) } - predicate callEdgeReturn(NodeEx ret, NodeEx out, Ap ap) { - PrevStage::callEdgeReturn(_, _, ret, _, out, true, ap) + predicate callEdgeReturn(NodeEx ret, NodeEx out) { + callEdgeReturn(_, _, ret, _, out, true, _) } predicate readContentStep(NodeEx node1, Content c, NodeEx node2) { - PrevStage::readStepCand(node1, c, node2) + readStepCand0(node1, c, node2) } predicate storeContentStep(NodeEx node1, Content c, NodeEx node2) { - PrevStage::storeStepCand(node1, _, c, node2, _, _) + storeStepCand0(node1, _, c, node2, _, _) } int accessPathConfigLimit() { result = Config::accessPathLimit() } } - private import StoreReadMatching + import StoreReadMatching /** * Holds if `node` is reachable with access path `ap` from a source. @@ -1562,7 +1574,7 @@ module MakeImpl Lang> { private predicate storeMayReachReadInlineLate( NodeEx storeSource, Content c, NodeEx readTarget ) { - storeMayReachRead(storeSource, c, readTarget) + PrevStage::storeMayReachRead(storeSource, c, readTarget) } bindingset[node1, state1] @@ -1689,8 +1701,7 @@ module MakeImpl Lang> { fwdFlow(node1, state, cc, summaryCtx, argT, argAp, t1, ap1, apa1) and PrevStage::storeStepCand(node1, apa1, c, node2, contentType, containerType) and t2 = getTyp(containerType) and - typecheckStore(t1, contentType) and - if enableStoreReadMatching() then storeMayReachRead(node1, c, _) else any() + typecheckStore(t1, contentType) ) } @@ -1699,13 +1710,14 @@ module MakeImpl Lang> { * store of `c` on a container of type `t2` resulting in access path * `cons`. `storeSource` is a relevant store source. * - * This predicate is only evaluated when `enableStoreReadMatching()` holds. + * This predicate is only evaluated when `PrevStage::providesStoreReadMatching()` + * holds. */ pragma[nomagic] private predicate fwdFlowConsCandStoreReadMatchingEnabled( NodeEx storeSource, Typ t2, Ap cons, Content c, Typ t1, Ap tail ) { - enableStoreReadMatching() and + PrevStage::providesStoreReadMatching() and fwdFlowStore(storeSource, t1, tail, c, t2, _, _, _, _, _, _) and cons = apCons(c, t1, tail) or @@ -1720,14 +1732,14 @@ module MakeImpl Lang> { * store of `c` on a container of type `t2` resulting in access path * `cons`. * - * This predicate is only evaluated when `enableStoreReadMatching()` + * This predicate is only evaluated when `PrevStage::providesStoreReadMatching()` * doesn't hold. */ pragma[nomagic] private predicate fwdFlowConsCandStoreReadMatchingDisabled( Typ t2, Ap cons, Content c, Typ t1, Ap tail ) { - not enableStoreReadMatching() and + not PrevStage::providesStoreReadMatching() and fwdFlowStore(_, t1, tail, c, t2, _, _, _, _, _, _) and cons = apCons(c, t1, tail) or @@ -1740,8 +1752,7 @@ module MakeImpl Lang> { pragma[nomagic] private predicate readStepCand(NodeEx node1, ApHeadContent apc, Content c, NodeEx node2) { PrevStage::readStepCand(node1, c, node2) and - apc = projectToHeadContent(c) and - if enableStoreReadMatching() then storeMayReachRead(_, c, node2) else any() + apc = projectToHeadContent(c) } bindingset[node1, apc] @@ -2392,7 +2403,7 @@ module MakeImpl Lang> { */ pragma[nomagic] private predicate revFlowConsCandStoreReadMatchingDisabled(Ap cons, Content c, Ap tail) { - not enableStoreReadMatching() and + not PrevStage::providesStoreReadMatching() and revFlowConsCand(_, cons, c, tail) } @@ -2513,7 +2524,7 @@ module MakeImpl Lang> { } pragma[nomagic] - predicate storeStepCand( + private predicate storeStepCand0( NodeEx node1, Ap ap1, Content c, NodeEx node2, DataFlowType contentType, DataFlowType containerType ) { @@ -2525,7 +2536,7 @@ module MakeImpl Lang> { } pragma[nomagic] - predicate readStepCand(NodeEx node1, Content c, NodeEx node2) { + private predicate readStepCand0(NodeEx node1, Content c, NodeEx node2) { exists(Ap ap1, Ap ap2 | revFlow(node2, _, _, _, pragma[only_bind_into](ap2)) and readStepFwd(node1, ap1, c, node2, ap2) and @@ -2546,12 +2557,9 @@ module MakeImpl Lang> { private predicate fwdConsCand(Content c, Typ t, Ap ap) { storeStepFwd(_, t, ap, c, _, _) } private predicate revConsCand(NodeEx readTarget, Content c, Typ t, Ap ap) { - exists(NodeEx storeSource, Ap ap2 | - revFlowStore(ap2, c, ap, t, storeSource, _, _, _, _) and - revFlowConsCand(readTarget, ap2, c, ap) and - if enableStoreReadMatching() - then storeMayReachRead(storeSource, c, readTarget) - else any() + exists(Ap ap2 | + revFlowStore(ap2, c, ap, t, _, _, _, _, _) and + revFlowConsCand(readTarget, ap2, c, ap) ) } @@ -2856,8 +2864,14 @@ module MakeImpl Lang> { // read exists(NodeEx mid, Typ t0, Ap ap0, Content c | pn1 = TStagePathNodeMid(mid, state, cc, summaryCtx, argT, argAp, t0, ap0) and - fwdFlowRead(t0, ap0, c, mid, node, state, cc, summaryCtx, argT, argAp) and - fwdFlowConsCand(t0, ap0, c, t, ap) + fwdFlowRead(t0, ap0, c, mid, node, state, cc, summaryCtx, argT, argAp) + | + exists(NodeEx storeSource | + fwdFlowConsCandStoreReadMatchingEnabled(storeSource, t0, ap0, c, t, ap) and + storeMayReachReadInlineLate(storeSource, c, node) + ) + or + fwdFlowConsCandStoreReadMatchingDisabled(t0, ap0, c, t, ap) ) or // flow into a callable @@ -2933,8 +2947,6 @@ module MakeImpl Lang> { * be able to handle access paths of maximum length 5. */ private module StoreReadReachability { - final private class ApFinal = Ap; - private class ApCons extends ApFinal { ApCons() { not this instanceof ApNil } } @@ -3380,8 +3392,6 @@ module MakeImpl Lang> { predicate typecheckStore(Typ typ, DataFlowType contentType) { any() } predicate enableTypeFlow() { none() } - - predicate enableStoreReadMatching() { none() } } private module Stage2 = MkStage::Stage; @@ -3631,7 +3641,8 @@ module MakeImpl Lang> { predicate localTaintStep( NodeEx node1, FlowState state1, NodeEx node2, FlowState state2, Typ t, LocalCc lcc ) { - localFlowBigStep(node1, state1, node2, state2, false, t, _, _) and + localFlowBigStep(node1, state1, node2, state2, false, _, _, _) and + exists(t) and exists(lcc) } @@ -4031,10 +4042,9 @@ module MakeImpl Lang> { pragma[nomagic] predicate localValueStep(NodeEx node1, NodeEx node2, LocalCc lcc) { exists(FlowState state | - localFlowBigStep(node1, _, node2, _, true, _, _, _) and + localFlowBigStep(node1, _, node2, _, true, _, lcc, _) and PrevStage::revFlow(node1, pragma[only_bind_into](state), _) and - PrevStage::revFlow(node2, pragma[only_bind_into](state), _) and - exists(lcc) + PrevStage::revFlow(node2, pragma[only_bind_into](state), _) ) } @@ -4042,10 +4052,9 @@ module MakeImpl Lang> { predicate localTaintStep( NodeEx node1, FlowState state1, NodeEx node2, FlowState state2, Typ t, LocalCc lcc ) { - localFlowBigStep(node1, state1, node2, state2, false, t, _, _) and + localFlowBigStep(node1, state1, node2, state2, false, t, lcc, _) and PrevStage::revFlow(node1, pragma[only_bind_into](state1), _) and - PrevStage::revFlow(node2, pragma[only_bind_into](state2), _) and - exists(lcc) + PrevStage::revFlow(node2, pragma[only_bind_into](state2), _) } bindingset[node, state, t0, ap] diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index 01d123b215e2c..3b445dc96320d 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -2363,15 +2363,15 @@ module MakeImplCommon Lang> { Content getHead(); } - predicate revFlow(NodeEx node, Ap ap); + predicate nodeApRange(NodeEx node, Ap ap); predicate localValueStep(NodeEx node1, NodeEx node2); predicate jumpValueStep(NodeEx node1, NodeEx node2); - predicate callEdgeArgParam(NodeEx arg, NodeEx param, Ap ap); + predicate callEdgeArgParam(NodeEx arg, NodeEx param); - predicate callEdgeReturn(NodeEx ret, NodeEx out, Ap ap); + predicate callEdgeReturn(NodeEx ret, NodeEx out); predicate readContentStep(NodeEx node1, Content c, NodeEx node2); @@ -2410,16 +2410,16 @@ module MakeImplCommon Lang> { pragma[nomagic] private predicate valueStep(NodeEx node1, NodeEx node2) { exists(ApCons ap | - revFlow(node1, pragma[only_bind_into](ap)) and - revFlow(node2, pragma[only_bind_into](ap)) + nodeApRange(node1, pragma[only_bind_into](ap)) and + nodeApRange(node2, pragma[only_bind_into](ap)) | localValueStep(node1, node2) or jumpValueStep(node1, node2) or - callEdgeArgParam(node1, node2, ap) + callEdgeArgParam(node1, node2) or - callEdgeReturn(node1, node2, ap) + callEdgeReturn(node1, node2) ) } @@ -2448,7 +2448,7 @@ module MakeImplCommon Lang> { private newtype TNodeOrContent = TNodeOrContentNode(NodeEx n, ApCons ap, UsesPrevDeltaInfo usesPrevDelta) { enabled() and - revFlow(n, ap) + nodeApRange(n, ap) } or TNodeOrContentContentStart(Content c) { enabled() and storeContentStep(_, c, _) } or TNodeOrContentContentEnd(Content c) { enabled() and readContentStep(_, c, _) } @@ -2471,6 +2471,10 @@ module MakeImplCommon Lang> { } } + bindingset[ap, result] + pragma[inline_late] + private Content getHeadInlineLate(Ap ap) { result = ap.getHead() } + pragma[nomagic] private predicate step(NodeOrContent node1, NodeOrContent node2) { exists( @@ -2530,22 +2534,24 @@ module MakeImplCommon Lang> { ) } + pragma[nomagic] private predicate isStoreTarget0(NodeOrContent node, Content c) { exists(Ap ap, UsesPrevDeltaInfo usesPrevDelta | contentIsReadAndStored(c) and storeContentStep(_, c, node.asNodeEx(ap, usesPrevDelta)) and - c = ap.getHead() and + c = getHeadInlineLate(ap) and usesPrevDelta.toBoolean() = false ) } private predicate isStoreTarget(NodeOrContent node) { isStoreTarget0(node, _) } + pragma[nomagic] private predicate isReadSource0(NodeOrContent node, Content c) { exists(Ap ap, UsesPrevDeltaInfo usesPrevDelta | contentIsReadAndStored(c) and readContentStep(node.asNodeEx(ap, usesPrevDelta), c, _) and - c = ap.getHead() and + c = getHeadInlineLate(ap) and usesPrevDelta.toBoolean() = true ) } @@ -2584,15 +2590,9 @@ module MakeImplCommon Lang> { ) } - private predicate isStoreTargetPruned(NodeOrContent node) { - isStoreTarget(node) and - storeMayReachARead(node, _) - } + private predicate isStoreTargetPruned(NodeOrContent node) { storeMayReachARead(node, _) } - private predicate isReadSourcePruned(NodeOrContent node) { - isReadSource(node) and - aStoreMayReachRead(node, _) - } + private predicate isReadSourcePruned(NodeOrContent node) { aStoreMayReachRead(node, _) } private predicate storeMayReachReadTc(NodeOrContent node1, NodeOrContent node2) = doublyBoundedFastTC(step/2, isStoreTargetPruned/1, isReadSourcePruned/1)(node1, node2) @@ -2602,7 +2602,7 @@ module MakeImplCommon Lang> { exists(Ap ap, UsesPrevDeltaInfo usesPrevDelta | storeMayReachARead(node2, c) and storeContentStep(node1, c, node2.asNodeEx(ap, usesPrevDelta)) and - c = ap.getHead() and + c = getHeadInlineLate(ap) and usesPrevDelta.toBoolean() = false ) } @@ -2612,7 +2612,7 @@ module MakeImplCommon Lang> { exists(Ap ap, UsesPrevDeltaInfo usesPrevDelta | aStoreMayReachRead(node1, c) and readContentStep(node1.asNodeEx(ap, usesPrevDelta), c, node2) and - c = ap.getHead() and + c = getHeadInlineLate(ap) and usesPrevDelta.toBoolean() = true ) } diff --git a/swift/ql/test/query-tests/Security/CWE-078/CommandInjection.expected b/swift/ql/test/query-tests/Security/CWE-078/CommandInjection.expected index f9c1f7bd17553..0fe8ae7b8685d 100644 --- a/swift/ql/test/query-tests/Security/CWE-078/CommandInjection.expected +++ b/swift/ql/test/query-tests/Security/CWE-078/CommandInjection.expected @@ -44,7 +44,6 @@ edges | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:137:36:137:36 | userControlledString | provenance | | | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:138:21:138:21 | userControlledString | provenance | | | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:139:22:139:22 | userControlledString | provenance | | -| CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:140:24:140:24 | userControlledString | provenance | | | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:150:42:150:42 | userControlledString | provenance | | | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:151:75:151:75 | userControlledString | provenance | | | CommandInjection.swift:105:12:105:12 | userControlledString | CommandInjection.swift:154:35:154:35 | userControlledString | provenance | | @@ -65,7 +64,6 @@ edges | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:137:36:137:36 | userControlledString | provenance | | | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:138:21:138:21 | userControlledString | provenance | | | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:139:22:139:22 | userControlledString | provenance | | -| CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:140:24:140:24 | userControlledString | provenance | | | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:150:42:150:42 | userControlledString | provenance | | | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:151:75:151:75 | userControlledString | provenance | | | CommandInjection.swift:105:40:105:94 | call to String.init(contentsOf:) | CommandInjection.swift:154:35:154:35 | userControlledString | provenance | | @@ -110,14 +108,6 @@ edges | CommandInjection.swift:138:21:138:21 | userControlledString | CommandInjection.swift:138:20:138:41 | [...] [Collection element] | provenance | | | CommandInjection.swift:139:21:139:42 | [...] [Collection element] | CommandInjection.swift:99:20:99:40 | arguments [Collection element] | provenance | | | CommandInjection.swift:139:22:139:22 | userControlledString | CommandInjection.swift:139:21:139:42 | [...] [Collection element] | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:150:42:150:42 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:151:75:151:75 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:154:35:154:35 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:155:70:155:70 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:160:53:160:53 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:163:52:163:52 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:164:33:164:33 | userControlledString | provenance | | -| CommandInjection.swift:140:24:140:24 | userControlledString | CommandInjection.swift:166:57:166:57 | userControlledString | provenance | | | CommandInjection.swift:151:67:151:95 | [...] [Collection element] | CommandInjection.swift:151:67:151:95 | [...] | provenance | | | CommandInjection.swift:151:75:151:75 | userControlledString | CommandInjection.swift:151:67:151:95 | [...] [Collection element] | provenance | | | CommandInjection.swift:154:23:154:55 | call to URL.init(string:) [some:0] | CommandInjection.swift:154:23:154:56 | ...! | provenance | | @@ -158,11 +148,8 @@ edges | CommandInjection.swift:205:19:205:19 | userControlledString | CommandInjection.swift:205:18:205:39 | [...] [Collection element] | provenance | | | CommandInjection.swift:207:3:207:3 | [post] getter for .p1 [arguments, Collection element] | CommandInjection.swift:207:3:207:3 | [post] getter for .p1 | provenance | | | CommandInjection.swift:207:18:207:18 | tainted1 [Collection element] | CommandInjection.swift:207:3:207:3 | [post] getter for .p1 [arguments, Collection element] | provenance | | -| CommandInjection.swift:207:18:207:18 | tainted1 [Collection element] | CommandInjection.swift:208:19:208:19 | tainted1 [Collection element] | provenance | | -| CommandInjection.swift:207:18:207:18 | tainted1 [Collection element] | CommandInjection.swift:209:18:209:18 | tainted1 [Collection element] | provenance | | | CommandInjection.swift:208:3:208:5 | [post] ...! [arguments, Collection element] | CommandInjection.swift:208:3:208:5 | [post] ...! | provenance | | | CommandInjection.swift:208:19:208:19 | tainted1 [Collection element] | CommandInjection.swift:208:3:208:5 | [post] ...! [arguments, Collection element] | provenance | | -| CommandInjection.swift:208:19:208:19 | tainted1 [Collection element] | CommandInjection.swift:209:18:209:18 | tainted1 [Collection element] | provenance | | | CommandInjection.swift:209:3:209:3 | [post] ...! [arguments, Collection element] | CommandInjection.swift:209:3:209:3 | [post] ...! | provenance | | | CommandInjection.swift:209:18:209:18 | tainted1 [Collection element] | CommandInjection.swift:209:3:209:3 | [post] ...! [arguments, Collection element] | provenance | | | CommandInjection.swift:211:30:211:51 | [...] [Collection element] | CommandInjection.swift:213:18:213:18 | tainted2 [Collection element] | provenance | | @@ -259,7 +246,6 @@ nodes | CommandInjection.swift:138:21:138:21 | userControlledString | semmle.label | userControlledString | | CommandInjection.swift:139:21:139:42 | [...] [Collection element] | semmle.label | [...] [Collection element] | | CommandInjection.swift:139:22:139:22 | userControlledString | semmle.label | userControlledString | -| CommandInjection.swift:140:24:140:24 | userControlledString | semmle.label | userControlledString | | CommandInjection.swift:150:42:150:42 | userControlledString | semmle.label | userControlledString | | CommandInjection.swift:151:67:151:95 | [...] | semmle.label | [...] | | CommandInjection.swift:151:67:151:95 | [...] [Collection element] | semmle.label | [...] [Collection element] |