Merge pull request #10 from asger-semmle/json-parsers

JavaScript: Add model of JSON parsers

change-notes/1.18/analysis-javascript.md

@@ -14,10 +14,16 @@
   - [crypto-js](https://github.com/https://github.com/brix/crypto-js)
   - [express-jwt](https://github.com/auth0/express-jwt)
   - [express-session](https://github.com/expressjs/session)
+  - [fast-json-parse](https://github.com/mcollina/fast-json-parse)
   - [forge](https://github.com/digitalbazaar/forge)
+  - [json-parse-better-errors](https://github.com/zkat/json-parse-better-errors)
+  - [json-parse-safe](https://github.com/joaquimserafim/json-parse-safe)
+  - [json-safe-parse](https://github.com/bahamas10/node-json-safe-parse)
   - [MySQL2](https://github.com/sidorares/node-mysql2)
+  - [parse-json](https://github.com/sindresorhus/parse-json)
   - [q](http://documentup.com/kriskowal/q/)
-  
+  - [safe-json-parse](https://github.com/Raynos/safe-json-parse)
+
 ## New queries
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
@@ -43,3 +49,4 @@
 ## Changes to QL libraries
 
 * HTTP header names are now always normalized to lower case to reflect the fact that they are case insensitive. In particular, the result of `HeaderDefinition.getAHeaderName`, and the first parameter of `HeaderDefinition.defines`, `ExplicitHeaderDefinition.definesExplicitly` and `RouteHandler.getAResponseHeader` is now always a lower-case string.
+* The class `JsonParseCall` has been deprecated. Use `JsonParserCall` instead.

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

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

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

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`.

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;

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 |

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

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"
+  }
+}

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

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 |

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