[Javascript] Add CWE-348 ClientSuppliedIpUsedInSecurityCheck

javascript/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>An original client IP address is retrieved from an http header (<code>X-Forwarded-For</code> or <code>X-Real-IP</code> or <code>Proxy-Client-IP</code> 
+etc.), which is used to ensure security. Attackers can forge the value of these identifiers to
+bypass a blacklist, for example.</p>
+
+</overview>
+<recommendation>
+
+<p>Do not trust the values of HTTP headers allegedly identifying the originating IP. If you are aware your application will run behind some reverse proxies then the last entry of a <code>X-Forwarded-For</code> header value may be more trustworthy than the rest of it because some reverse proxies append the IP address they observed to the end of any remote-supplied header.</p>
+
+</recommendation>
+<example>
+
+<p>The following examples show the bad case and the good case respectively. 
+In <code>/bad1</code> endpoint, the client IP from the <code>X-Forwarded-For</code> header is used directly to check against a whitelist. In <code>/bad2</code> endpoint, this value is split into multiple IPs, separated by a comma, but the less-trustworthy first one is used. Both of these examples could be deceived by providing a forged HTTP header. 
+The endpoint <code>/good</code> similarly splits the value from <code>X-Forwarded-For</code> header but uses the last, more trustworthy entry.</p>
+
+<sample src="examples/ClientSuppliedIpUsedInSecurityCheck.js" />
+
+</example>
+<references>
+
+<li>Dennis Schneider: <a href="https://www.dennis-schneider.com/blog/prevent-ip-address-spoofing-with-x-forwarded-for-header-and-aws-elb-in-clojure-ring/">
+Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring</a>
+</li>
+
+<li>Security Rule Zero: <a href="https://www.f5.com/company/blog/security-rule-zero-a-warning-about-x-forwarded-for">A Warning about X-Forwarded-For</a>
+</li>
+
+</references>
+</qhelp>

javascript/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql

@@ -0,0 +1,56 @@
+/**
+ * @name IP address spoofing
+ * @description A remote endpoint identifier is read from an HTTP header. Attackers can modify the value
+ *              of the identifier to forge the client ip.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/ip-address-spoofing
+ * @tags security
+ *       external/cwe/cwe-348
+ */
+
+import javascript
+import semmle.javascript.dataflow.DataFlow
+import semmle.javascript.dataflow.TaintTracking
+import ClientSuppliedIpUsedInSecurityCheckLib
+
+/**
+ * Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use.
+ */
+class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration {
+  ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof ClientSuppliedIp }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof PossibleSecurityCheck }
+
+  /**
+   * Splitting a header value by `,` and taking an entry other than the first is sanitizing, because
+   * later entries may originate from more-trustworthy intermediate proxies, not the original client.
+   */
+  override predicate isSanitizer(DataFlow::Node node) {
+    // ip.split(",").pop(); or var temp = ip.split(","); ip = temp.pop();
+    exists(DataFlow::MethodCallNode split, DataFlow::MethodCallNode pop |
+      split = node and
+      split.getMethodName() = "split" and
+      pop.getMethodName() = "pop" and
+      split.getACall() = pop
+    )
+    or
+    // ip.split(",")[ip.split(",").length - 1]; or var temp = ip.split(","); ip = temp[temp.length - 1];
+    exists(DataFlow::MethodCallNode split, DataFlow::PropRead read |
+      split = node and
+      split.getMethodName() = "split" and
+      read = split.getAPropertyRead() and
+      not read.getPropertyNameExpr().getIntValue() = 0
+    )
+  }
+}
+
+from
+  ClientSuppliedIpUsedInSecurityCheckConfig config, DataFlow::PathNode source,
+  DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
+  source.getNode(), "this user input"

javascript/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll

