Skip to content

Commit a88ad62

Browse files
joefarebrotherjoefarebrother
committed
Implemented sinks for bulk header updates, and added corresponding tests.
1 parent 3e9341f commit a88ad62

File tree

7 files changed

+158
-50
lines changed

7 files changed

+158
-50
lines changed

‎python/ql/lib/semmle/python/Concepts.qll

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,6 +1084,55 @@ module Http {
10841084
}
10851085
}
10861086

1087+
/**
1088+
* A data-flow node that sets multiple headers in an HTTP response using a dict.
1089+
*
1090+
* Extend this class to model new APIs. If you want to refine existing API models,
1091+
* extend `ResponseHeaderBulkWrite::Range` instead.
1092+
*/
1093+
class ResponseHeaderBulkWrite extends DataFlow::Node instanceof ResponseHeaderBulkWrite::Range {
1094+
/**
1095+
* Gets the argument containing the headers dictionary.
1096+
*/
1097+
DataFlow::Node geBulkArg() { result = super.getBulkArg() }
1098+
1099+
/**
1100+
* Holds if newlines are accepted in the header name argument.
1101+
*/
1102+
predicate nameAllowsNewline() { super.nameAllowsNewline() }
1103+
1104+
/**
1105+
* Holds if newlines are accepted in the header value argument.
1106+
*/
1107+
predicate valueAllowsNewline() { super.valueAllowsNewline() }
1108+
}
1109+
1110+
/** Provides a class for modelling bulk header writes on HTTP responses. */
1111+
module ResponseHeaderBulkWrite {
1112+
/**
1113+
*sets multiple headers in an HTTP response using a dict.
1114+
*
1115+
* Extend this class to model new APIs. If you want to refine existing API models,
1116+
* extend `ResponseHeaderBulkWrite` instead.
1117+
*/
1118+
abstract class Range extends DataFlow::Node {
1119+
/**
1120+
* Gets the argument containing the headers dictionary.
1121+
*/
1122+
abstract DataFlow::Node getBulkArg();
1123+
1124+
/**
1125+
* Holds if newlines are accepted in the header name argument.
1126+
*/
1127+
abstract predicate nameAllowsNewline();
1128+
1129+
/**
1130+
* Holds if newlines are accepted in the header value argument.
1131+
*/
1132+
abstract predicate valueAllowsNewline();
1133+
}
1134+
}
1135+
10871136
/**
10881137
* A data-flow node that sets a cookie in an HTTP response.
10891138
*

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

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -222,14 +222,41 @@ module Flask {
222222
DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
223223

224224
/** An `Headers` instance that is part of a Flask response. */
225-
private class FlaskResponseHeadersInstances extends Werkzeug::Headers::InstanceSource
226-
{
225+
private class FlaskResponseHeadersInstances extends Werkzeug::Headers::InstanceSource {
227226
FlaskResponseHeadersInstances() {
228227
this.(DataFlow::AttrRead).getObject() = instance() and
229228
this.(DataFlow::AttrRead).getAttributeName() = "headers"
230229
}
231230
}
232-
// TODO: headers arg to make_response
231+
232+
/** A class instantiation of `Response` that sets response headers. */
233+
private class ResponseClassHeadersWrite extends Http::Server::ResponseHeaderBulkWrite::Range,
234+
ClassInstantiation
235+
{
236+
override DataFlow::Node getBulkArg() {
237+
result = [this.getArg(2), this.getArgByName("headers")]
238+
}
239+
240+
override predicate nameAllowsNewline() { any() }
241+
242+
override predicate valueAllowsNewline() { none() }
243+
}
244+
245+
/** A call to `make_response that sets response headers. */
246+
private class MakeResponseHeadersWrite extends Http::Server::ResponseHeaderBulkWrite::Range,
247+
FlaskMakeResponseCall
248+
{
249+
override DataFlow::Node getBulkArg() {
250+
result = this.getArg(2)
251+
or
252+
strictcount(this.getArg(_)) = 2 and
253+
result = this.getArg(1)
254+
}
255+
256+
override predicate nameAllowsNewline() { any() }
257+
258+
override predicate valueAllowsNewline() { none() }
259+
}
233260
}
234261

235262
// ---------------------------------------------------------------------------

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,9 +237,21 @@ module Werkzeug {
237237

238238
override predicate valueAllowsNewline() { none() }
239239
}
240+
241+
/** A call to `Headers.extend`, assumed to be a response header. */
242+
private class HeaderExtendCall extends Http::Server::ResponseHeaderBulkWrite::Range,
243+
DataFlow::MethodCallNode
244+
{
245+
HeaderExtendCall() { this.calls(instance(), "extend") }
246+
247+
override DataFlow::Node getBulkArg() { result = this.getArg(0) }
248+
249+
override predicate nameAllowsNewline() { any() }
250+
251+
override predicate valueAllowsNewline() { none() }
252+
}
240253
}
241254

