diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java new file mode 100644 index 000000000000..93a860981d1d --- /dev/null +++ b/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]; + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp new file mode 100644 index 000000000000..fd62ab2968b2 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp @@ -0,0 +1,35 @@ + + + +

An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP +etc.), which is used to ensure security. Attackers can forge the value of these identifiers to +bypass a ban-list, for example.

+ +
+ + +

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 X-Forwarded-For 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.

+ +
+ + +

The following examples show the bad case and the good case respectively. +In bad1 method and bad2 method, the client ip the X-Forwarded-For 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 +good1 similarly splits an X-Forwarded-For value, but uses the last, more-trustworthy entry.

+ + + +
+ + +
  • Dennis Schneider: +Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring +
  • + +
  • Security Rule Zero: A Warning about X-Forwarded-For +
  • + +
    +
    diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql new file mode 100644 index 000000000000..78d8bfee5f06 --- /dev/null +++ b/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" diff --git a/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/java/ql/src/experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll new file mode 100644 index 000000000000..fd9353d3414b --- /dev/null +++ b/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])?)$" +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected new file mode 100644 index 000000000000..5627f9541f18 --- /dev/null +++ b/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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.java new file mode 100644 index 000000000000..93a860981d1d --- /dev/null +++ b/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]; + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref new file mode 100644 index 000000000000..476249f5fd9a --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-348/options b/java/ql/test/experimental/query-tests/security/CWE-348/options new file mode 100644 index 000000000000..05f93394f49a --- /dev/null +++ b/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/ \ No newline at end of file diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/beans/factory/annotation/Autowired.java new file mode 100644 index 000000000000..8fcef9793603 --- /dev/null +++ b/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; +} \ No newline at end of file