Merge pull request github#13329 from erik-krogh/sqlhelp

JS: improve the sql-injection help page

Author: erik-krogh

javascript/ql/src/Security/CWE-089/SqlInjection.inc.qhelp

@@ -20,7 +20,13 @@ or prepared statements.
 <p>
 For NoSQL queries, make use of an operator like MongoDB's <code>$eq</code>
 to ensure that untrusted data is interpreted as a literal value and not as
-a query object.
+a query object. Alternatively, check that the untrusted data is a literal
+value and not a query object before using it in a query.
+</p>
+<p>
+For SQL queries, use query parameters or prepared statements to
+embed untrusted data into the query string, or use a library like 
+<code>sqlstring</code> to escape untrusted data.
 </p>
 </recommendation>
 
@@ -32,31 +38,61 @@ an HTTP request handler in a web application, whose parameter
 </p>
 
 <p>
-The handler constructs two copies of the same SQL query involving
-user input taken from the request object, once unsafely using
-string concatenation, and once safely using query parameters.
+The handler constructs an SQL query string from user input
+and executes it as a database query using the <code>pg</code> library. 
+The user input may contain quote characters, so this code is vulnerable
+to a SQL injection attack.
 </p>
 
-<p>
-In the first case, the query string <code>query1</code> is built by
-directly concatenating a user-supplied request parameter with some
-string literals. The parameter may include quote characters, so this
-code is vulnerable to a SQL injection attack.
-</p>
+<sample src="examples/SqlInjection.js" />
 
 <p>
-In the second case, the parameter is embedded into the query string
-<code>query2</code> using query parameters. In this example, we use
+To fix this vulnerability, we can use query parameters to embed the
+user input into the query string. In this example, we use
 the API offered by the <code>pg</code> Postgres database connector
 library, but other libraries offer similar features. This version is
 immune to injection attacks.
 </p>
 
-<sample src="examples/SqlInjection.js" />
+<sample src="examples/SqlInjectionFix.js" />
+
+<p>
+Alternatively, we can use a library like <code>sqlstring</code> to
+escape the user input before embedding it into the query string:
+</p>
+<sample src="examples/SqlInjectionFix2.js" />
+</example>
+
+<example>
+<p>
+In the following example, an express handler attempts to delete
+a single document from a MongoDB collection. The document to be
+deleted is identified by its <code>_id</code> field, which is
+constructed from user input. The user input may contain a query
+object, so this code is vulnerable to a NoSQL injection attack.
+</p>
+
+<sample src="examples/NoSqlInjection.js" />
+
+<p>
+To fix this vulnerability, we can use the <code>$eq</code> operator
+to ensure that the user input is interpreted as a literal value
+and not as a query object:
+</p>
+
+<sample src="examples/NoSqlInjectionFix.js" />
+
+<p>
+Alternatively check that the user input is a
+literal value and not a query object before using it:
+</p>
+
+<sample src="examples/NoSqlInjectionFix2.js" />
 </example>
 
 <references>
 <li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
 <li>MongoDB: <a href="https://docs.mongodb.com/manual/reference/operator/query/eq">$eq operator</a>.</li>
+<li>OWASP: <a href="https://owasp.org/www-pdf-archive/GOD16-NOSQL.pdf">NoSQL injection</a>.</li>
 </references>
 </qhelp>

javascript/ql/src/Security/CWE-089/SqlInjection.ql

@@ -18,12 +18,13 @@ import semmle.javascript.security.dataflow.SqlInjectionQuery as SqlInjection
 import semmle.javascript.security.dataflow.NosqlInjectionQuery as NosqlInjection
 import DataFlow::PathGraph
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, string type
 where
   (
-    cfg instanceof SqlInjection::Configuration or
-    cfg instanceof NosqlInjection::Configuration
+    cfg instanceof SqlInjection::Configuration and type = "string"
+    or
+    cfg instanceof NosqlInjection::Configuration and type = "object"
   ) and
   cfg.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
+select sink.getNode(), source, sink, "This query " + type + " depends on a $@.", source.getNode(),
   "user-provided value"

javascript/ql/src/Security/CWE-089/examples/NoSqlInjection.js

@@ -0,0 +1,18 @@
+const express = require("express");
+const mongoose = require("mongoose");
+const Todo = mongoose.model(
+  "Todo",
+  new mongoose.Schema({ text: { type: String } }, { timestamps: true })
+);
+
+const app = express();
+app.use(express.json());
+app.use(express.urlencoded({ extended: false }));
+
+app.delete("/api/delete", async (req, res) => {
+  let id = req.body.id;
+
+  await Todo.deleteOne({ _id: id }); // BAD: id might be an object with special properties
+
+  res.json({ status: "ok" });
+});

javascript/ql/src/Security/CWE-089/examples/NoSqlInjectionFix.js

