Switch to type tracking for dataflow from 'new db()' to 'db.exec()'

2025-12-16 20:03:04 +01:00 · 2023-11-27 15:04:34 -08:00
parent 5496a1c5ae
commit c1962230c2
5 changed files with 183 additions and 38 deletions
--- a/session/session.ql
+++ b/session/session.ql
@@ -16,11 +16,14 @@ Expr uSource(MethodCallExpr sbts) {
 // Ultimate sink
 // ----------------
 //     db.exec(query);
-Expr uSink(MethodCallExpr exec) {
-  exec.getMethodName() = "exec" and
-  result = exec.getArgument(0)
-}
-
+// Expr uSink(MethodCallExpr exec) {
+//     //   exec.getMethodName() = "exec" and
+//     //   result = exec.getArgument(0)
+//     exists(IdentifyFlowSink cfg, DataFlow::Node source, DataFlow::Node sink |
+//         cfg.hasFlow(source, sink) and
+//         result = sink.asExpr()
+//     )
+// }
 // Flow sink origin
 // ------------------------
 // Connect
@@ -28,49 +31,39 @@ Expr uSink(MethodCallExpr exec) {
 // to its use
 //     db.exec(query);
 //
-class FlowSinkOrigin extends DataFlow::FlowLabel {
-  FlowSinkOrigin() { this = "FlowSinkOrigin" }
+predicate isDBCreate(DataFlow::SourceNode nd) {
+  exists(NewExpr newdb |
+    newdb.getCalleeName() = "Database" and
+    nd.asExpr() = newdb
+  )
 }

-class IdentifyFlowSink extends TaintTracking::Configuration {
-  IdentifyFlowSink() { this = "IdentifyFlowSink" }
+import DataFlow as DF

-  override predicate isSource(DataFlow::Node nd, DataFlow::FlowLabel lbl) {
-    //     const db = new sqlite3.Database(
-    exists(NewExpr newdb |
-      newdb.getCalleeName() = "Database" and
-      nd.asExpr() = newdb and
-      lbl instanceof FlowSinkOrigin
-    )
-  }
-
-  override predicate isSink(DataFlow::Node nd, DataFlow::FlowLabel lbl) {
-    //     db.exec(query);
-    exists(Expr db, MethodCallExpr exec |
-      exec.getMethodName() = "exec" and
-      db = exec.getReceiver() and
-      nd.asExpr() = db and
-      lbl instanceof FlowSinkOrigin
-    )
-  }
+DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
+  t.start() and
+  isDBCreate(result)
+  or
+  exists(DF::TypeTracker t2 | result = myType(t2).track(t2, t))
 }

+DF::SourceNode myType() { result = myType(DF::TypeTracker::end()) }
+
 class UltimateFlowCfg extends TaintTracking::Configuration {
  UltimateFlowCfg() { this = "UltimateFlowCfg" }

  override predicate isSource(DataFlow::Node nd) { nd.asExpr() = uSource(_) }

-  override predicate isSink(DataFlow::Node nd) { nd.asExpr() = uSink(_) }
+  override predicate isSink(DataFlow::Node nd) {
+    exists(Expr db, MethodCallExpr exec |
+      exec.getMethodName() = "exec" and
+      db = exec.getReceiver() and
+      nd.asExpr() = exec.getAnArgument() and
+      db.flow().getALocalSource() = myType()
+    )
+  }
 }

-from
-  UltimateFlowCfg ucfg, DataFlow::PathNode usource, DataFlow::PathNode usink, IdentifyFlowSink cfg,
-  DataFlow::Node source, DataFlow::Node sink
-where
-  cfg.hasFlow(source, sink) and
-  ucfg.hasFlowPath(usource, usink) and
-  exists(MethodCallExpr exec |
-    sink.asExpr() = exec.getReceiver() and
-    usink.getNode().asExpr() = exec.getAnArgument()
-  )
+from UltimateFlowCfg ucfg, DataFlow::PathNode usource, DataFlow::PathNode usink
+where ucfg.hasFlowPath(usource, usink)
 select usink, usource, usink, "Sql injected from $@", usource, "here"
--- a/solutions/RestrictedSqlInjectionViaTT.ql
+++ b/solutions/RestrictedSqlInjectionViaTT.ql
@@ -0,0 +1,69 @@
+/**
+ * @kind path-problem
+ */
+
+import javascript
+import DataFlow::PathGraph
+
+// Ultimate source
+// ----------------
+//     var line = stdinBuffer.toString();
+Expr uSource(MethodCallExpr sbts) {
+  sbts.getMethodName().matches("%toString%") and
+  result = sbts
+}
+
+// Ultimate sink
+// ----------------
+//     db.exec(query);
+// Expr uSink(MethodCallExpr exec) {
+//     //   exec.getMethodName() = "exec" and
+//     //   result = exec.getArgument(0)
+//     exists(IdentifyFlowSink cfg, DataFlow::Node source, DataFlow::Node sink |
+//         cfg.hasFlow(source, sink) and
+//         result = sink.asExpr()
+//     )
+// }
+// Flow sink origin
+// ------------------------
+// Connect
+//     const db = new sqlite3.Database(
+// to its use
+//     db.exec(query);
+//
+predicate isDBCreate(DataFlow::SourceNode nd) {
+  exists(NewExpr newdb |
+    newdb.getCalleeName() = "Database" and
+    nd.asExpr() = newdb
+  )
+}
+
+import DataFlow as DF
+
+DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
+  t.start() and
+  isDBCreate(result)
+  or
+  exists(DF::TypeTracker t2 | result = myType(t2).track(t2, t))
+}
+
+DF::SourceNode myType() { result = myType(DF::TypeTracker::end()) }
+
+class UltimateFlowCfg extends TaintTracking::Configuration {
+  UltimateFlowCfg() { this = "UltimateFlowCfg" }
+
+  override predicate isSource(DataFlow::Node nd) { nd.asExpr() = uSource(_) }
+
+  override predicate isSink(DataFlow::Node nd) {
+    exists(Expr db, MethodCallExpr exec |
+      exec.getMethodName() = "exec" and
+      db = exec.getReceiver() and
+      nd.asExpr() = exec.getAnArgument() and
+      db.flow().getALocalSource() = myType()
+    )
+  }
+}
+
+from UltimateFlowCfg ucfg, DataFlow::PathNode usource, DataFlow::PathNode usink
+where ucfg.hasFlowPath(usource, usink)
+select usink, usource, usink, "Sql injected from $@", usource, "here"
--- a/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.expected
+++ b/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.expected
@@ -0,0 +1,35 @@
+nodes
+| add-user.js:4:9:4:37 | line |
+| add-user.js:4:16:4:37 | stdinBu ... tring() |
+| add-user.js:4:16:4:37 | stdinBu ... tring() |
+| add-user.js:6:5:6:45 | line |
+| add-user.js:6:12:6:15 | line |
+| add-user.js:6:12:6:45 | line.re ... gm, "") |
+| add-user.js:7:12:7:15 | line |
+| add-user.js:31:29:31:32 | info |
+| add-user.js:33:11:33:63 | query |
+| add-user.js:33:19:33:63 | `INSERT ... nfo}")` |
+| add-user.js:33:56:33:59 | info |
+| add-user.js:35:13:35:17 | query |
+| add-user.js:35:13:35:17 | query |
+| add-user.js:41:9:41:30 | info |
+| add-user.js:41:16:41:30 | get_user_info() |
+| add-user.js:44:24:44:27 | info |
+edges
+| add-user.js:4:9:4:37 | line | add-user.js:6:12:6:15 | line |
+| add-user.js:4:16:4:37 | stdinBu ... tring() | add-user.js:4:9:4:37 | line |
+| add-user.js:4:16:4:37 | stdinBu ... tring() | add-user.js:4:9:4:37 | line |
+| add-user.js:6:5:6:45 | line | add-user.js:7:12:7:15 | line |
+| add-user.js:6:12:6:15 | line | add-user.js:6:12:6:45 | line.re ... gm, "") |
+| add-user.js:6:12:6:45 | line.re ... gm, "") | add-user.js:6:5:6:45 | line |
+| add-user.js:7:12:7:15 | line | add-user.js:41:16:41:30 | get_user_info() |
+| add-user.js:31:29:31:32 | info | add-user.js:33:56:33:59 | info |
+| add-user.js:33:11:33:63 | query | add-user.js:35:13:35:17 | query |
+| add-user.js:33:11:33:63 | query | add-user.js:35:13:35:17 | query |
+| add-user.js:33:19:33:63 | `INSERT ... nfo}")` | add-user.js:33:11:33:63 | query |
+| add-user.js:33:56:33:59 | info | add-user.js:33:19:33:63 | `INSERT ... nfo}")` |
+| add-user.js:41:9:41:30 | info | add-user.js:44:24:44:27 | info |
+| add-user.js:41:16:41:30 | get_user_info() | add-user.js:41:9:41:30 | info |
+| add-user.js:44:24:44:27 | info | add-user.js:31:29:31:32 | info |
+#select
+| add-user.js:35:13:35:17 | query | add-user.js:4:16:4:37 | stdinBu ... tring() | add-user.js:35:13:35:17 | query | Sql injected from $@ | add-user.js:4:16:4:37 | stdinBu ... tring() | here |
--- a/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref
+++ b/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref
@@ -0,0 +1 @@
+RestrictedSqlInjectionViaTT.ql
--- a/tests/RestrictedSqlInjectionViaTT/add-user.js
+++ b/tests/RestrictedSqlInjectionViaTT/add-user.js
@@ -0,0 +1,47 @@
+function get_user_info() {
+    var fs = require("fs");
+    var stdinBuffer = fs.readFileSync(process.stdin.fd);
+    var line = stdinBuffer.toString();
+    console.log(line);
+    line = line.replace(/(\r\n|\n|\r)/gm, "");
+    return line
+}
+
+function get_new_id() {
+    return Math.floor(Math.random() * 12345);
+}
+
+function connect_db() {
+    const sqlite3 = require('sqlite3').verbose();
+    const db = new sqlite3.Database(
+        'users.sqlite',
+        sqlite3.OPEN_READWRITE | sqlite3.OPEN_FULLMUTEX, 
+        err => {
+            if (err){
+                console.log(err);
+                throw err;
+            } else {
+                console.log('DB opened');
+            }
+        });
+
+    return db;
+}
+
+function write_info(db, id, info) {
+    db.serialize();
+    const query = `INSERT INTO users VALUES (${id}, "${info}")`;
+    console.log(query);
+    db.exec(query);
+    db.close();
+}
+
+let add_user = () => {
+    console.log("Running add-user");
+    var info = get_user_info();
+    var id = get_new_id();
+    var db = connect_db();
+    write_info(db, id, info);
+}
+
+add_user()