@@ -0,0 +1,78 @@
+private import javascript
+private import DataFlow::PathGraph
+private import semmle.javascript.dataflow.DataFlow
+
+/**
+ * A data flow source of the client ip obtained according to the remote endpoint identifier specified
+ * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
+ *
+ * For example: `req.headers["X-Forwarded-For"]`.
+ */
+abstract class ClientSuppliedIp extends DataFlow::Node { }
+
+private class GenericClientSuppliedIp extends ClientSuppliedIp {
+  GenericClientSuppliedIp() {
+    exists(DataFlow::SourceNode source, DataFlow::PropRead read |
+      this.(RemoteFlowSource).getSourceType() = "Server request header" and source = this
+    |
+      read.getPropertyName().toLowerCase() = clientIpParameterName() and
+      source.flowsTo(read)
+    )
+  }
+}
+
+private string clientIpParameterName() {
+  result in [
+      "x-forwarded-for", "x_forwarded_for", "x-real-ip", "x_real_ip", "proxy-client-ip",
+      "proxy_client_ip", "wl-proxy-client-ip", "wl_proxy_client_ip", "http_x_forwarded_for",
+      "http-x-forwarded-for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip",
+      "http_forwarded_for", "http_forwarded", "http_via", "remote_addr"
+    ]
+}
+
+/** A data flow sink for ip address forgery vulnerabilities. */
+abstract class PossibleSecurityCheck extends DataFlow::Node { }
+
+/**
+ * A data flow sink for remote client ip comparison.
+ *
+ * For example: `if !ip.startsWith("10.")` determine whether the client ip starts
+ * with `10.`, and the program can be deceived by forging the ip address.
+ */
+private class CompareSink extends PossibleSecurityCheck {
+  CompareSink() {
+    // ip.startsWith("10.") or ip.includes("10.")
+    exists(CallExpr call |
+      call.getAChild() = this.asExpr() and
+      call.getCalleeName() in ["startsWith", "includes"] and
+      call.getArgument(0).getStringValue().regexpMatch(getIpAddressRegex()) and
+      not call.getArgument(0).getStringValue() = "0:0:0:0:0:0:0:1"
+    )
+    or
+    // ip === "127.0.0.1" or ip !== "127.0.0.1" or ip == "127.0.0.1" or or ip != "127.0.0.1"
+    exists(Comparison compare |
+      compare instanceof EqualityTest and
+      (
+        [compare, compare.getAnOperand()] = this.asExpr() and
+        compare.getAnOperand().getStringValue() instanceof PrivateHostName and
+        not compare.getAnOperand().getStringValue() = "0:0:0:0:0:0:0:1"
+      )
+    )
+  }
+}
+
+string getIpAddressRegex() {
+  result =
+    "^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$"
+}
+
+/**
+ * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.
+ * Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1]
+ */
+private class PrivateHostName extends string {
+  bindingset[this]
+  PrivateHostName() {
+    this.regexpMatch("(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?")
+  }
+}

javascript/ql/src/experimental/Security/CWE-348/examples/ClientSuppliedIpUsedInSecurityCheck.js

@@ -0,0 +1,60 @@
+const express = require("express");
+const app = express();
+const port = 3000;
+
+app.get("/bad1", (req, res) => {
+  var ip = req.headers["x-forwarded-for"];
+
+  // Bad: use this value directly
+  if (ip && ip === "127.0.0.1") {
+    res.writeHead(200);
+    return res.end("Hello, World!");
+  }
+
+  res.writeHead(403);
+  res.end("illegal ip");
+});
+
+app.get("/bad2", (req, res) => {
+  var ip = req.headers["x-forwarded-for"];
+  if (!ip) {
+    res.writeHead(403);
+    return res.end("illegal ip");
+  }
+
+  // Bad: the first IP is used
+  var temp = ip.split(",");
+  ip = temp[0];
+
+  if (ip && ip === "127.0.0.1") {
+    res.writeHead(200);
+    return res.end("Hello, World!");
+  }
+
+  res.writeHead(403);
+  res.end("illegal ip");
+});
+
+app.get("/good", (req, res) => {
+  var ip = req.headers["x-forwarded-for"];
+  if (!ip) {
+    res.writeHead(403);
+    return res.end("illegal ip");
+  }
+
+  // Good: if this application runs behind a reverse proxy it may append the real remote IP to the end of any client-supplied X-Forwarded-For header.
+  var temp = ip.split(",");
+  ip = temp[temp.length - 1];
+
+  if (ip && ip === "127.0.0.1") {
+    res.writeHead(200);
+    return res.end("Hello, World!");
+  }
+
+  res.writeHead(403);
+  res.end("illegal ip");
+});
+
+app.listen(port, () => {
+  console.log(`Example app listening at http://localhost:${port}`);
+});