Skip to content

Commit 0629ce0

Browse files
yoffyoff
authored
Merge pull request github#6214 from haby0/python/ClientSuppliedIpUsedInSecurityCheck
[Python] CWE-348: Client supplied ip used in security check
2 parents 058a04f + c2d0fcf commit 0629ce0

9 files changed

+379
-0
lines changed

‎python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#!/usr/bin/env python
2+
# -*- coding: UTF-8 -*-
3+
"""
4+
@Desc ：ip address spoofing
5+
"""
6+
from flask import Flask
7+
from flask import request
8+
9+
app = Flask(__name__)
10+
11+
@app.route('/bad1')
12+
def bad1():
13+
client_ip = request.headers.get('x-forwarded-for')
14+
if not client_ip.startswith('192.168.'):
15+
raise Exception('ip illegal')
16+
return 'bad1'
17+
18+
@app.route('/bad2')
19+
def bad2():
20+
client_ip = request.headers.get('x-forwarded-for')
21+
if not client_ip == '127.0.0.1':
22+
raise Exception('ip illegal')
23+
return 'bad2'
24+
25+
@app.route('/good1')
26+
def good1():
27+
client_ip = request.headers.get('x-forwarded-for')
28+
client_ip = client_ip.split(',')[client_ip.split(',').length - 1]
29+
if not client_ip == '127.0.0.1':
30+
raise Exception('ip illegal')
31+
return 'good1'
32+
33+
if __name__ == '__main__':
34+
app.debug = True
35+
app.run()

