diff --git a/solutions/GuardedSafeToWrite.ql b/solutions/GuardedSafeToWrite.ql new file mode 100644 index 0000000..3c4a052 --- /dev/null +++ b/solutions/GuardedSafeToWrite.ql @@ -0,0 +1,190 @@ +/** + * @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 + ) +} + +// Def-Use special handling: +// - Include sanitizer check when flagging successive object member calls in taint +// step. +// - Stop at +// ua.safeToWrite() +predicate sanitizerCheckedSuccessor(ControlFlowNode gr, ControlFlowNode postgr) { + gr.getASuccessor() = postgr and + not inSafeToWrite(postgr) + or + exists(ControlFlowNode p | + sanitizerCheckedSuccessor(gr, p) and + not gr.getASuccessor() = postgr 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. +} + +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) + ) +} + +predicate foo2(Expr gr, Expr postgr) { + exists(DotExpr temp, MethodCallExpr mce | + temp.getPropertyName() = "setValue" and + mce.getReceiver() = temp.getBase() and + gr = mce.getReceiver() and + sanitizerCheckedSuccessor(gr, postgr) + ) +} + +predicate inSafeToWrite(ControlFlowNode p) { + exists( + // DotExpr temp, MethodCallExpr mce, + IfStmt if_ + | + // XX: + if_.getAChild+().getFirstControlFlowNode() = p + // and + // temp.getPropertyName() = "safeToWrite" and + // p = mce.getReceiver() and + // p = temp.getBase() + ) +} + +// Preparation for including a 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 isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { + nd instanceof CanWriteGuard + } + + 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() + ) + } +} + +class CanWriteGuard extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode { + CanWriteGuard() { this.getCalleeName() = "safeToWrite" } + + override predicate sanitizes(boolean outcome, Expr e) { + // outcome is the result of the conditional (the true or false branch) + outcome = true and + e = this.getReceiver().asExpr() + // or e.getASuccessor+() = this.getReceiver().asExpr() + } +} + +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/GuardedSafeToWrite/GuardedSafeToWrite.expected b/tests/GuardedSafeToWrite/GuardedSafeToWrite.expected new file mode 100644 index 0000000..e9660f6 --- /dev/null +++ b/tests/GuardedSafeToWrite/GuardedSafeToWrite.expected @@ -0,0 +1,32 @@ +WARNING: Unused predicate foo (/Users/hohn/local/codeql-javascript-multiflow/solutions/GuardedSafeToWrite.ql:75,11-14) +WARNING: Unused predicate foo1 (/Users/hohn/local/codeql-javascript-multiflow/solutions/GuardedSafeToWrite.ql:84,11-15) +WARNING: Unused predicate foo2 (/Users/hohn/local/codeql-javascript-multiflow/solutions/GuardedSafeToWrite.ql:93,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: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-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:17:34:17:38 | 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: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-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-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/GuardedSafeToWrite/GuardedSafeToWrite.qlref b/tests/GuardedSafeToWrite/GuardedSafeToWrite.qlref new file mode 100644 index 0000000..615ea1b --- /dev/null +++ b/tests/GuardedSafeToWrite/GuardedSafeToWrite.qlref @@ -0,0 +1 @@ +GuardedSafeToWrite.ql diff --git a/tests/GuardedSafeToWrite/sample-utility-0.js b/tests/GuardedSafeToWrite/sample-utility-0.js new file mode 100644 index 0000000..4549868 --- /dev/null +++ b/tests/GuardedSafeToWrite/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/GuardedSafeToWrite/sample-utility-1.js b/tests/GuardedSafeToWrite/sample-utility-1.js new file mode 100644 index 0000000..aea3135 --- /dev/null +++ b/tests/GuardedSafeToWrite/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(); + } +}