[Java] CWE-348: Using a client-supplied IP address in a security check

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

@@ -0,0 +1,49 @@
+import javax.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.StringUtils;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.ResponseBody;
+
+@Controller
+public class ClientSuppliedIpUsedInSecurityCheck {
+
+    @Autowired
+    private HttpServletRequest request;
+
+    @GetMapping(value = "bad1")
+    public void bad1(HttpServletRequest request) {
+        String ip = getClientIP();
+        if (!StringUtils.startsWith(ip, "192.168.")) {
+            new Exception("ip illegal");
+        }
+    }
+
+    @GetMapping(value = "bad2")
+    public void bad2(HttpServletRequest request) {
+        String ip = getClientIP();
+        if (!"127.0.0.1".equals(ip)) {
+            new Exception("ip illegal");
+        }
+    }
+
+    @GetMapping(value = "good1")
+    @ResponseBody
+    public String good1(HttpServletRequest request) {
+        String ip = request.getHeader("X-FORWARDED-FOR");
+        // 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.
+        ip = ip.split(",")[ip.split(",").length - 1];
+        if (!StringUtils.startsWith(ip, "192.168.")) {
+            new Exception("ip illegal");
+        }
+        return ip;
+    }
+
+    protected String getClientIP() {
+        String xfHeader = request.getHeader("X-Forwarded-For");
+        if (xfHeader == null) {
+            return request.getRemoteAddr();
+        }
+        return xfHeader.split(",")[0];
+    }
+}

java/ql/src/experimental/Security/CWE/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 ban-list, 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> method and <code>bad2</code> method, the client ip the <code>X-Forwarded-For</code> is split into comma-separated values, but the less-trustworthy first one is used. Both of these examples could be deceived by providing a forged HTTP header. The method
+<code>good1</code> similarly splits an <code>X-Forwarded-For</code> value, but uses the last, more-trustworthy entry.</p>
+
+<sample src="ClientSuppliedIpUsedInSecurityCheck.java" />
+
+</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>

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

@@ -0,0 +1,53 @@
+/**
+ * @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 java/ip-address-spoofing
+ * @tags security
+ *       external/cwe/cwe-348
+ */
+
+import java
+import ClientSuppliedIpUsedInSecurityCheckLib
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+/**
+ * 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 ClientSuppliedIpUsedInSecurityCheck
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink instanceof ClientSuppliedIpUsedInSecurityCheckSink
+  }
+
+  /**
+   * 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) {
+    exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma |
+      ma.getQualifier() = node.asExpr() and
+      ma.getMethod() instanceof SplitMethod and
+      not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0
+    )
+    or
+    node.getType() instanceof PrimitiveType
+    or
+    node.getType() instanceof BoxedType
+  }
+}
+
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, ClientSuppliedIpUsedInSecurityCheckConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
+  source.getNode(), "this user input"

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

@@ -0,0 +1,100 @@
+import java
+import DataFlow
+import semmle.code.java.frameworks.Networking
+import semmle.code.java.security.QueryInjection
+import experimental.semmle.code.java.Logging
+
+/**
+ * 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: `ServletRequest.getHeader("X-Forwarded-For")`.
+ */
+class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node {
+  ClientSuppliedIpUsedInSecurityCheck() {
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("getHeader") and
+      ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [
+          "x-forwarded-for", "x-real-ip", "proxy-client-ip", "wl-proxy-client-ip",
+          "http_x_forwarded_for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip",
+          "http_forwarded_for", "http_forwarded", "http_via", "remote_addr"
+        ] and
+      ma = this.asExpr()
+    )
+  }
+}
+
+/** A data flow sink for ip address forgery vulnerabilities. */
+abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::Node { }
+
+/**
+ * A data flow sink for remote client ip comparison.
+ *
+ * For example: `if (!StringUtils.startsWith(ipAddr, "192.168.")){...` determine whether the client ip starts
+ * with `192.168.`, and the program can be deceived by forging the ip address.
+ */
+private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink {
+  CompareSink() {
+    exists(MethodAccess ma |
+      ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and
+      ma.getMethod().getDeclaringType() instanceof TypeString and
+      ma.getMethod().getNumberOfParameters() = 1 and
+      (
+        ma.getArgument(0) = this.asExpr() and
+        ma.getQualifier().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
+        not ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
+        or
+        ma.getQualifier() = this.asExpr() and
+        ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
+        not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
+      )
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().getName() in ["contains", "startsWith"] and
+      ma.getMethod().getDeclaringType() instanceof TypeString and
+      ma.getMethod().getNumberOfParameters() = 1 and
+      ma.getQualifier() = this.asExpr() and
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex()) // Matches IP-address-like strings
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("startsWith") and
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and
+      ma.getMethod().getNumberOfParameters() = 2 and
+      ma.getAnArgument() = this.asExpr() and
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex())
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and
+      ma.getMethod().getNumberOfParameters() = 2 and
+      ma.getAnArgument() = this.asExpr() and
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
+      not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
+    )
+  }
+}
+
+/** A data flow sink for sql operation. */
+private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink {
+  SqlOperationSink() { this instanceof QueryInjectionSink }
+}
+
+/** A method that split string. */
+class SplitMethod extends Method {
+  SplitMethod() {
+    this.getNumberOfParameters() = 1 and
+    this.hasQualifiedName("java.lang", "String", "split")
+  }
+}
+
+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])?)$"
+}

java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected

@@ -0,0 +1,16 @@
+edges
+| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String |
+nodes
+| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | semmle.label | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | semmle.label | ip |
+| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | semmle.label | getHeader(...) : String |
+| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | semmle.label | ...[...] : String |
+#select
+| ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) | this user input |
+| ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) | this user input |

java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java

@@ -0,0 +1,49 @@
+import javax.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.StringUtils;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.ResponseBody;
+
+@Controller
+public class ClientSuppliedIpUsedInSecurityCheck {
+
+    @Autowired
+    private HttpServletRequest request;
+
+    @GetMapping(value = "bad1")
+    public void bad1(HttpServletRequest request) {
+        String ip = getClientIP();
+        if (!StringUtils.startsWith(ip, "192.168.")) {
+            new Exception("ip illegal");
+        }
+    }
+
+    @GetMapping(value = "bad2")
+    public void bad2(HttpServletRequest request) {
+        String ip = getClientIP();
+        if (!"127.0.0.1".equals(ip)) {
+            new Exception("ip illegal");
+        }
+    }
+
+    @GetMapping(value = "good1")
+    @ResponseBody
+    public String good1(HttpServletRequest request) {
+        String ip = request.getHeader("X-FORWARDED-FOR");
+        // 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.
+        ip = ip.split(",")[ip.split(",").length - 1];
+        if (!StringUtils.startsWith(ip, "192.168.")) {
+            new Exception("ip illegal");
+        }
+        return ip;
+    }
+
+    protected String getClientIP() {
+        String xfHeader = request.getHeader("X-Forwarded-For");
+        if (xfHeader == null) {
+            return request.getRemoteAddr();
+        }
+        return xfHeader.split(",")[0];
+    }
+}

java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql

java/ql/test/experimental/query-tests/security/CWE-348/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/

java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java

@@ -0,0 +1,14 @@
+package org.springframework.beans.factory.annotation;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Target({ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD, ElementType.ANNOTATION_TYPE})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface Autowired {
+    boolean required() default true;
+}