@@ -0,0 +1,6 @@
+app.delete("/api/delete", async (req, res) => {
+  let id = req.body.id;
+  await Todo.deleteOne({ _id: { $eq: id } }); // GOOD: using $eq operator for the comparison
+
+  res.json({ status: "ok" });
+});

javascript/ql/src/Security/CWE-089/examples/NoSqlInjectionFix2.js

@@ -0,0 +1,10 @@
+app.delete("/api/delete", async (req, res) => {
+  let id = req.body.id;
+  if (typeof id !== "string") {
+    res.status(400).json({ status: "error" });
+    return;
+  }
+  await Todo.deleteOne({ _id: id }); // GOOD: id is guaranteed to be a string
+
+  res.json({ status: "ok" });
+});

javascript/ql/src/Security/CWE-089/examples/SqlInjection.js

@@ -11,11 +11,4 @@ app.get("search", function handler(req, res) {
   pool.query(query1, [], function(err, results) {
     // process results
   });
-
-  // GOOD: use parameters
-  var query2 =
-    "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY=$1" + " ORDER BY PRICE";
-  pool.query(query2, [req.params.category], function(err, results) {
-    // process results
-  });
 });

javascript/ql/src/Security/CWE-089/examples/SqlInjectionFix.js

@@ -0,0 +1,12 @@
+const app = require("express")(),
+      pg = require("pg"),
+      pool = new pg.Pool(config);
+
+app.get("search", function handler(req, res) {
+  // GOOD: use parameters
+  var query2 =
+    "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY=$1 ORDER BY PRICE";
+  pool.query(query2, [req.params.category], function(err, results) {
+    // process results
+  });
+});

javascript/ql/src/Security/CWE-089/examples/SqlInjectionFix2.js

@@ -0,0 +1,15 @@
+const app = require("express")(),
+      pg = require("pg"),
+      SqlString = require('sqlstring'),
+      pool = new pg.Pool(config);
+
+app.get("search", function handler(req, res) {
+  // GOOD: the category is escaped using mysql.escape
+  var query1 =
+    "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" +
+    SqlString.escape(req.params.category) +
+    "' ORDER BY PRICE";
+  pool.query(query1, [], function(err, results) {
+    // process results
+  });
+});

javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-089/SqlInjection.ql

@@ -20,13 +20,13 @@ import semmle.javascript.security.dataflow.NosqlInjectionQuery as NosqlInjection
 import DataFlow::PathGraph
 import semmle.javascript.heuristics.AdditionalSources
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, string type
 where
   (
-    cfg instanceof SqlInjection::Configuration or
-    cfg instanceof NosqlInjection::Configuration
+    cfg instanceof SqlInjection::Configuration and type = "string"
+    or
+    cfg instanceof NosqlInjection::Configuration and type = "object"
   ) and
-  cfg.hasFlowPath(source, sink) and
-  source.getNode() instanceof HeuristicSource
-select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
+  cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This query " + type + " depends on a $@.", source.getNode(),
   "user-provided value"

javascript/ql/test/query-tests/Security/CWE-089/typed/SqlInjection.expected

@@ -37,6 +37,6 @@ edges
 | typedClient.ts:23:33:23:33 | v | typedClient.ts:23:27:23:35 | { id: v } |
 | typedClient.ts:23:33:23:33 | v | typedClient.ts:23:27:23:35 | { id: v } |
 #select
-| typedClient.ts:14:24:14:32 | { id: v } | typedClient.ts:13:22:13:29 | req.body | typedClient.ts:14:24:14:32 | { id: v } | This query depends on a $@. | typedClient.ts:13:22:13:29 | req.body | user-provided value |
-| typedClient.ts:22:27:22:35 | { id: v } | typedClient.ts:21:22:21:29 | req.body | typedClient.ts:22:27:22:35 | { id: v } | This query depends on a $@. | typedClient.ts:21:22:21:29 | req.body | user-provided value |
-| typedClient.ts:23:27:23:35 | { id: v } | typedClient.ts:21:22:21:29 | req.body | typedClient.ts:23:27:23:35 | { id: v } | This query depends on a $@. | typedClient.ts:21:22:21:29 | req.body | user-provided value |
+| typedClient.ts:14:24:14:32 | { id: v } | typedClient.ts:13:22:13:29 | req.body | typedClient.ts:14:24:14:32 | { id: v } | This query object depends on a $@. | typedClient.ts:13:22:13:29 | req.body | user-provided value |
+| typedClient.ts:22:27:22:35 | { id: v } | typedClient.ts:21:22:21:29 | req.body | typedClient.ts:22:27:22:35 | { id: v } | This query object depends on a $@. | typedClient.ts:21:22:21:29 | req.body | user-provided value |
+| typedClient.ts:23:27:23:35 | { id: v } | typedClient.ts:21:22:21:29 | req.body | typedClient.ts:23:27:23:35 | { id: v } | This query object depends on a $@. | typedClient.ts:21:22:21:29 | req.body | user-provided value |