elaborate on both SQL and NoSQL injection in the js/sql-injection qhelp

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,47 @@ 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 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" />
+</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 check that the user input is a
+literal value and not a query object before using it in a query.
+</p>
+
+<sample src="examples/NoSqlInjectionFix.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/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,21 @@
+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;
+  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
+  });
+});