242-
// TODO: `extend` bulk header update
243255
/**
244256
* Provides models for the `werkzeug.datastructures.Authorization` class
245257
*

‎python/ql/lib/semmle/python/security/dataflow/HttpHeaderInjectionCustomizations.qll

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,25 @@ module HttpHeaderInjection {
5050
)
5151
}
5252
}
53+
54+
/** A key-value pair in a literal for a bulk header update, considered as a single header update. */
55+
// TODO: We could instead consider bulk writes as sinks with an implicit read step of DictionaryKey/DictionaryValue content as needed.
56+
private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
57+
{
58+
KeyValuePair item;
59+
60+
HeaderBulkWriteDictLiteral() { item = super.geBulkArg().asExpr().(Dict).getAnItem() }
61+
62+
override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
63+
64+
override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
65+
66+
override predicate nameAllowsNewline() {
67+
Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
68+
}
69+
70+
override predicate valueAllowsNewline() {
71+
Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
72+
}
73+
}
5374
}

‎python/ql/src/experimental/semmle/python/frameworks/Flask.qll

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -11,50 +11,6 @@ private import semmle.python.ApiGraphs
1111
private import semmle.python.frameworks.Flask
1212

1313
module ExperimentalFlask {
14-
/**
15-
* A reference to either `flask.make_response` function, or the `make_response` method on
16-
* an instance of `flask.Flask`. This creates an instance of the `flask_response`
17-
* class (class-attribute on a flask application), which by default is
18-
* `flask.Response`.
19-
*
20-
* See
21-
* - https://flask.palletsprojects.com/en/1.1.x/api/#flask.Flask.make_response
22-
* - https://flask.palletsprojects.com/en/1.1.x/api/#flask.make_response
23-
*/
24-
private API::Node flaskMakeResponse() {
25-
result =
26-
[API::moduleImport("flask"), Flask::FlaskApp::instance()]
27-
.getMember(["make_response", "jsonify", "make_default_options_response"])
28-
}
29-
30-
/** Gets a reference to a header instance. */
31-
private DataFlow::LocalSourceNode headerInstance() {
32-
result =
33-
[Flask::Response::classRef(), flaskMakeResponse()]
34-
.getReturn()
35-
.getAMember()
36-
.getAValueReachableFromSource()
37-
}
38-
39-
/** Gets a reference to a header instance call/subscript */
40-
private DataFlow::Node headerInstanceCall() {
41-
headerInstance() in [result.(DataFlow::AttrRead), result.(DataFlow::AttrRead).getObject()] or
42-
headerInstance().asExpr() = result.asExpr().(Subscript).getObject()
43-
}
44-
45-
private class FlaskResponse extends DataFlow::CallCfgNode, HeaderDeclaration::Range {
46-
KeyValuePair item;
47-
48-
FlaskResponse() {
49-
this = Flask::Response::classRef().getACall() and
50-
item = this.getArg(_).asExpr().(Dict).getAnItem()
51-
}
52-
53-
override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
54-
55-
override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
56-
}
57-
5814
/**
5915
* Gets a call to `set_cookie()`.
6016
*

‎python/ql/test/query-tests/Security/CWE-113-HeaderInjection/HeaderInjection.expected

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,27 @@ edges
33
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:9:18:9:24 | ControlFlowNode for request | provenance | |
44
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:20:18:20:24 | ControlFlowNode for request | provenance | |
55
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:29:18:29:24 | ControlFlowNode for request | provenance | |
6+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:38:18:38:24 | ControlFlowNode for request | provenance | |
7+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:49:44:49:50 | ControlFlowNode for request | provenance | |
8+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:49:72:49:78 | ControlFlowNode for request | provenance | |
9+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:53:18:53:24 | ControlFlowNode for request | provenance | |
10+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:54:41:54:47 | ControlFlowNode for request | provenance | |
11+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:59:18:59:24 | ControlFlowNode for request | provenance | |
12+
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | flask_tests.py:60:36:60:42 | ControlFlowNode for request | provenance | |
613
| flask_tests.py:9:5:9:14 | ControlFlowNode for rfs_header | flask_tests.py:13:17:13:26 | ControlFlowNode for rfs_header | provenance | |
714
| flask_tests.py:9:18:9:24 | ControlFlowNode for request | flask_tests.py:9:5:9:14 | ControlFlowNode for rfs_header | provenance | |
815
| flask_tests.py:20:5:20:14 | ControlFlowNode for rfs_header | flask_tests.py:23:22:23:31 | ControlFlowNode for rfs_header | provenance | |
916
| flask_tests.py:20:18:20:24 | ControlFlowNode for request | flask_tests.py:20:5:20:14 | ControlFlowNode for rfs_header | provenance | |
1017
| flask_tests.py:29:5:29:14 | ControlFlowNode for rfs_header | flask_tests.py:32:22:32:31 | ControlFlowNode for rfs_header | provenance | |
1118
| flask_tests.py:29:18:29:24 | ControlFlowNode for request | flask_tests.py:29:5:29:14 | ControlFlowNode for rfs_header | provenance | |
19+
| flask_tests.py:38:5:38:14 | ControlFlowNode for rfs_header | flask_tests.py:43:10:43:19 | ControlFlowNode for rfs_header | provenance | |
20+
| flask_tests.py:38:18:38:24 | ControlFlowNode for request | flask_tests.py:38:5:38:14 | ControlFlowNode for rfs_header | provenance | |
21+
| flask_tests.py:49:44:49:50 | ControlFlowNode for request | flask_tests.py:49:72:49:97 | ControlFlowNode for Subscript | provenance | |
22+
| flask_tests.py:49:72:49:78 | ControlFlowNode for request | flask_tests.py:49:72:49:97 | ControlFlowNode for Subscript | provenance | |
23+
| flask_tests.py:53:18:53:24 | ControlFlowNode for request | flask_tests.py:54:41:54:66 | ControlFlowNode for Subscript | provenance | |
24+
| flask_tests.py:54:41:54:47 | ControlFlowNode for request | flask_tests.py:54:41:54:66 | ControlFlowNode for Subscript | provenance | |
25+
| flask_tests.py:59:18:59:24 | ControlFlowNode for request | flask_tests.py:60:36:60:61 | ControlFlowNode for Subscript | provenance | |
26+
| flask_tests.py:60:36:60:42 | ControlFlowNode for request | flask_tests.py:60:36:60:61 | ControlFlowNode for Subscript | provenance | |
1227
nodes
1328
| flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
1429
| flask_tests.py:1:29:1:35 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
@@ -21,8 +36,24 @@ nodes
2136
| flask_tests.py:29:5:29:14 | ControlFlowNode for rfs_header | semmle.label | ControlFlowNode for rfs_header |
2237
| flask_tests.py:29:18:29:24 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
2338
| flask_tests.py:32:22:32:31 | ControlFlowNode for rfs_header | semmle.label | ControlFlowNode for rfs_header |
39+
| flask_tests.py:38:5:38:14 | ControlFlowNode for rfs_header | semmle.label | ControlFlowNode for rfs_header |
40+
| flask_tests.py:38:18:38:24 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
41+
| flask_tests.py:43:10:43:19 | ControlFlowNode for rfs_header | semmle.label | ControlFlowNode for rfs_header |
42+
| flask_tests.py:49:44:49:50 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
43+
| flask_tests.py:49:72:49:78 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
44+
| flask_tests.py:49:72:49:97 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
45+
| flask_tests.py:53:18:53:24 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
46+
| flask_tests.py:54:41:54:47 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
47+
| flask_tests.py:54:41:54:66 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
48+
| flask_tests.py:59:18:59:24 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
49+
| flask_tests.py:60:36:60:42 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
50+
| flask_tests.py:60:36:60:61 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
2451
subpaths
2552
#select
2653
| flask_tests.py:13:17:13:26 | ControlFlowNode for rfs_header | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:13:17:13:26 | ControlFlowNode for rfs_header | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
2754
| flask_tests.py:23:22:23:31 | ControlFlowNode for rfs_header | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:23:22:23:31 | ControlFlowNode for rfs_header | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
2855
| flask_tests.py:32:22:32:31 | ControlFlowNode for rfs_header | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:32:22:32:31 | ControlFlowNode for rfs_header | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
56+
| flask_tests.py:43:10:43:19 | ControlFlowNode for rfs_header | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:43:10:43:19 | ControlFlowNode for rfs_header | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
57+
| flask_tests.py:49:72:49:97 | ControlFlowNode for Subscript | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:49:72:49:97 | ControlFlowNode for Subscript | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
58+
| flask_tests.py:54:41:54:66 | ControlFlowNode for Subscript | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:54:41:54:66 | ControlFlowNode for Subscript | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |
59+
| flask_tests.py:60:36:60:61 | ControlFlowNode for Subscript | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | flask_tests.py:60:36:60:61 | ControlFlowNode for Subscript | This HTTP header is constructed from a $@. | flask_tests.py:1:29:1:35 | ControlFlowNode for ImportMember | user-provided value |

‎python/ql/test/query-tests/Security/CWE-113-HeaderInjection/flask_tests.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,22 @@ def flask_make_response_extend():
4040
resp.headers.extend(
4141
{'HeaderName': rfs_header}) # GOOD
4242
resp.headers.extend(
43-
{rfs_header: "HeaderValue"}) # BAD but not yet found
43+
{rfs_header: "HeaderValue"}) # BAD
4444
return resp
4545

4646

4747
@app.route("/Response_arg")
4848
def Response_arg():
49-
return Response(headers={'HeaderName': request.args["rfs_header"], request.args["rfs_header"]: "HeaderValue"}) # BAD but not yet found
49+
return Response(headers={'HeaderName': request.args["rfs_header"], request.args["rfs_header"]: "HeaderValue"}) # BAD
50+
51+
@app.route("/flask_make_response_header_arg3")
52+
def flask_make_response_header_arg3():
53+
rfs_header = request.args["rfs_header"]
54+
resp = make_response("hello", 200, {request.args["rfs_header"]: "HeaderValue"}) # BAD
55+
return resp
56+
57+
@app.route("/flask_make_response_header_arg2")
58+
def flask_make_response_header_arg2():
59+
rfs_header = request.args["rfs_header"]
60+
resp = make_response("hello", {request.args["rfs_header"]: "HeaderValue"}) # BAD
61+
return resp

0 commit comments

Comments
 (0)