From d5097d820d23bd6fb52f3ee77424e70bcca26c03 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 10:02:59 +0200 Subject: [PATCH 1/6] support direct callbacks to require("net").createServer --- .../ql/src/semmle/javascript/frameworks/NodeJSLib.qll | 8 ++++++-- .../frameworks/NodeJSLib/createServer.js | 11 +++++++++++ .../library-tests/frameworks/NodeJSLib/tests.expected | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 53a5f18c0d4..06a530a622c 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -1068,8 +1068,10 @@ module NodeJSLib { /** * An instance of net.createServer(), which creates a new TCP/IPC server. */ - private class NodeJSNetServer extends DataFlow::SourceNode { - NodeJSNetServer() { this = DataFlow::moduleMember("net", "createServer").getAnInvocation() } + class NodeJSNetServer extends DataFlow::InvokeNode { + NodeJSNetServer() { + this = DataFlow::moduleMember(["net", "tls"], "createServer").getAnInvocation() + } private DataFlow::SourceNode ref(DataFlow::TypeTracker t) { t.start() and result = this @@ -1096,6 +1098,8 @@ module NodeJSLib { | this = call.getCallback(1).getParameter(0) ) + or + this = server.getCallback([0, 1]).getParameter(0) } DataFlow::SourceNode ref() { result = EventEmitter::trackEventEmitter(this) } diff --git a/javascript/ql/test/library-tests/frameworks/NodeJSLib/createServer.js b/javascript/ql/test/library-tests/frameworks/NodeJSLib/createServer.js index 5d5e1df1705..11b312203e4 100644 --- a/javascript/ql/test/library-tests/frameworks/NodeJSLib/createServer.js +++ b/javascript/ql/test/library-tests/frameworks/NodeJSLib/createServer.js @@ -2,3 +2,14 @@ var https = require('https'); https.createServer(function (req, res) {}); https.createServer(o, function (req, res) {}); require('http2').createServer((req, res) => {}); + +require("tls").createServer((socket) => { + socket.on("data", (data) => {}) +}); + +const net = require('net'); +const tls = require('tls'); + +const server = (isSecure ? tls : net).createServer(options, (socket) => { + socket.on("data", (data) => {}) +}); \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected b/javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected index 6c2a02011fa..3e678ba1093 100644 --- a/javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected +++ b/javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected @@ -163,6 +163,8 @@ test_ClientRequest_getADataNode | src/http.js:27:16:27:73 | http.re ... POST'}) | src/http.js:50:16:50:22 | 'stuff' | | src/http.js:27:16:27:73 | http.re ... POST'}) | src/http.js:51:14:51:25 | 'more stuff' | test_RemoteFlowSources +| createServer.js:7:24:7:27 | data | +| createServer.js:14:24:14:27 | data | | src/http.js:6:26:6:32 | req.url | | src/http.js:8:3:8:20 | req.headers.cookie | | src/http.js:9:3:9:17 | req.headers.foo | From bb97829e1d5ff436939abce2261c489736a389ef Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 15:06:35 +0200 Subject: [PATCH 2/6] add a model for the ClientRequest `new require("net").Socket()` --- .../javascript/frameworks/ClientRequests.qll | 47 +++++++++++++++++++ .../ClientRequests/ClientRequests.expected | 5 ++ .../frameworks/ClientRequests/tst.js | 13 +++++ 3 files changed, 65 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll index 2e0908ca4e7..1c0ac8d6a16 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll @@ -389,6 +389,53 @@ module ClientRequest { } } + /** + * Gets an instantiation `socket` of `require("net").Socket` type tracked using `t`. + */ + private DataFlow::SourceNode netSocketInstantiation( + DataFlow::TypeTracker t, DataFlow::NewNode socket + ) { + t.start() and + socket = DataFlow::moduleMember("net", "Socket").getAnInstantiation() and + result = socket + or + exists(DataFlow::TypeTracker t2 | result = netSocketInstantiation(t2, socket).track(t2, t)) + } + + /** + * A model of a request made using `(new require("net").Socket()).connect(args);`. + */ + class NetSocketRequest extends ClientRequest::Range { + DataFlow::NewNode socket; + + NetSocketRequest() { + this = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMethodCall("connect") + } + + override DataFlow::Node getUrl() { + result = getArgument([0, 1]) // there are multiple overrides of `connect`, and the URL can be in the first or second argument. + } + + override DataFlow::Node getHost() { result = getOptionArgument(0, "host") } + + override DataFlow::Node getAResponseDataNode(string responseType, boolean promise) { + responseType = "text" and + promise = false and + exists(DataFlow::CallNode call | + call = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMemberCall("on") and + call.getArgument(0).mayHaveStringValue("data") and + result = call.getABoundCallbackParameter(1, 0) + ) + } + + override DataFlow::Node getADataNode() { + exists(DataFlow::CallNode call | + call = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMemberCall("write") and + result = call.getArgument(0) + ) + } + } + /** * A model of a URL request made using the `superagent` library. */ diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequests.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequests.expected index 061bc6baece..1c91826ac8d 100644 --- a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequests.expected +++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequests.expected @@ -66,6 +66,7 @@ test_ClientRequest | tst.js:200:2:200:21 | $.get("example.php") | | tst.js:202:5:208:7 | $.ajax( ... }}) | | tst.js:210:2:210:21 | $.get("example.php") | +| tst.js:219:5:219:41 | data.so ... Host"}) | test_getADataNode | tst.js:53:5:53:23 | axios({data: data}) | tst.js:53:18:53:21 | data | | tst.js:57:5:57:39 | axios.p ... data2}) | tst.js:57:19:57:23 | data1 | @@ -95,11 +96,13 @@ test_getADataNode | tst.js:179:2:179:60 | $.getJS ... a ) {}) | tst.js:179:31:179:38 | "MyData" | | tst.js:183:2:183:60 | $.post( ... ) { }) | tst.js:183:28:183:37 | "PostData" | | tst.js:187:2:193:3 | $.ajax( ... on"\\n\\t}) | tst.js:190:11:190:20 | "AjaxData" | +| tst.js:219:5:219:41 | data.so ... Host"}) | tst.js:223:23:223:30 | "foobar" | test_getHost | tst.js:87:5:87:39 | http.ge ... host}) | tst.js:87:34:87:37 | host | | tst.js:89:5:89:23 | axios({host: host}) | tst.js:89:18:89:21 | host | | tst.js:91:5:91:34 | got(rel ... host}) | tst.js:91:29:91:32 | host | | tst.js:93:5:93:35 | net.req ... host }) | tst.js:93:29:93:32 | host | +| tst.js:219:5:219:41 | data.so ... Host"}) | tst.js:219:32:219:39 | "myHost" | test_getUrl | tst.js:11:5:11:16 | request(url) | tst.js:11:13:11:15 | url | | tst.js:13:5:13:20 | request.get(url) | tst.js:13:17:13:19 | url | @@ -173,6 +176,7 @@ test_getUrl | tst.js:200:2:200:21 | $.get("example.php") | tst.js:200:8:200:20 | "example.php" | | tst.js:202:5:208:7 | $.ajax( ... }}) | tst.js:203:10:203:22 | "example.php" | | tst.js:210:2:210:21 | $.get("example.php") | tst.js:210:8:210:20 | "example.php" | +| tst.js:219:5:219:41 | data.so ... Host"}) | tst.js:219:25:219:40 | {host: "myHost"} | test_getAResponseDataNode | tst.js:19:5:19:23 | requestPromise(url) | tst.js:19:5:19:23 | requestPromise(url) | text | true | | tst.js:21:5:21:23 | superagent.get(url) | tst.js:21:5:21:23 | superagent.get(url) | stream | true | @@ -233,3 +237,4 @@ test_getAResponseDataNode | tst.js:200:2:200:21 | $.get("example.php") | tst.js:200:37:200:44 | response | | false | | tst.js:202:5:208:7 | $.ajax( ... }}) | tst.js:207:21:207:36 | err.responseText | json | false | | tst.js:210:2:210:21 | $.get("example.php") | tst.js:210:55:210:70 | xhr.responseText | | false | +| tst.js:219:5:219:41 | data.so ... Host"}) | tst.js:221:29:221:32 | data | text | false | diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js index 2da17fa99bb..e828423f91c 100644 --- a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js +++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js @@ -209,3 +209,16 @@ import {ClientRequest, net} from 'electron'; $.get("example.php").fail(function(xhr) {console.log(xhr.responseText)}); }); + +const net = require("net"); +(function () { + var data = { + socket: new net.Socket() + } + + data.socket.connect({host: "myHost"}); + + data.socket.on("data", (data) => {}); + + data.socket.write("foobar"); +})(); \ No newline at end of file From b97c09a3197550b3eda390b424cb07702a6f27df Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 15:09:27 +0200 Subject: [PATCH 3/6] use tuples to simplify arrayFunctionTaintStep --- .../ql/src/semmle/javascript/Arrays.qll | 47 +++++-------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/Arrays.qll b/javascript/ql/src/semmle/javascript/Arrays.qll index 247f5cdf1ce..5bbcc00a0c1 100644 --- a/javascript/ql/src/semmle/javascript/Arrays.qll +++ b/javascript/ql/src/semmle/javascript/Arrays.qll @@ -24,13 +24,11 @@ module ArrayTaintTracking { predicate arrayFunctionTaintStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode call) { // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are // `elt` and `ary`; similar for `forEach` - exists(string name, Function f, int i | - (name = "map" or name = "forEach") and - (i = 0 or i = 2) and + exists(Function f | call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and - call.(DataFlow::MethodCallNode).getMethodName() = name and + call.(DataFlow::MethodCallNode).getMethodName() = ["map", "forEach"] and pred = call.getReceiver() and - succ = DataFlow::parameterNode(f.getParameter(i)) + succ = DataFlow::parameterNode(f.getParameter([0, 2])) ) or // `array.map` with tainted return value in callback @@ -47,41 +45,22 @@ module ArrayTaintTracking { succ = call or // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`. - exists(string name | - name = "push" or - name = "unshift" - | - pred = call.getAnArgument() and - succ.(DataFlow::SourceNode).getAMethodCall(name) = call - ) + pred = call.getAnArgument() and + succ.(DataFlow::SourceNode).getAMethodCall(["push", "unshift"]) = call or // `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`. - exists(string name | - name = "push" or - name = "unshift" - | - pred = call.getASpreadArgument() and - // Make sure we handle reflective calls - succ = call.getReceiver().getALocalSource() and - call.getCalleeName() = name - ) + pred = call.getASpreadArgument() and + // Make sure we handle reflective calls + succ = call.getReceiver().getALocalSource() and + call.getCalleeName() = ["push", "unshift"] or // `array.splice(i, del, e)`: if `e` is tainted, then so is `array`. - exists(string name | name = "splice" | - pred = call.getArgument(2) and - succ.(DataFlow::SourceNode).getAMethodCall(name) = call - ) + pred = call.getArgument(2) and + succ.(DataFlow::SourceNode).getAMethodCall("splice") = call or // `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`. - exists(string name | - name = "pop" or - name = "shift" or - name = "slice" or - name = "splice" - | - call.(DataFlow::MethodCallNode).calls(pred, name) and - succ = call - ) + call.(DataFlow::MethodCallNode).calls(pred, ["pop", "shift", "slice", "splice"]) and + succ = call or // `e = Array.from(x)`: if `x` is tainted, then so is `e`. call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and From eb80705e99c8e519cb71b967ad4c85d2680748bf Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 15:45:42 +0200 Subject: [PATCH 4/6] add a taint-step for `require("bluebird").mapSeries()` --- javascript/ql/src/semmle/javascript/Promises.qll | 12 ++++++++++++ javascript/ql/test/library-tests/Promises/flow.js | 13 ++++++++++++- .../ql/test/library-tests/Promises/tests.expected | 2 ++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll index a8fb3dc1fc3..a8231d75dce 100644 --- a/javascript/ql/src/semmle/javascript/Promises.qll +++ b/javascript/ql/src/semmle/javascript/Promises.qll @@ -440,6 +440,18 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) { pred.getEnclosingExpr() = await.getOperand() and succ.getEnclosingExpr() = await ) + or + exists(DataFlow::CallNode mapSeries | + mapSeries = DataFlow::moduleMember("bluebird", "mapSeries").getACall() + | + // from `xs` to `x` in `require("bluebird").mapSeries(xs, (x) => {...})`. + pred = mapSeries.getArgument(0) and + succ = mapSeries.getABoundCallbackParameter(1, 0) + or + // from `y` to `require("bluebird").mapSeries(x, x => y)`. + pred = mapSeries.getCallback(1).getAReturn() and + succ = mapSeries + ) } /** diff --git a/javascript/ql/test/library-tests/Promises/flow.js b/javascript/ql/test/library-tests/Promises/flow.js index ff8040f6e9d..81af660561a 100644 --- a/javascript/ql/test/library-tests/Promises/flow.js +++ b/javascript/ql/test/library-tests/Promises/flow.js @@ -154,4 +154,15 @@ } catch (e) { sink(e); // NOT OK } - })(); \ No newline at end of file +})(); + +(function () { + var source = "source"; + + var bluebird = require("bluebird"); + + bluebird.mapSeries(source, x => sink(x)); // NOT OK (for taint-tracking configs) + + const foo = bluebird.mapSeries(source, x => x); + sink(foo); // NOT OK (for taint-tracking configs) +}) \ No newline at end of file diff --git a/javascript/ql/test/library-tests/Promises/tests.expected b/javascript/ql/test/library-tests/Promises/tests.expected index 69e3fd093bc..21c685e0c40 100644 --- a/javascript/ql/test/library-tests/Promises/tests.expected +++ b/javascript/ql/test/library-tests/Promises/tests.expected @@ -237,6 +237,8 @@ flow exclusiveTaintFlow | flow2.js:2:15:2:22 | "source" | flow2.js:20:7:20:14 | tainted3 | | flow.js:136:15:136:22 | "source" | flow.js:141:7:141:13 | async() | +| flow.js:160:15:160:22 | "source" | flow.js:164:39:164:39 | x | +| flow.js:160:15:160:22 | "source" | flow.js:167:7:167:9 | foo | | interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error | typetrack | flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | copy $PromiseResolveField$ | From cffe573d0610a143cac9ebc62355fee0f0e4b16b Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 21:30:41 +0200 Subject: [PATCH 5/6] add taint-steps for underscore methods --- .../frameworks/LodashUnderscore.qll | 46 +++++++++++++++++++ .../TaintTracking.expected | 6 +++ .../InterProceduralFlow/underscore.js | 21 +++++++++ 3 files changed, 73 insertions(+) create mode 100644 javascript/ql/test/library-tests/InterProceduralFlow/underscore.js diff --git a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll index 5461231365f..3e2b3b777b0 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll @@ -394,6 +394,52 @@ module LodashUnderscore { succ = this.getExceptionalReturn() } } + + /** + * Holds if there is a taint-step involving a (non-function) underscore method from `pred` to `succ`. + */ + private predicate underscoreTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(string name, DataFlow::CallNode call | + call = any(Member member | member.getName() = name).getACall() + | + name = + ["find", "filter", "findWhere", "where", "reject", "pluck", "max", "min", "sortBy", + "shuffle", "sample", "toArray", "partition", "compact", "first", "initial", "last", + "rest", "flatten", "without", "difference", "uniq", "unique", "unzip", "transpose", + "object", "chunk", "values", "mapObject", "pick", "omit", "defaults", "clone", "tap", + "identity"] and + pred = call.getArgument(0) and + succ = call + or + name = ["union", "zip"] and + pred = call.getAnArgument() and + succ = call + or + name = + ["each", "map", "every", "some", "max", "min", "sortBy", "partition", "mapObject", "tap"] and + pred = call.getArgument(0) and + succ = call.getABoundCallbackParameter(1, 0) + or + name = ["reduce", "reduceRight"] and + pred = call.getArgument(0) and + succ = call.getABoundCallbackParameter(1, 1) + or + name = ["map", "reduce", "reduceRight"] and + pred = call.getCallback(1).getAReturn() and + succ = call + ) + } + + /** + * A model for taint-steps involving (non-function) underscore methods. + */ + private class UnderscoreTaintStep extends TaintTracking::AdditionalTaintStep { + UnderscoreTaintStep() { underscoreTaintStep(this, _) } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + underscoreTaintStep(pred, succ) and pred = this + } + } } /** diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected index eeac69e3af1..c3ea1cdb5f2 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected @@ -86,3 +86,9 @@ | tst.js:2:17:2:22 | "src1" | tst.js:61:16:61:18 | o.r | | tst.js:2:17:2:22 | "src1" | tst.js:68:16:68:22 | inner() | | tst.js:2:17:2:22 | "src1" | tst.js:80:16:80:22 | outer() | +| underscore.js:2:17:2:22 | "src1" | underscore.js:3:15:3:28 | _.max(source1) | +| underscore.js:5:17:5:22 | "src2" | underscore.js:6:15:6:34 | _.union([], source2) | +| underscore.js:5:17:5:22 | "src2" | underscore.js:7:15:7:32 | _.zip(source2, []) | +| underscore.js:9:17:9:22 | "src3" | underscore.js:11:17:11:17 | x | +| underscore.js:14:17:14:22 | "src4" | underscore.js:16:17:16:17 | e | +| underscore.js:19:17:19:22 | "src5" | underscore.js:20:15:20:44 | _.map([ ... ource5) | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/underscore.js b/javascript/ql/test/library-tests/InterProceduralFlow/underscore.js new file mode 100644 index 00000000000..858099d28a9 --- /dev/null +++ b/javascript/ql/test/library-tests/InterProceduralFlow/underscore.js @@ -0,0 +1,21 @@ +(function() { + var source1 = "src1"; + var sink1 = _.max(source1); // NOT OK + + var source2 = "src2"; + var sink2 = _.union([], source2); // NOT OK + var sink3 = _.zip(source2, []); // NOT OK + + var source3 = "src3"; + _.map(source3, (x) => { + let sink4 = x; // NOT OK + }); + + var source4 = "src4"; + _.reduce(source4, (acc, e) => { + let sink5 = e; // NOT OK + }); + + var source5 = "src5"; + var sink6 = _.map([1,2,3], (x) => source5); // NOT OK +})(); From 88bbc2f1f455c924b3789e9d0868ca20154dcb11 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 8 Sep 2020 21:35:19 +0200 Subject: [PATCH 6/6] add change note --- change-notes/1.26/analysis-javascript.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 09aba390abb..383c22c5a41 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -3,6 +3,7 @@ ## General improvements * Support for the following frameworks and libraries has been improved: + - [bluebird](https://www.npmjs.com/package/bluebird) - [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify) - [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify) - [javascript-stringify](https://www.npmjs.com/package/javascript-stringify) @@ -10,9 +11,11 @@ - [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify) - [json-stringify-safe](https://www.npmjs.com/package/json-stringify-safe) - [json3](https://www.npmjs.com/package/json3) + - [lodash](https://www.npmjs.com/package/lodash) - [object-inspect](https://www.npmjs.com/package/object-inspect) - [pretty-format](https://www.npmjs.com/package/pretty-format) - [stringify-object](https://www.npmjs.com/package/stringify-object) + - [underscore](https://www.npmjs.com/package/underscore) * Analyzing files with the ".cjs" extension is now supported.