From 788d7725562ac40bc821a0cb20452762eda03bd7 Mon Sep 17 00:00:00 2001 From: Michael Hohn Date: Wed, 29 Nov 2023 20:21:32 -0800 Subject: [PATCH] Add PreGuardRecursivePredicate test --- solutions/PreGuardRecursivePredicate.ql | 133 ++++++++++++++++++ .../PreGuardRecursivePredicate.expected | 48 +++++++ .../PreGuardRecursivePredicate.qlref | 1 + .../sample-utility-0.js | 29 ++++ .../sample-utility-1.js | 17 +++ 5 files changed, 228 insertions(+) create mode 100644 solutions/PreGuardRecursivePredicate.ql create mode 100644 tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.expected create mode 100644 tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.qlref create mode 100644 tests/PreGuardRecursivePredicate/sample-utility-0.js create mode 100644 tests/PreGuardRecursivePredicate/sample-utility-1.js diff --git a/solutions/PreGuardRecursivePredicate.ql b/solutions/PreGuardRecursivePredicate.ql new file mode 100644 index 0000000..235055d --- /dev/null +++ b/solutions/PreGuardRecursivePredicate.ql @@ -0,0 +1,133 @@ +/** + * @kind path-problem + * @problem.severity warning + * @id javascript/example/multiflow + */ + +import javascript +// XX: debug flow query +// import semmle.javascript.explore.ForwardDataFlow +import DataFlow::PathGraph +import DataFlow as DF + +// Flow to consider: +// +// var value = this.getParameter('value'); //: source 1 +// var ua = new GR('status'); //: source 2 +// ua.setValue('status',value); //: taint step +// ua.update(); //: sink (if from source 2) +// +// var value = this.getParameter('value'); //: source 1 +class ParameterSource extends CallExpr { + ParameterSource() { + exists(Expr inst | + this.getCalleeName() = "getParameter" and + (this.getReceiver().(ThisExpr) = inst or this.getReceiver().(Identifier) = inst) + ) + } +} + +// ua.setValue('status',value); //: taint step +predicate setValueTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DotExpr temp, MethodCallExpr mce, VarAccess gr, VarAccess postgr | + temp.getPropertyName() = "setValue" and + mce.getReceiver() = temp.getBase() and + gr = mce.getReceiver() and + pred.asExpr() = mce.getArgument(1) and + // + // Taint all accesses after setValue call. + // Trying data flow, this would be: + // succ = gr.flow().getASuccessor+() and + // + // Using control flow: + // 1. without sanitizer + // gr.getASuccessor+() = postgr and + // succ.asExpr() = postgr + // + // 2. with recursive predicate, no sanitizer + recursiveSuccessor(gr, postgr) and + succ.asExpr() = postgr + // // 3. with recursive predicate, with sanitizer + // sanitizerCheckedSuccessor(gr, postgr) and + // succ.asExpr() = postgr + ) +} + +predicate foo(VarAccess gr, VarAccess postgr) { + exists(DotExpr temp, MethodCallExpr mce | + temp.getPropertyName() = "setValue" and + mce.getReceiver() = temp.getBase() and + gr = mce.getReceiver() and + gr.getASuccessor+() = postgr + ) +} + +predicate foo1(Expr gr, Expr postgr) { + exists(DotExpr temp, MethodCallExpr mce | + temp.getPropertyName() = "setValue" and + mce.getReceiver() = temp.getBase() and + gr = mce.getReceiver() and + recursiveSuccessor(gr, postgr) + ) +} + +// Def-Use special handling: +// Include sanitizer check when flagging successive object member calls in taint step +predicate recursiveSuccessor(ControlFlowNode gr, ControlFlowNode postgr) { + gr.getASuccessor() = postgr + or + exists(ControlFlowNode p | + recursiveSuccessor(gr, p) and + p.getASuccessor() = postgr + ) + // The final postgr needs to be a VarAccess for this query, but for the + // recursion we need to be able to traverse expressions. +} + +// source 2 to sink flow +DF::SourceNode grType(DF::TypeTracker t) { + t.start() and + exists(GR gr | result.asExpr() = gr) + or + exists(DF::TypeTracker t2 | result = grType(t2).track(t2, t)) +} + +DF::SourceNode grType() { result = grType(DF::TypeTracker::end()) } + +// ua.update(); //: sink (if from source 2) +DotExpr updateExpression() { result.getPropertyName() = "update" } + +VarRef recordUpdate() { result = updateExpression().getBase() } + +// var ua = new GR('status'); //: source 2 +class GR extends NewExpr { + GR() { this.getCalleeName() = "GR" } +} + +// The global flow configuration +class FromRequestToGrUpdate extends TaintTracking::Configuration { + FromRequestToGrUpdate() { this = "FromRequestToGrUpdate" } + + override predicate isSource(DataFlow::Node source) { + exists(ParameterSource getParameter | source.asExpr() = getParameter) + } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + setValueTaintStep(pred, succ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(VarRef grUpdate | + sink.asExpr() = recordUpdate() and + grUpdate = sink.asExpr() and + grUpdate.getName() = "ua" and + // It's only a sink if it connects to source 2 + grUpdate.flow().getALocalSource() = grType() + ) + } +} + +from FromRequestToGrUpdate dataflow, DataFlow::PathNode source, DataFlow::PathNode sink +where dataflow.hasFlowPath(source, sink) +select sink, source, sink, "Data flow from $@ to $@.", source, source.toString(), sink, + sink.toString() diff --git a/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.expected b/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.expected new file mode 100644 index 0000000..a26c709 --- /dev/null +++ b/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.expected @@ -0,0 +1,48 @@ +WARNING: Unused predicate foo (/Users/hohn/local/codeql-javascript-multiflow/solutions/PreGuardRecursivePredicate.ql:56,11-14) +WARNING: Unused predicate foo1 (/Users/hohn/local/codeql-javascript-multiflow/solutions/PreGuardRecursivePredicate.ql:65,11-15) +nodes +| sample-utility-0.js:5:13:5:46 | value | +| sample-utility-0.js:5:21:5:46 | this.ge ... value') | +| sample-utility-0.js:5:21:5:46 | this.ge ... value') | +| sample-utility-0.js:12:34:12:38 | value | +| sample-utility-0.js:17:34:17:38 | value | +| sample-utility-0.js:18:13:18:14 | ua | +| sample-utility-0.js:18:13:18:14 | ua | +| sample-utility-0.js:22:35:22:39 | value | +| sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-1.js:2:9:2:42 | value | +| sample-utility-1.js:2:17:2:42 | this.ge ... value') | +| sample-utility-1.js:2:17:2:42 | this.ge ... value') | +| sample-utility-1.js:14:23:14:27 | value | +| sample-utility-1.js:15:2:15:3 | ua | +| sample-utility-1.js:15:2:15:3 | ua | +edges +| sample-utility-0.js:5:13:5:46 | value | sample-utility-0.js:12:34:12:38 | value | +| sample-utility-0.js:5:13:5:46 | value | sample-utility-0.js:17:34:17:38 | value | +| sample-utility-0.js:5:13:5:46 | value | sample-utility-0.js:22:35:22:39 | value | +| sample-utility-0.js:5:21:5:46 | this.ge ... value') | sample-utility-0.js:5:13:5:46 | value | +| sample-utility-0.js:5:21:5:46 | this.ge ... value') | sample-utility-0.js:5:13:5:46 | value | +| sample-utility-0.js:12:34:12:38 | value | sample-utility-0.js:12:34:12:38 | value | +| sample-utility-0.js:12:34:12:38 | value | sample-utility-0.js:22:35:22:39 | value | +| sample-utility-0.js:12:34:12:38 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:12:34:12:38 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:17:34:17:38 | value | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:18:13:18:14 | ua | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:18:13:18:14 | ua | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:22:35:22:39 | value | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:17:34:17:38 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:22:35:22:39 | value | sample-utility-0.js:22:35:22:39 | value | +| sample-utility-0.js:22:35:22:39 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-0.js:22:35:22:39 | value | sample-utility-0.js:23:13:23:14 | ua | +| sample-utility-1.js:2:9:2:42 | value | sample-utility-1.js:14:23:14:27 | value | +| sample-utility-1.js:2:17:2:42 | this.ge ... value') | sample-utility-1.js:2:9:2:42 | value | +| sample-utility-1.js:2:17:2:42 | this.ge ... value') | sample-utility-1.js:2:9:2:42 | value | +| sample-utility-1.js:14:23:14:27 | value | sample-utility-1.js:14:23:14:27 | value | +| sample-utility-1.js:14:23:14:27 | value | sample-utility-1.js:15:2:15:3 | ua | +| sample-utility-1.js:14:23:14:27 | value | sample-utility-1.js:15:2:15:3 | ua | +#select +| sample-utility-0.js:18:13:18:14 | ua | sample-utility-0.js:5:21:5:46 | this.ge ... value') | sample-utility-0.js:18:13:18:14 | ua | Data flow from $@ to $@. | sample-utility-0.js:5:21:5:46 | this.ge ... value') | this.ge ... value') | sample-utility-0.js:18:13:18:14 | ua | ua | +| sample-utility-0.js:23:13:23:14 | ua | sample-utility-0.js:5:21:5:46 | this.ge ... value') | sample-utility-0.js:23:13:23:14 | ua | Data flow from $@ to $@. | sample-utility-0.js:5:21:5:46 | this.ge ... value') | this.ge ... value') | sample-utility-0.js:23:13:23:14 | ua | ua | +| sample-utility-1.js:15:2:15:3 | ua | sample-utility-1.js:2:17:2:42 | this.ge ... value') | sample-utility-1.js:15:2:15:3 | ua | Data flow from $@ to $@. | sample-utility-1.js:2:17:2:42 | this.ge ... value') | this.ge ... value') | sample-utility-1.js:15:2:15:3 | ua | ua | diff --git a/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.qlref b/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.qlref new file mode 100644 index 0000000..cd96ff0 --- /dev/null +++ b/tests/PreGuardRecursivePredicate/PreGuardRecursivePredicate.qlref @@ -0,0 +1 @@ +PreGuardRecursivePredicate.ql diff --git a/tests/PreGuardRecursivePredicate/sample-utility-0.js b/tests/PreGuardRecursivePredicate/sample-utility-0.js new file mode 100644 index 0000000..4549868 --- /dev/null +++ b/tests/PreGuardRecursivePredicate/sample-utility-0.js @@ -0,0 +1,29 @@ +var SampleUtility = function(){}; +SampleUtility.prototype = Object.extendsObject(Processor, { + + setUserStatus: function() { + var value = this.getParameter('value'); + + var ua = new GR('users'); + ua.query(); + + if(!ua.hasNext()){ + ua.initialize(); + ua.setValue('status',value); + ua.insert(); + } + else { + ua.next(); + ua.setValue('status',value); // unsafe + ua.update(); + } + + if (ua.safeToWrite()) { + ua.setValue('status', value); // safe + ua.update(); + } + + }, + + type: 'SampleUtility' +}); diff --git a/tests/PreGuardRecursivePredicate/sample-utility-1.js b/tests/PreGuardRecursivePredicate/sample-utility-1.js new file mode 100644 index 0000000..aea3135 --- /dev/null +++ b/tests/PreGuardRecursivePredicate/sample-utility-1.js @@ -0,0 +1,17 @@ +var SampleUtility = function() { + var value = this.getParameter('value'); + + var ua = new GR('users'); + ua.query(); + + if(!ua.hasNext()){ + ua.initialize(); + ua.setValue('status',value); + ua.insert(); + } + else { + ua.next(); + ua.setValue('status',value); + ua.update(); + } +}