diff --git a/cruft/make-db b/cruft/make-db index 0fa5b58..9cdf681 100755 --- a/cruft/make-db +++ b/cruft/make-db @@ -7,7 +7,7 @@ echo $DB test -d "$DB" && rm -fR "$DB" mkdir -p "$DB" -codeql database create --language=javascript -s . -j 8 -v $DB +codeql database create --language=javascript -s source -j 8 -v $DB # Check it echo "The DB is in $DB" diff --git a/cruft/snapshot-query-1 b/cruft/snapshot-query-1 index 92826cd..d7f3765 100755 --- a/cruft/snapshot-query-1 +++ b/cruft/snapshot-query-1 @@ -26,10 +26,10 @@ git add tests/$qname/$qname.expected git add tests/$qname/$qname.qlref # snapshot the session -cp sample-utility-0.js tests/$qname/ +cp source/sample-utility-0.js tests/$qname/ git add tests/$qname/sample-utility-0.js -cp sample-utility-1.js tests/$qname/ +cp source/sample-utility-1.js tests/$qname/ git add tests/$qname/sample-utility-1.js cp session/session1.ql solutions/$qname.ql diff --git a/sample-utility-1.js b/sample-utility-1.js deleted file mode 100644 index aea3135..0000000 --- a/sample-utility-1.js +++ /dev/null @@ -1,17 +0,0 @@ -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(); - } -} diff --git a/session/session1.ql b/session/session1.ql index 3c4a052..02be83e 100644 --- a/session/session1.ql +++ b/session/session1.ql @@ -72,33 +72,6 @@ predicate sanitizerCheckedSuccessor(ControlFlowNode gr, ControlFlowNode postgr) // 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, diff --git a/solutions/10-NestedIf.ql b/solutions/10-NestedIf.ql new file mode 100644 index 0000000..02be83e --- /dev/null +++ b/solutions/10-NestedIf.ql @@ -0,0 +1,163 @@ +/** + * @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 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/add-user.js b/source/add-user.js similarity index 100% rename from add-user.js rename to source/add-user.js diff --git a/sample-utility-0.js b/source/sample-utility-0.js similarity index 75% rename from sample-utility-0.js rename to source/sample-utility-0.js index 4549868..0029e89 100644 --- a/sample-utility-0.js +++ b/source/sample-utility-0.js @@ -16,12 +16,13 @@ SampleUtility.prototype = Object.extendsObject(Processor, { ua.next(); ua.setValue('status',value); // unsafe ua.update(); + // Nested if() test. + if (ua.safeToWrite()) { + ua.setValue('status', value); // safe + ua.update(); + } } - if (ua.safeToWrite()) { - ua.setValue('status', value); // safe - ua.update(); - } }, diff --git a/source/sample-utility-1.js b/source/sample-utility-1.js new file mode 100644 index 0000000..01b3632 --- /dev/null +++ b/source/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(); + } +} diff --git a/tests/10-NestedIf/10-NestedIf.expected b/tests/10-NestedIf/10-NestedIf.expected new file mode 100644 index 0000000..4cab065 --- /dev/null +++ b/tests/10-NestedIf/10-NestedIf.expected @@ -0,0 +1,29 @@ +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:31:14:35 | value | +| sample-utility-1.js:15:9:15:10 | ua | +| sample-utility-1.js:15:9:15:10 | 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:31:14:35 | 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:31:14:35 | value | sample-utility-1.js:14:31:14:35 | value | +| sample-utility-1.js:14:31:14:35 | value | sample-utility-1.js:15:9:15:10 | ua | +| sample-utility-1.js:14:31:14:35 | value | sample-utility-1.js:15:9:15:10 | 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:9:15:10 | ua | sample-utility-1.js:2:17:2:42 | this.ge ... value') | sample-utility-1.js:15:9:15:10 | ua | Data flow from $@ to $@. | sample-utility-1.js:2:17:2:42 | this.ge ... value') | this.ge ... value') | sample-utility-1.js:15:9:15:10 | ua | ua | diff --git a/tests/10-NestedIf/10-NestedIf.qlref b/tests/10-NestedIf/10-NestedIf.qlref new file mode 100644 index 0000000..3c1ae37 --- /dev/null +++ b/tests/10-NestedIf/10-NestedIf.qlref @@ -0,0 +1 @@ +10-NestedIf.ql diff --git a/tests/10-NestedIf/sample-utility-0.js b/tests/10-NestedIf/sample-utility-0.js new file mode 100644 index 0000000..0029e89 --- /dev/null +++ b/tests/10-NestedIf/sample-utility-0.js @@ -0,0 +1,30 @@ +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(); + // Nested if() test. + if (ua.safeToWrite()) { + ua.setValue('status', value); // safe + ua.update(); + } + } + + + }, + + type: 'SampleUtility' +}); diff --git a/tests/10-NestedIf/sample-utility-1.js b/tests/10-NestedIf/sample-utility-1.js new file mode 100644 index 0000000..01b3632 --- /dev/null +++ b/tests/10-NestedIf/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(); + } +}