Add nested if() test. Update source locations.

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"

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

sample-utility-1.js

@@ -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();
-    }
-}

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,

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()

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();
-        }        
 
     },
     

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();
+    }
+}

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 |

tests/10-NestedIf/10-NestedIf.qlref

@@ -0,0 +1 @@
+10-NestedIf.ql

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'
+});

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();
+    }
+}