Skip to content

Commit b2c0259

Browse files
smowtonsmowton
authored
Merge pull request github#5631 from haby0/UseOfLessTrustedSource
[Java] CWE-348: Using a client-supplied IP address in a security check
2 parents ecd40e5 + fdcc517 commit b2c0259

File tree

9 files changed

+318
-0
lines changed

9 files changed

+318
-0
lines changed

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import javax.servlet.http.HttpServletRequest;
2+
import org.apache.commons.lang3.StringUtils;
3+
import org.springframework.beans.factory.annotation.Autowired;
4+
import org.springframework.stereotype.Controller;
5+
import org.springframework.web.bind.annotation.GetMapping;
6+
import org.springframework.web.bind.annotation.ResponseBody;
7+
8+
@Controller
9+
public class ClientSuppliedIpUsedInSecurityCheck {
10+
11+
@Autowired
12+
private HttpServletRequest request;
13+
14+
@GetMapping(value = "bad1")
15+
public void bad1(HttpServletRequest request) {
16+
String ip = getClientIP();
17+
if (!StringUtils.startsWith(ip, "192.168.")) {
18+
new Exception("ip illegal");
19+
}
20+
}
21+
22+
@GetMapping(value = "bad2")
23+
public void bad2(HttpServletRequest request) {
24+
String ip = getClientIP();
25+
if (!"127.0.0.1".equals(ip)) {
26+
new Exception("ip illegal");
27+
}
28+
}
29+
30+
@GetMapping(value = "good1")
31+
@ResponseBody
32+
public String good1(HttpServletRequest request) {
33+
String ip = request.getHeader("X-FORWARDED-FOR");
34+
// 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.
35+
ip = ip.split(",")[ip.split(",").length - 1];
36+
if (!StringUtils.startsWith(ip, "192.168.")) {
37+
new Exception("ip illegal");
38+
}
39+
return ip;
40+
}
41+
42+
protected String getClientIP() {
43+
String xfHeader = request.getHeader("X-Forwarded-For");
44+
if (xfHeader == null) {
45+
return request.getRemoteAddr();
46+
}
47+
return xfHeader.split(",")[0];
48+
}
49+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<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>
7+
etc.), which is used to ensure security. Attackers can forge the value of these identifiers to
8+
bypass a ban-list, for example.</p>
9+
10+
</overview>
11+
<recommendation>
12+
13+
<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>
14+
15+
</recommendation>
16+
<example>
17+
18+
<p>The following examples show the bad case and the good case respectively.
19+
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
20+
<code>good1</code> similarly splits an <code>X-Forwarded-For</code> value, but uses the last, more-trustworthy entry.</p>
21+
22+
<sample src="ClientSuppliedIpUsedInSecurityCheck.java" />
23+
24+
</example>
25+
<references>
26+
27+
<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/">
28+
Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring</a>
29+
</li>
30+
31+
<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>
32+
</li>
33+
34+
</references>
35+
</qhelp>

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/**
2+
* @name IP address spoofing
3+
* @description A remote endpoint identifier is read from an HTTP header. Attackers can modify the value
4+
* of the identifier to forge the client ip.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/ip-address-spoofing
9+
* @tags security
10+
* external/cwe/cwe-348
11+
*/
12+
13+
import java
14+
import ClientSuppliedIpUsedInSecurityCheckLib
15+
import semmle.code.java.dataflow.FlowSources
16+
import DataFlow::PathGraph
17+
18+
/**
19+
* Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use.
20+
*/
21+
class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration {
22+
ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" }
23+
24+
override predicate isSource(DataFlow::Node source) {
25+
source instanceof ClientSuppliedIpUsedInSecurityCheck
26+
}
27+
28+
override predicate isSink(DataFlow::Node sink) {
29+
sink instanceof ClientSuppliedIpUsedInSecurityCheckSink
30+
}
31+
32+
/**
33+
* Splitting a header value by `,` and taking an entry other than the first is sanitizing, because
34+
* later entries may originate from more-trustworthy intermediate proxies, not the original client.
35+
*/
36+
override predicate isSanitizer(DataFlow::Node node) {
37+
exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma |
38+
ma.getQualifier() = node.asExpr() and
39+
ma.getMethod() instanceof SplitMethod and
40+
not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0
41+
)
42+
or
43+
node.getType() instanceof PrimitiveType
44+
or
45+
node.getType() instanceof BoxedType
46+
}
47+
}
48+
49+
from
50+
DataFlow::PathNode source, DataFlow::PathNode sink, ClientSuppliedIpUsedInSecurityCheckConfig conf
51+
where conf.hasFlowPath(source, sink)
52+
select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
53+
source.getNode(), "this user input"

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
import java
2+
import DataFlow
3+
import semmle.code.java.frameworks.Networking
4+
import semmle.code.java.security.QueryInjection
5+
import experimental.semmle.code.java.Logging
6+
7+
/**
8+
* A data flow source of the client ip obtained according to the remote endpoint identifier specified
9+
* (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
10+
*
11+
* For example: `ServletRequest.getHeader("X-Forwarded-For")`.
12+
*/
13+
class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node {
14+
ClientSuppliedIpUsedInSecurityCheck() {
15+
exists(MethodAccess ma |
16+
ma.getMethod().hasName("getHeader") and
17+
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [
18+
"x-forwarded-for", "x-real-ip", "proxy-client-ip", "wl-proxy-client-ip",
19+
"http_x_forwarded_for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip",
20+
"http_forwarded_for", "http_forwarded", "http_via", "remote_addr"
21+
] and
22+
ma = this.asExpr()
23+
)
24+
}
25+
}
26+
27+
/** A data flow sink for ip address forgery vulnerabilities. */
28+
abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::Node { }
29+
30+
/**
31+
* A data flow sink for remote client ip comparison.
32+
*
33+
* For example: `if (!StringUtils.startsWith(ipAddr, "192.168.")){...` determine whether the client ip starts
34+
* with `192.168.`, and the program can be deceived by forging the ip address.
35+
*/
36+
private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink {
37+
CompareSink() {
38+
exists(MethodAccess ma |
39+
ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and
40+
ma.getMethod().getDeclaringType() instanceof TypeString and
41+
ma.getMethod().getNumberOfParameters() = 1 and
42+
(
43+
ma.getArgument(0) = this.asExpr() and
44+
ma.getQualifier().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
45+
not ma.getQualifier().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
46+
or
47+
ma.getQualifier() = this.asExpr() and
48+
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
49+
not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
50+
)
51+
)
52+
or
53+
exists(MethodAccess ma |
54+
ma.getMethod().getName() in ["contains", "startsWith"] and
55+
ma.getMethod().getDeclaringType() instanceof TypeString and
56+
ma.getMethod().getNumberOfParameters() = 1 and
57+
ma.getQualifier() = this.asExpr() and
58+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex()) // Matches IP-address-like strings
59+
)
60+
or
61+
exists(MethodAccess ma |
62+
ma.getMethod().hasName("startsWith") and
63+
ma.getMethod()
64+
.getDeclaringType()
65+
.hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and
66+
ma.getMethod().getNumberOfParameters() = 2 and
67+
ma.getAnArgument() = this.asExpr() and
68+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().regexpMatch(getIpAddressRegex())
69+
)
70+
or
71+
exists(MethodAccess ma |
72+
ma.getMethod().getName() in ["equals", "equalsIgnoreCase"] and
73+
ma.getMethod()
74+
.getDeclaringType()
75+
.hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], "StringUtils") and
76+
ma.getMethod().getNumberOfParameters() = 2 and
77+
ma.getAnArgument() = this.asExpr() and
78+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() instanceof PrivateHostName and
79+
not ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "0:0:0:0:0:0:0:1"
80+
)
81+
}
82+
}
83+
84+
/** A data flow sink for sql operation. */
85+
private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink {
86+
SqlOperationSink() { this instanceof QueryInjectionSink }
87+
}
88+
89+
/** A method that split string. */
90+
class SplitMethod extends Method {
91+
SplitMethod() {
92+
this.getNumberOfParameters() = 1 and
93+
this.hasQualifiedName("java.lang", "String", "split")
94+
}
95+
}
96+
97+
string getIpAddressRegex() {
98+
result =
99+
"^((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])?)$"
100+
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
edges
2+
| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip |
3+
| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip |
4+
| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String |
5+
| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String |
6+
| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String |
7+
nodes
8+
| ClientSuppliedIpUsedInSecurityCheck.java:16:21:16:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
9+
| ClientSuppliedIpUsedInSecurityCheck.java:17:37:17:38 | ip | semmle.label | ip |
10+
| ClientSuppliedIpUsedInSecurityCheck.java:24:21:24:33 | getClientIP(...) : String | semmle.label | getClientIP(...) : String |
11+
| ClientSuppliedIpUsedInSecurityCheck.java:25:33:25:34 | ip | semmle.label | ip |
12+
| ClientSuppliedIpUsedInSecurityCheck.java:43:27:43:62 | getHeader(...) : String | semmle.label | getHeader(...) : String |
13+
| ClientSuppliedIpUsedInSecurityCheck.java:47:16:47:37 | ...[...] : String | semmle.label | ...[...] : String |
14+
#select
15+
| 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 |
16+
| 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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import javax.servlet.http.HttpServletRequest;
2+
import org.apache.commons.lang3.StringUtils;
3+
import org.springframework.beans.factory.annotation.Autowired;
4+
import org.springframework.stereotype.Controller;
5+
import org.springframework.web.bind.annotation.GetMapping;
6+
import org.springframework.web.bind.annotation.ResponseBody;
7+
8+
@Controller
9+
public class ClientSuppliedIpUsedInSecurityCheck {
10+
11+
@Autowired
12+
private HttpServletRequest request;
13+
14+
@GetMapping(value = "bad1")
15+
public void bad1(HttpServletRequest request) {
16+
String ip = getClientIP();
17+
if (!StringUtils.startsWith(ip, "192.168.")) {
18+
new Exception("ip illegal");
19+
}
20+
}
21+
22+
@GetMapping(value = "bad2")
23+
public void bad2(HttpServletRequest request) {
24+
String ip = getClientIP();
25+
if (!"127.0.0.1".equals(ip)) {
26+
new Exception("ip illegal");
27+
}
28+
}
29+
30+
@GetMapping(value = "good1")
31+
@ResponseBody
32+
public String good1(HttpServletRequest request) {
33+
String ip = request.getHeader("X-FORWARDED-FOR");
34+
// 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.
35+
ip = ip.split(",")[ip.split(",").length - 1];
36+
if (!StringUtils.startsWith(ip, "192.168.")) {
37+
new Exception("ip illegal");
38+
}
39+
return ip;
40+
}
41+
42+
protected String getClientIP() {
43+
String xfHeader = request.getHeader("X-Forwarded-For");
44+
if (xfHeader == null) {
45+
return request.getRemoteAddr();
46+
}
47+
return xfHeader.split(",")[0];
48+
}
49+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
package org.springframework.beans.factory.annotation;
2+
3+
import java.lang.annotation.Documented;
4+
import java.lang.annotation.ElementType;
5+
import java.lang.annotation.Retention;
6+
import java.lang.annotation.RetentionPolicy;
7+
import java.lang.annotation.Target;
8+
9+
@Target({ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD, ElementType.ANNOTATION_TYPE})
10+
@Retention(RetentionPolicy.RUNTIME)
11+
@Documented
12+
public @interface Autowired {
13+
boolean required() default true;
14+
}

0 commit comments

Comments
 (0)