Merge pull request #7057 from erik-krogh/cwe598

JS: add js/sensitive-get-query query

Author: erik-krogh

javascript/change-notes/2021-11-04-sensitive-get-query.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/sensitive-get-query` query has been added. It highlights GET requests that read sensitive information from the query string.

javascript/ql/src/Security/CWE-598/SensitiveGetQuery.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Sensitive information such as user passwords should not be transmitted within the query string of the requested URL. 
+Sensitive information within URLs may be logged in various locations, including the user's browser, the web server, 
+and any forward or reverse proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked  
+or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are 
+followed. Placing sensitive information into the URL therefore increases the risk that it will be captured by an attacker.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Use HTTP POST to send sensitive information as part of the request body; for example, as form data.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two route handlers that both receive a username and a password. 
+The first receives this sensitive information from the query parameters of a GET request, which is
+transmitted in the URL. The second receives this sensitive information from the request body of a POST request.
+</p>
+<sample src="examples/SensitiveGet.js" />
+</example>
+
+<references>
+  <li>
+    CWE:
+    <a href="https://cwe.mitre.org/data/definitions/598.html">CWE-598: Use of GET Request Method with Sensitive Query Strings</a>
+  </li>
+  <li>
+    PortSwigger (Burp):
+    <a href="https://portswigger.net/kb/issues/00400300_password-submitted-using-get-method">Password Submitted using GET Method</a>
+  </li>
+  <li>
+    OWASP:
+    <a href="https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url">Information Exposure through Query Strings in URL</a>
+  </li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-598/SensitiveGetQuery.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Sensitive data read from GET request
+ * @description Placing sensitive data in a GET request increases the risk of
+ *              the data being exposed to an attacker.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision high
+ * @id js/sensitive-get-query
+ * @tags security
+ *       external/cwe/cwe-598
+ */
+
+import javascript
+
+from
+  Express::RouteSetup setup, Express::RouteHandler handler, Express::RequestInputAccess input,
+  SensitiveExpr sensitive
+where
+  setup.getRequestMethod() = "GET" and
+  handler = setup.getARouteHandler() and
+  input.getRouteHandler() = handler and
+  input.getKind() = "parameter" and
+  input.(DataFlow::SourceNode).flowsToExpr(sensitive) and
+  not sensitive.getClassification() = SensitiveDataClassification::id()
+select input, "$@ for GET requests uses query parameter as sensitive data.", handler,
+  "Route handler"

javascript/ql/src/Security/CWE-598/examples/SensitiveGet.js

@@ -0,0 +1,25 @@
+const express = require('express');
+const app = express();
+app.use(require('body-parser').urlencoded({ extended: false }))
+
+// bad: sensitive information is read from query parameters
+app.get('/login1', (req, res) => {
+    const user = req.query.user;
+    const password = req.query.password;
+    if (checkUser(user, password)) {
+        res.send('Welcome');
+    } else {
+        res.send('Access denied');
+    }
+});
+
+// good: sensitive information is read from post body
+app.post('/login2', (req, res) => {
+    const user = req.body.user;
+    const password = req.body.password;
+    if (checkUser(user, password)) {
+        res.send('Welcome');
+    } else {
+        res.send('Access denied');
+    }
+});

javascript/ql/test/query-tests/Security/CWE-598/SensitiveGetQuery.expected

@@ -0,0 +1,3 @@
+| tst.js:8:22:8:39 | req.query.password | $@ for GET requests uses query parameter as sensitive data. | tst.js:6:19:14:1 | (req, r ... serId\\n} | Route handler |
+| tst.js:26:22:26:42 | req.par ... sword') | $@ for GET requests uses query parameter as sensitive data. | tst.js:24:20:35:1 | (req, r ...   });\\n} | Route handler |
+| tst.js:31:24:31:40 | req.param('word') | $@ for GET requests uses query parameter as sensitive data. | tst.js:24:20:35:1 | (req, r ...   });\\n} | Route handler |

javascript/ql/test/query-tests/Security/CWE-598/SensitiveGetQuery.qlref

@@ -0,0 +1 @@
+Security/CWE-598/SensitiveGetQuery.ql

javascript/ql/test/query-tests/Security/CWE-598/tst.js

@@ -0,0 +1,35 @@
+const express = require('express');
+const app = express();
+const bodyParser = require('body-parser');
+app.use(bodyParser.urlencoded({extended: true}));
+
+app.get("/login", (req, res) => {
+    const username = req.query.username; // OK - usernames are fine
+    const password = req.query.password; // NOT OK - password read
+    checkUser(username, password, (result) => {
+        res.send(result);
+    });
+
+    doThing(req.query.userId); // OK - userId
+});
+
+app.post("/login", (req, res) => {
+    const username = req.body.username; // OK - usernames are fine
+    const password = req.body.password; // OK - not a query parameter
+    checkUser(username, password, (result) => {
+        res.send(result);
+    });
+});
+
+app.get("/login2", (req, res) => {
+    const username = req.param('username'); // NOT OK - usernames are fine
+    const password = req.param('password'); // NOT OK - password read
+    checkUser(username, password, (result) => {
+        res.send(result);
+    });
+
+    const myPassword = req.param('word'); // NOT OK - is used in a sensitive write below.
+    checkUser(username, myPassword, (result) => {
+        res.send(result);
+    });
+});