From c1962230c270679a2197790e9867331f227e3bfc Mon Sep 17 00:00:00 2001 From: Michael Hohn Date: Mon, 27 Nov 2023 15:04:34 -0800 Subject: [PATCH] Switch to type tracking for dataflow from 'new db()' to 'db.exec()' --- session/session.ql | 69 +++++++++---------- solutions/RestrictedSqlInjectionViaTT.ql | 69 +++++++++++++++++++ .../RestrictedSqlInjectionViaTT.expected | 35 ++++++++++ .../RestrictedSqlInjectionViaTT.qlref | 1 + tests/RestrictedSqlInjectionViaTT/add-user.js | 47 +++++++++++++ 5 files changed, 183 insertions(+), 38 deletions(-) create mode 100644 solutions/RestrictedSqlInjectionViaTT.ql create mode 100644 tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.expected create mode 100644 tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref create mode 100644 tests/RestrictedSqlInjectionViaTT/add-user.js diff --git a/session/session.ql b/session/session.ql index 04833bf..95f4577 100644 --- 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" diff --git a/solutions/RestrictedSqlInjectionViaTT.ql b/solutions/RestrictedSqlInjectionViaTT.ql new file mode 100644 index 0000000..95f4577 --- /dev/null +++ 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" diff --git a/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.expected b/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.expected new file mode 100644 index 0000000..fb659c1 --- /dev/null +++ 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 | diff --git a/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref b/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref new file mode 100644 index 0000000..aaab83e --- /dev/null +++ b/tests/RestrictedSqlInjectionViaTT/RestrictedSqlInjectionViaTT.qlref @@ -0,0 +1 @@ +RestrictedSqlInjectionViaTT.ql diff --git a/tests/RestrictedSqlInjectionViaTT/add-user.js b/tests/RestrictedSqlInjectionViaTT/add-user.js new file mode 100644 index 0000000..a4c8b1c --- /dev/null +++ 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()