From 156b94e436f7175ce6cabe3e134fd8896ab97c3c Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 27 Jul 2018 16:07:28 +0100 Subject: [PATCH] JavaScript: Add model of JSON parsers --- javascript/ql/src/javascript.qll | 1 + .../ql/src/semmle/javascript/JsonParsers.qll | 80 +++++++++++++++++++ .../src/semmle/javascript/StandardLibrary.qll | 3 + .../javascript/dataflow/TaintTracking.qll | 25 ++++-- .../security/dataflow/NosqlInjection.qll | 30 ------- .../JsonParsers/JsonParserCalls.expected | 9 +++ .../JsonParsers/JsonParserCalls.ql | 17 ++++ .../library-tests/JsonParsers/package.json | 11 +++ .../ql/test/library-tests/JsonParsers/tst.js | 21 +++++ .../Security/CWE-089/SqlInjection.expected | 1 + .../Security/CWE-089/mongooseJsonParse.js | 25 ++++++ 11 files changed, 186 insertions(+), 37 deletions(-) create mode 100644 javascript/ql/src/semmle/javascript/JsonParsers.qll create mode 100644 javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected create mode 100644 javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.ql create mode 100644 javascript/ql/test/library-tests/JsonParsers/package.json create mode 100644 javascript/ql/test/library-tests/JsonParsers/tst.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-089/mongooseJsonParse.js diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll index e073d14aa6c..ec52b6d2890 100644 --- a/javascript/ql/src/javascript.qll +++ b/javascript/ql/src/javascript.qll @@ -23,6 +23,7 @@ import semmle.javascript.Functions import semmle.javascript.HTML import semmle.javascript.JSDoc import semmle.javascript.JSON +import semmle.javascript.JsonParsers import semmle.javascript.JSX import semmle.javascript.Lines import semmle.javascript.Locations diff --git a/javascript/ql/src/semmle/javascript/JsonParsers.qll b/javascript/ql/src/semmle/javascript/JsonParsers.qll new file mode 100644 index 00000000000..760f8b9e468 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/JsonParsers.qll @@ -0,0 +1,80 @@ +/** + * Provides classes for working with JSON parsers. + */ +import javascript + +/** + * A call to a JSON parser such as `JSON.parse`. + */ +abstract class JsonParserCall extends DataFlow::CallNode { + /** + * Gets the data flow node holding the input string to be parsed. + */ + abstract DataFlow::Node getInput(); + + /** + * Gets the data flow node holding the resulting JSON object. + */ + abstract DataFlow::SourceNode getOutput(); +} + +/** + * A JSON parser that returns its result. + */ +private class PlainJsonParserCall extends JsonParserCall { + PlainJsonParserCall() { + exists (DataFlow::SourceNode callee | this = callee.getACall() | + callee = DataFlow::globalVarRef("JSON").getAPropertyRead("parse") or + callee = DataFlow::moduleImport("parse-json") or + callee = DataFlow::moduleImport("json-parse-better-errors") or + callee = DataFlow::moduleImport("json-safe-parse")) + } + + override DataFlow::Node getInput() { + result = getArgument(0) + } + + override DataFlow::SourceNode getOutput() { + result = this + } +} + +/** + * A JSON parser that returns its result wrapped in a another object. + */ +private class JsonParserCallWithWrapper extends JsonParserCall { + string outputPropName; + + JsonParserCallWithWrapper() { + exists (DataFlow::SourceNode callee | this = callee.getACall() | + callee = DataFlow::moduleImport("safe-json-parse/tuple") and outputPropName = "1" or + callee = DataFlow::moduleImport("safe-json-parse/result") and outputPropName = "v" or + callee = DataFlow::moduleImport("fast-json-parse") and outputPropName = "value" or + callee = DataFlow::moduleImport("json-parse-safe") and outputPropName = "value") + } + + override DataFlow::Node getInput() { + result = getArgument(0) + } + + override DataFlow::SourceNode getOutput() { + result = this.getAPropertyRead(outputPropName) + } +} + +/** + * A JSON parser that returns its result through a callback argument. + */ +private class JsonParserCallWithCallback extends JsonParserCall { + JsonParserCallWithCallback() { + this = DataFlow::moduleImport("safe-json-parse/callback").getACall() + } + + override DataFlow::Node getInput() { + result = getArgument(0) + } + + override DataFlow::SourceNode getOutput() { + result = getCallback(1).getParameter(1) + } +} \ No newline at end of file diff --git a/javascript/ql/src/semmle/javascript/StandardLibrary.qll b/javascript/ql/src/semmle/javascript/StandardLibrary.qll index 2ceda6377a7..0609aa82dca 100644 --- a/javascript/ql/src/semmle/javascript/StandardLibrary.qll +++ b/javascript/ql/src/semmle/javascript/StandardLibrary.qll @@ -44,8 +44,11 @@ class DirectEval extends CallExpr { } /** + * DEPRECATED. Use `JsonParserCall` and the data flow API instead. + * * A call to `JSON.parse`. */ +deprecated class JsonParseCall extends MethodCallExpr { JsonParseCall() { this = DataFlow::globalVarRef("JSON").getAMemberCall("parse").asExpr() diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index a68b2ad052f..ea8fc1b4654 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -400,14 +400,11 @@ module TaintTracking { } /** - * A taint propagating data flow edge arising from JSON parsing or unparsing. + * A taint propagating data flow edge arising from JSON unparsing. */ - private class JsonManipulationTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode { - JsonManipulationTaintStep() { - exists (string methodName | - methodName = "parse" or methodName = "stringify" | - this = DataFlow::globalVarRef("JSON").getAMemberCall(methodName) - ) + private class JsonStringifyTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode { + JsonStringifyTaintStep() { + this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") } override predicate step(DataFlow::Node pred, DataFlow::Node succ) { @@ -415,6 +412,20 @@ module TaintTracking { } } + /** + * A taint propagating data flow edge arising from JSON parsing. + */ + private class JsonParserTaintStep extends AdditionalTaintStep, DataFlow::CallNode { + JsonParserCall call; + + JsonParserTaintStep() { this = call } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + pred = call.getInput() and + succ = call.getOutput() + } + } + /** * Holds if `params` is a `URLSearchParams` object providing access to * the parameters encoded in `input`. diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll index 7fb6bfdb098..9c031728d7e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll @@ -55,36 +55,6 @@ module NosqlInjection { RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } } - /** - * A taint tracking configuration for tracking user input that flows - * into a call to `JSON.parse`. - */ - private class RemoteJsonTrackingConfig extends TaintTracking::Configuration { - RemoteJsonTrackingConfig() { - this = "RemoteJsonTrackingConfig" - } - - override predicate isSource(DataFlow::Node nd) { - nd instanceof RemoteFlowSource - } - - override predicate isSink(DataFlow::Node nd) { - nd.asExpr() = any(JsonParseCall c).getArgument(0) - } - } - - /** - * A call to `JSON.parse` where the argument is user-provided. - */ - class RemoteJson extends Source, DataFlow::ValueNode { - RemoteJson() { - exists (DataFlow::Node parsedArg | - parsedArg.asExpr() = astNode.(JsonParseCall).getArgument(0) and - any(RemoteJsonTrackingConfig cfg).hasFlow(_, parsedArg) - ) - } - } - /** An expression interpreted as a NoSQL query, viewed as a sink. */ class NosqlQuerySink extends Sink, DataFlow::ValueNode { override NoSQL::Query astNode; diff --git a/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected b/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected new file mode 100644 index 00000000000..53c6352978f --- /dev/null +++ b/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected @@ -0,0 +1,9 @@ +| tst.js:11:1:11:28 | checkJS ... input)) | OK | +| tst.js:12:1:12:39 | checkJS ... input)) | OK | +| tst.js:13:1:13:53 | checkJS ... input)) | OK | +| tst.js:14:1:14:44 | checkJS ... input)) | OK | +| tst.js:16:1:16:53 | checkJS ... ut)[1]) | OK | +| tst.js:17:1:17:53 | checkJS ... put).v) | OK | +| tst.js:18:1:18:50 | checkJS ... .value) | OK | +| tst.js:19:1:19:50 | checkJS ... .value) | OK | +| tst.js:21:61:21:77 | checkJSON(result) | OK | diff --git a/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.ql b/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.ql new file mode 100644 index 00000000000..a0da55af6ea --- /dev/null +++ b/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.ql @@ -0,0 +1,17 @@ +import javascript + +class Assertion extends DataFlow::CallNode { + Assertion() { + getCalleeName() = "checkJSON" + } + + string getMessage() { + if not any(JsonParserCall call).getOutput().flowsTo(getArgument(0)) then + result = "Should be JSON parser" + else + result = "OK" + } +} + +from Assertion assertion +select assertion, assertion.getMessage() diff --git a/javascript/ql/test/library-tests/JsonParsers/package.json b/javascript/ql/test/library-tests/JsonParsers/package.json new file mode 100644 index 00000000000..2dd6d3b3e64 --- /dev/null +++ b/javascript/ql/test/library-tests/JsonParsers/package.json @@ -0,0 +1,11 @@ +{ + "private": true, + "dependencies": { + "fast-json-parse": "^1.0.3", + "json-parse-better-errors": "^1.0.2", + "json-parse-safe": "^1.0.5", + "json-safe-parse": "^0.0.2", + "parse-json": "^4.0.0", + "safe-json-parse": "^4.0.0" + } +} diff --git a/javascript/ql/test/library-tests/JsonParsers/tst.js b/javascript/ql/test/library-tests/JsonParsers/tst.js new file mode 100644 index 00000000000..2e342efb31f --- /dev/null +++ b/javascript/ql/test/library-tests/JsonParsers/tst.js @@ -0,0 +1,21 @@ +let fs = require('fs'); // mark as node.js module + +function checkJSON(value) { + if (value !== 'JSON string') { + throw new Error('Not the JSON output: ' + value); + } +} + +let input = '"JSON string"'; + +checkJSON(JSON.parse(input)); +checkJSON(require('parse-json')(input)); +checkJSON(require('json-parse-better-errors')(input)); +checkJSON(require('json-safe-parse')(input)); + +checkJSON(require('safe-json-parse/tuple')(input)[1]); +checkJSON(require('safe-json-parse/result')(input).v); +checkJSON(require('fast-json-parse')(input).value); +checkJSON(require('json-parse-safe')(input).value); + +require('safe-json-parse/callback')(input, (err, result) => checkJSON(result)); diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected index fcff7825d71..55a095634ae 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected @@ -1,6 +1,7 @@ | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value | | mongodb.js:39:16:39:20 | query | This query depends on $@. | mongodb.js:34:19:34:33 | req.query.title | a user-provided value | | mongoose.js:24:19:24:23 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value | | tst2.js:9:27:9:84 | "select ... d + "'" | This query depends on $@. | tst2.js:9:66:9:78 | req.params.id | a user-provided value | | tst3.js:10:14:10:19 | query1 | This query depends on $@. | tst3.js:9:16:9:34 | req.params.category | a user-provided value | | tst4.js:8:10:8:66 | 'SELECT ... d + '"' | This query depends on $@. | tst4.js:8:46:8:60 | $routeParams.id | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongooseJsonParse.js b/javascript/ql/test/query-tests/Security/CWE-089/mongooseJsonParse.js new file mode 100644 index 00000000000..a340bf2d968 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-089/mongooseJsonParse.js @@ -0,0 +1,25 @@ +'use strict'; +const Express = require('express'); +const BodyParser = require('body-parser'); +const Mongoose = require('mongoose'); +Mongoose.Promise = global.Promise; +Mongoose.connect('mongodb://localhost/injectable1'); + +const app = Express(); + +const Document = Mongoose.model('Document', { + title: { + type: String, + unique: true + }, + type: String +}); + +app.get('/documents/find', (req, res) => { + const query = {}; + query.title = JSON.parse(req.query.data).title; + + // NOT OK: query is tainted by user-provided object value + Document.find(query); +}); +