Skip to content

Commit d7be27a

Browse files
RasmusWLRasmusWL
committed
Python: Fix experimental py/ip-address-spoofing
I realized the modeling was done in a non-recommended way, so I changed the modeling. It was very nice that I could use API graphs for the flask part, and a little sad when I couldn't for Django/Tornado.
1 parent b01a0ae commit d7be27a

File tree

3 files changed

+45
-52
lines changed

3 files changed

+45
-52
lines changed

‎python/ql/lib/semmle/python/frameworks/Tornado.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@ private import semmle.python.frameworks.Stdlib
1414
private import semmle.python.frameworks.internal.InstanceTaintStepsHelper
1515

1616
/**
17+
* INTERNAL: Do not use.
18+
*
1719
* Provides models for the `tornado` PyPI package.
1820
* See https://www.tornadoweb.org/en/stable/.
1921
*/
20-
private module Tornado {
22+
module Tornado {
2123
/**
2224
* Provides models for the `tornado.httputil.HTTPHeaders` class
2325
*
@@ -126,8 +128,7 @@ private module Tornado {
126128
abstract class InstanceSource extends DataFlow::LocalSourceNode { }
127129

128130
/** The `self` parameter in a method on the `tornado.web.RequestHandler` class or any subclass. */
129-
private class SelfParam extends InstanceSource, RemoteFlowSource::Range,
130-
DataFlow::ParameterNode {
131+
class SelfParam extends InstanceSource, RemoteFlowSource::Range, DataFlow::ParameterNode {
131132
SelfParam() {
132133
exists(RequestHandlerClass cls | cls.getAMethod().getArg(0) = this.getParameter())
133134
}

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

Lines changed: 33 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,73 +1,57 @@
11
private import python
22
private import semmle.python.Concepts
33
private import semmle.python.ApiGraphs
4-
private import semmle.python.dataflow.new.RemoteFlowSources
4+
private import semmle.python.frameworks.Flask
5+
private import semmle.python.frameworks.Django
6+
private import semmle.python.frameworks.Tornado
57

68
/**
79
* A data flow source of the client ip obtained according to the remote endpoint identifier specified
810
* (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
911
*
1012
* For example: `request.headers.get("X-Forwarded-For")`.
13+
*
14+
* A call to `request.headers.get` or `request.headers.get_all` or `request.headers.getlist`.
1115
*/
12-
abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::CallCfgNode { }
16+
abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node { }
1317

14-
private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
18+
private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
19+
DataFlow::MethodCallNode {
1520
FlaskClientSuppliedIpUsedInSecurityCheck() {
16-
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
17-
rfs.getSourceType() = "flask.request" and this.getFunction() = get
18-
|
19-
// `get` is a call to request.headers.get or request.headers.get_all or request.headers.getlist
20-
// request.headers
21-
get.getObject()
22-
.(DataFlow::AttrRead)
23-
// request
24-
.getObject()
25-
.getALocalSource() = rfs and
26-
get.getAttributeName() in ["get", "get_all", "getlist"] and
27-
get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
28-
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
29-
)
21+
this = Flask::request().getMember("headers").getMember(["get", "get_all", "getlist"]).getACall() and
22+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
3023
}
3124
}
3225

33-
private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
26+
private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
27+
DataFlow::MethodCallNode {
3428
DjangoClientSuppliedIpUsedInSecurityCheck() {
35-
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
36-
rfs.getSourceType() = "django.http.request.HttpRequest" and this.getFunction() = get
37-
|
38-
// `get` is a call to request.headers.get or request.META.get
39-
// request.headers
40-
get.getObject()
41-
.(DataFlow::AttrRead)
42-
// request
43-
.getObject()
44-
.getALocalSource() = rfs and
45-
get.getAttributeName() = "get" and
46-
get.getObject().(DataFlow::AttrRead).getAttributeName() in ["headers", "META"] and
47-
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
48-
)
29+
exists(DataFlow::Node req, DataFlow::AttrRead headers |
30+
// a call to request.headers.get or request.META.get
31+
req = PrivateDjango::DjangoImpl::DjangoHttp::Request::HttpRequest::instance() and
32+
headers.getObject().getALocalSource() = req and
33+
headers.getAttributeName() in ["headers", "META"] and
34+
this.calls(headers, "get")
35+
) and
36+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
4937
}
5038
}
5139

52-
private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck {
40+
private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
41+
DataFlow::MethodCallNode {
5342
TornadoClientSuppliedIpUsedInSecurityCheck() {
54-
exists(RemoteFlowSource rfs, DataFlow::AttrRead get |
55-
rfs.getSourceType() = "tornado.web.RequestHandler" and this.getFunction() = get
43+
// a call to self.request.headers.get or self.request.headers.get_list inside a tornado requesthandler
44+
exists(
45+
Tornado::TornadoModule::Web::RequestHandler::SelfParam selfParam, DataFlow::AttrRead headers,
46+
DataFlow::AttrRead req
5647
|
57-
// `get` is a call to `rfs`.request.headers.get
58-
// `rfs`.request.headers
59-
get.getObject()
60-
.(DataFlow::AttrRead)
61-
// `rfs`.request
62-
.getObject()
63-
.(DataFlow::AttrRead)
64-
// `rfs`
65-
.getObject()
66-
.getALocalSource() = rfs and
67-
get.getAttributeName() in ["get", "get_list"] and
68-
get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and
69-
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
70-
)
48+
req.getObject().getALocalSource() = selfParam and
49+
req.getAttributeName() = "request" and
50+
headers.getObject().getALocalSource() = req and
51+
headers.getAttributeName() = "headers" and
52+
this.calls(headers, ["get", "get_list"])
53+
) and
54+
this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
7155
}
7256
}
7357

‎python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,16 @@
11
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 |
24
| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip |
35
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 |
410
| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
511
| tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip |
612
subpaths
713
#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 |
816
| 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 |

0 commit comments

Comments
 (0)