Merge pull request github#13771 from github/max-schaefer/server-side-url-redirect-help

JavaScript: Improve query help for `js/server-side-unvalidated-url-redirection`.

Author: max-schaefer

javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp

@@ -16,6 +16,10 @@ To guard against untrusted URL redirection, it is advisable to avoid putting use
 directly into a redirect URL. Instead, maintain a list of authorized
 redirects on the server; then choose from that list based on the user input provided.
 </p>
+<p>
+If this is not possible, then the user input should be validated in some other way,
+for example, by verifying that the target URL is on the same host as the current page.
+</p>
 </recommendation>
 
 <example>
@@ -32,6 +36,21 @@ before doing the redirection:
 </p>
 
 <sample src="examples/ServerSideUrlRedirectGood.js"/>
+
+<p>
+Alternatively, we can check that the target URL does not redirect to a different host
+by parsing it relative to a base URL with a known host and verifying that the host
+stays the same:
+</p>
+
+<sample src="examples/ServerSideUrlRedirectGood2.js"/>
+
+<p>
+Note that as written, the above code will allow redirects to URLs on <code>example.com</code>,
+which is harmless but perhaps not intended. You can substitute your own domain (if known) for
+<code>example.com</code> to prevent this.
+</p>
+
 </example>
 
 <references>

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js

@@ -1,6 +1,6 @@
 const app = require("express")();
 
-app.get('/some/path', function(req, res) {
+app.get("/redirect", function (req, res) {
   // BAD: a request parameter is incorporated without validation into a URL redirect
-  res.redirect(req.param("target"));
+  res.redirect(req.query["target"]);
 });

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js

@@ -2,9 +2,12 @@ const app = require("express")();
 
 const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html";
 
-app.get('/some/path', function(req, res) {
+app.get("/redirect", function (req, res) {
   // GOOD: the request parameter is validated against a known fixed string
-  let target = req.param("target");
-  if (VALID_REDIRECT === target)
+  let target = req.query["target"];
+  if (VALID_REDIRECT === target) {
     res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
 });

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js

@@ -0,0 +1,22 @@
+const app = require("express")();
+
+function isLocalUrl(path) {
+  try {
+    return (
+      // TODO: consider substituting your own domain for example.com
+      new URL(path, "https://example.com").origin === "https://example.com"
+    );
+  } catch (e) {
+    return false;
+  }
+}
+
+app.get("/redirect", function (req, res) {
+  // GOOD: check that we don't redirect to a different host
+  let target = req.query["target"];
+  if (isLocalUrl(target)) {
+    res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
+});

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected

@@ -1,4 +1,7 @@
 nodes
+| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] |
+| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] |
+| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] |
 | express.js:7:16:7:34 | req.param("target") |
 | express.js:7:16:7:34 | req.param("target") |
 | express.js:7:16:7:34 | req.param("target") |
@@ -114,6 +117,7 @@ nodes
 | react-native.js:9:26:9:32 | tainted |
 | react-native.js:9:26:9:32 | tainted |
 edges
+| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] |
 | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") |
 | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") |
 | express.js:27:7:27:34 | target | express.js:33:18:33:23 | target |
@@ -211,6 +215,7 @@ edges
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 #select
+| ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | Untrusted URL redirection depends on a $@. | ServerSideUrlRedirect.js:5:16:5:34 | req.query["target"] | user-provided value |
 | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | express.js:7:16:7:34 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:7:16:7:34 | req.param("target") | user-provided value |
 | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | express.js:12:26:12:44 | req.param("target") | Untrusted URL redirection depends on a $@. | express.js:12:26:12:44 | req.param("target") | user-provided value |
 | express.js:33:18:33:23 | target | express.js:27:16:27:34 | req.param("target") | express.js:33:18:33:23 | target | Untrusted URL redirection depends on a $@. | express.js:27:16:27:34 | req.param("target") | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.js

@@ -0,0 +1,6 @@
+const app = require("express")();
+
+app.get("/redirect", function (req, res) {
+  // BAD: a request parameter is incorporated without validation into a URL redirect
+  res.redirect(req.query["target"]);
+});

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood.js

@@ -0,0 +1,13 @@
+const app = require("express")();
+
+const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html";
+
+app.get("/redirect", function (req, res) {
+  // GOOD: the request parameter is validated against a known fixed string
+  let target = req.query["target"];
+  if (VALID_REDIRECT === target) {
+    res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
+});

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirectGood2.js

@@ -0,0 +1,22 @@
+const app = require("express")();
+
+function isLocalUrl(path) {
+  try {
+    return (
+      // TODO: consider substituting your own domain for example.com
+      new URL(path, "https://example.com").origin === "https://example.com"
+    );
+  } catch (e) {
+    return false;
+  }
+}
+
+app.get("/redirect", function (req, res) {
+  // GOOD: check that we don't redirect to a different host
+  let target = req.query["target"];
+  if (isLocalUrl(target)) {
+    res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
+});