‎python/ql/src/experimental/Security/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.py" />
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>

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
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 py/ip-address-spoofing
9+
* @tags security
10+
* external/cwe/cwe-348
11+
*/
12+
13+
import python
14+
import semmle.python.dataflow.new.DataFlow
15+
import semmle.python.dataflow.new.TaintTracking
16+
import semmle.python.ApiGraphs
17+
import ClientSuppliedIpUsedInSecurityCheckLib
18+
import DataFlow::PathGraph
19+
20+
/**
21+
* Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use.
22+
*/
23+
class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration {
24+
ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" }
25+
26+
override predicate isSource(DataFlow::Node source) {
27+
source instanceof ClientSuppliedIpUsedInSecurityCheck
28+
}
29+
30+
override predicate isSink(DataFlow::Node sink) { sink instanceof PossibleSecurityCheck }
31+
32+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
33+
exists(DataFlow::CallCfgNode ccn |
34+
ccn = API::moduleImport("netaddr").getMember("IPAddress").getACall() and
35+
ccn.getArg(0) = pred and
36+
ccn = succ
37+
)
38+
}
39+
40+
override predicate isSanitizer(DataFlow::Node node) {
41+
// `client_supplied_ip.split(",")[n]` for `n` > 0
42+
exists(Subscript ss |
43+
not ss.getIndex().(IntegerLiteral).getText() = "0" and
44+
ss.getObject().(Call).getFunc().(Attribute).getName() = "split" and
45+
ss.getObject().(Call).getAnArg().(StrConst).getText() = "," and
46+
ss = node.asExpr()
47+
)
48+
}
49+
}
50+
51+
from
52+
ClientSuppliedIpUsedInSecurityCheckConfig config, DataFlow::PathNode source,
53+
DataFlow::PathNode sink
54+
where config.hasFlowPath(source, sink)
55+
select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
56+
source.getNode(), "this user input"

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

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
private import python
2+
private import semmle.python.Concepts
3+
private import semmle.python.ApiGraphs
4+
private import semmle.python.dataflow.new.DataFlow
5+
private import semmle.python.dataflow.new.RemoteFlowSources
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: `request.headers.get("X-Forwarded-For")`.
12+
*/
13+
abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::CallCfgNode { }
14+
15+
private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
16+
FlaskClientSuppliedIpUsedInSecurityCheck() {
17+
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
18+
rfs.getSourceType() = "flask.request" and this.getFunction() = get
19+
|
20+
// `get` is a call to request.headers.get or request.headers.get_all or request.headers.getlist
21+
// request.headers
22+
get.getObject()
23+
.(DataFlow::AttrRead)
24+
// request
25+
.getObject()
26+
.getALocalSource() = rfs and
27+
get.getAttributeName() in ["get", "get_all", "getlist"] and
28+
get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
29+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
30+
)
31+
}
32+
}
33+
34+
private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
35+
DjangoClientSuppliedIpUsedInSecurityCheck() {
36+
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
37+
rfs.getSourceType() = "django.http.request.HttpRequest" and this.getFunction() = get
38+
|
39+
// `get` is a call to request.headers.get or request.META.get
40+
// request.headers
41+
get.getObject()
42+
.(DataFlow::AttrRead)
43+
// request
44+
.getObject()
45+
.getALocalSource() = rfs and
46+
get.getAttributeName() = "get" and
47+
get.getObject().(DataFlow::AttrRead).getAttributeName() in ["headers", "META"] and
48+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
49+
)
50+
}
51+
}
52+
53+
private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
54+
TornadoClientSuppliedIpUsedInSecurityCheck() {
55+
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
56+
rfs.getSourceType() = "tornado.web.RequestHandler" and this.getFunction() = get
57+
|
58+
// `get` is a call to `rfs`.request.headers.get
59+
// `rfs`.request.headers
60+
get.getObject()
61+
.(DataFlow::AttrRead)
62+
// `rfs`.request
63+
.getObject()
64+
.(DataFlow::AttrRead)
65+
// `rfs`
66+
.getObject()
67+
.getALocalSource() = rfs and
68+
get.getAttributeName() in ["get", "get_list"] and
69+
get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
70+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
71+
)
72+
}
73+
}
74+
75+
private string clientIpParameterName() {
76+
result in [
77+
"x-forwarded-for", "x_forwarded_for", "x-real-ip", "x_real_ip", "proxy-client-ip",
78+
"proxy_client_ip", "wl-proxy-client-ip", "wl_proxy_client_ip", "http_x_forwarded_for",
79+
"http-x-forwarded-for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip",
80+
"http_forwarded_for", "http_forwarded", "http_via", "remote_addr"
81+
]
82+
}
83+
84+
/** A data flow sink for ip address forgery vulnerabilities. */
85+
abstract class PossibleSecurityCheck extends DataFlow::Node { }
86+
87+
/** A data flow sink for sql operation. */
88+
private class SqlOperationAsSecurityCheck extends PossibleSecurityCheck {
89+
SqlOperationAsSecurityCheck() { this = any(SqlExecution e).getSql() }
90+
}
91+
92+
/**
93+
* A data flow sink for remote client ip comparison.
94+
*
95+
* For example: `if not ipAddr.startswith('192.168.') : ...` determine whether the client ip starts
96+
* with `192.168.`, and the program can be deceived by forging the ip address.
97+
*/
98+
private class CompareSink extends PossibleSecurityCheck {
99+
CompareSink() {
100+
exists(Call call |
101+
call.getFunc().(Attribute).getName() = "startswith" and
102+
call.getArg(0).(StrConst).getText().regexpMatch(getIpAddressRegex()) and
103+
not call.getArg(0).(StrConst).getText() = "0:0:0:0:0:0:0:1" and
104+
call.getFunc().(Attribute).getObject() = this.asExpr()
105+
)
106+
or
107+
exists(Compare compare |
108+
(
109+
compare.getOp(0) instanceof Eq or
110+
compare.getOp(0) instanceof NotEq
111+
) and
112+
(
113+
compare.getLeft() = this.asExpr() and
114+
compare.getComparator(0).(StrConst).getText() instanceof PrivateHostName and
115+
not compare.getComparator(0).(StrConst).getText() = "0:0:0:0:0:0:0:1"
116+
or
117+
compare.getComparator(0) = this.asExpr() and
118+
compare.getLeft().(StrConst).getText() instanceof PrivateHostName and
119+
not compare.getLeft().(StrConst).getText() = "0:0:0:0:0:0:0:1"
120+
)
121+
)
122+
or
123+
exists(Compare compare |
124+
(
125+
compare.getOp(0) instanceof In or
126+
compare.getOp(0) instanceof NotIn
127+
) and
128+
(
129+
compare.getLeft() = this.asExpr()
130+
or
131+
compare.getComparator(0) = this.asExpr() and
132+
not compare.getLeft().(StrConst).getText() in ["%", ",", "."]
133+
)
134+
)
135+
}
136+
}
137+
138+
string getIpAddressRegex() {
139+
result =
140+
"^((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])?)$"
141+
}
142+
143+
/**
144+
* A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary.
145+
* 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]
146+
*/
147+
private class PrivateHostName extends string {
148+
bindingset[this]
149+
PrivateHostName() {
150+
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\\]?(?:[:/?#].*)?")
151+
}
152+
}

‎python/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+
| flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip |
3+
| flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip |
4+
| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip |
5+
nodes
6+
| flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
7+
| flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip |
8+
| flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
9+
| flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip |
10+
| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
11+
| tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip |
12+
subpaths
13+
#select
14+
| flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | this user input |
15+
| flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | this user input |
16+
| tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | this user input |

‎python/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-348/ClientSuppliedIpUsedInSecurityCheck.ql

‎python/ql/test/experimental/query-tests/Security/CWE-348/flask_bad.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
#!/usr/bin/env python
2+
# -*- coding: UTF-8 -*-
3+
"""
4+
@Desc ：ip address spoofing
5+
"""
6+
from flask import Flask
7+
from flask import request
8+
9+
app = Flask(__name__)
10+
11+
@app.route('/bad1')
12+
def bad1():
13+
client_ip = request.headers.get('x-forwarded-for')
14+
if not client_ip.startswith('192.168.'):
15+
raise Exception('ip illegal')
16+
return 'bad1'
17+
18+
@app.route('/bad2')
19+
def bad2():
20+
client_ip = request.headers.get('x-forwarded-for')
21+
if not client_ip == '127.0.0.1':
22+
raise Exception('ip illegal')
23+
return 'bad2'
24+
25+
if __name__ == '__main__':
26+
app.debug = True
27+
app.run()

‎python/ql/test/experimental/query-tests/Security/CWE-348/flask_good.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#!/usr/bin/env python
2+
# -*- coding: UTF-8 -*-
3+
"""
4+
@Desc ：ip address spoofing
5+
"""
6+
from flask import Flask
7+
from flask import request
8+
9+
app = Flask(__name__)
10+
11+
@app.route('/good1')
12+
def good1():
13+
client_ip = request.headers.get('x-forwarded-for')
14+
client_ip = client_ip.split(',')[len(client_ip.split(',')) - 1]
15+
if not client_ip == '127.0.0.1':
16+
raise Exception('ip illegal')
17+
return 'good1'
18+
19+
if __name__ == '__main__':
20+
app.debug = True
21+
app.run()

‎python/ql/test/experimental/query-tests/Security/CWE-348/tornado_bad.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#!/usr/bin/env python
2+
# -*- coding: UTF-8 -*-
3+
"""
4+
@Desc ：ip address spoofing
5+
"""
6+
import tornado.httpserver
7+
import tornado.options
8+
import tornado.web
9+
import tornado.ioloop
10+
11+
from tornado.options import define, options
12+
13+
define("port", default=8000, help="run on the given port,default 8000", type=int)
14+
15+
16+
class IndexHandler(tornado.web.RequestHandler):
17+
def get(self):
18+
client_ip = self.request.headers.get('x-forwarded-for')
19+
if client_ip:
20+
client_ip = client_ip.split(',')[len(client_ip.split(',')) - 1]
21+
else:
22+
client_ip = self.request.headers.get('REMOTE_ADDR', None)
23+
if not client_ip == '127.0.0.1':
24+
raise Exception('ip illegal')
25+
self.write("hello.")
26+
27+
handlers = [(r"/", IndexHandler)]
28+
29+
if __name__ == "__main__":
30+
tornado.options.parse_command_line()
31+
app = tornado.web.Application(
32+
handlers
33+
)
34+
http_server = tornado.httpserver.HTTPServer(app)
35+
http_server.listen(options.port)
36+
tornado.ioloop.IOLoop.instance().start()

0 commit comments

Comments
 (0)