Skip to content

Commit 8152ec7

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #16696 from joefarebrother/python-cookie-write-headers
Python: Model CookieWrites from HeaderWrites
2 parents 0b6714e + b81d41b commit 8152ec7

File tree

19 files changed

+349
-118
lines changed

19 files changed

+349
-118
lines changed

‎python/ql/lib/change-notes/2024-06-24-cookie-header-writes.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Additional modelling has been added to detect cookie writes from direct writes to the `Set-Cookie` header have been added for several web frameworks.

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,54 @@ module Http {
11341134
}
11351135
}
11361136

1137+
/** A key-value pair in a literal for a bulk header update, considered as a single header update. */
1138+
private class HeaderBulkWriteDictLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
1139+
{
1140+
KeyValuePair item;
1141+
1142+
HeaderBulkWriteDictLiteral() {
1143+
exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) |
1144+
item = dict.getAnItem()
1145+
)
1146+
}
1147+
1148+
override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
1149+
1150+
override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
1151+
1152+
override predicate nameAllowsNewline() {
1153+
Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
1154+
}
1155+
1156+
override predicate valueAllowsNewline() {
1157+
Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
1158+
}
1159+
}
1160+
1161+
/** A tuple in a list for a bulk header update, considered as a single header update. */
1162+
private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
1163+
{
1164+
Tuple item;
1165+
1166+
HeaderBulkWriteListLiteral() {
1167+
exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) |
1168+
item = list.getAnElt()
1169+
)
1170+
}
1171+
1172+
override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) }
1173+
1174+
override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) }
1175+
1176+
override predicate nameAllowsNewline() {
1177+
Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
1178+
}
1179+
1180+
override predicate valueAllowsNewline() {
1181+
Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
1182+
}
1183+
}
1184+
11371185
/**
11381186
* A data-flow node that sets a cookie in an HTTP response.
11391187
*
@@ -1186,6 +1234,27 @@ module Http {
11861234
}
11871235
}
11881236

1237+
/** A write to a `Set-Cookie` header that sets a cookie directly. */
1238+
private class CookieHeaderWrite extends CookieWrite::Range instanceof Http::Server::ResponseHeaderWrite
1239+
{
1240+
CookieHeaderWrite() {
1241+
exists(StringLiteral str |
1242+
str.getText().toLowerCase() = "set-cookie" and
1243+
DataFlow::exprNode(str)
1244+
.(DataFlow::LocalSourceNode)
1245+
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getNameArg())
1246+
)
1247+
}
1248+
1249+
override DataFlow::Node getNameArg() { none() }
1250+
1251+
override DataFlow::Node getHeaderArg() {
1252+
result = this.(Http::Server::ResponseHeaderWrite).getValueArg()
1253+
}
1254+
1255+
override DataFlow::Node getValueArg() { none() }
1256+
}
1257+
11891258
/**
11901259
* A data-flow node that enables or disables Cross-site request forgery protection
11911260
* in a global manner.

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,33 @@ module AiohttpWebModel {
706706

707707
override DataFlow::Node getValueArg() { result = value }
708708
}
709+
710+
/**
711+
* A dict-like write to an item of the `headers` attribute on a HTTP response, such as
712+
* `response.headers[name] = value`.
713+
*/
714+
class AiohttpResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
715+
DataFlow::Node index;
716+
DataFlow::Node value;
717+
718+
AiohttpResponseHeaderSubscriptWrite() {
719+
exists(API::Node i |
720+
value = aiohttpResponseInstance().getMember("headers").getSubscriptAt(i).asSink() and
721+
index = i.asSink() and
722+
// To give `this` a value, we need to choose between either LHS or RHS,
723+
// and just go with the RHS as it is readily available
724+
this = value
725+
)
726+
}
727+
728+
override DataFlow::Node getNameArg() { result = index }
729+
730+
override DataFlow::Node getValueArg() { result = value }
731+
732+
override predicate nameAllowsNewline() { none() }
733+
734+
override predicate valueAllowsNewline() { none() }
735+
}
709736
}
710737

711738
/**

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2239,6 +2239,71 @@ module PrivateDjango {
22392239

22402240
override DataFlow::Node getValueArg() { result = value }
22412241
}
2242+
2243+
/**
2244+
* A dict-like write to an item of the `headers` attribute on a HTTP response, such as
2245+
* `response.headers[name] = value`.
2246+
*/
2247+
class DjangoResponseHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
2248+
DataFlow::Node index;
2249+
DataFlow::Node value;
2250+
2251+
DjangoResponseHeaderSubscriptWrite() {
2252+
exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup |
2253+
// To give `this` a value, we need to choose between either LHS or RHS,
2254+
// and just go with the LHS
2255+
this.asCfgNode() = subscript
2256+
|
2257+
headerLookup
2258+
.accesses(DjangoImpl::DjangoHttp::Response::HttpResponse::instance(), "headers") and
2259+
exists(DataFlow::Node subscriptObj |
2260+
subscriptObj.asCfgNode() = subscript.getObject()
2261+
|
2262+
headerLookup.flowsTo(subscriptObj)
2263+
) and
2264+
value.asCfgNode() = subscript.(DefinitionNode).getValue() and
2265+
index.asCfgNode() = subscript.getIndex()
2266+
)
2267+
}
2268+
2269+
override DataFlow::Node getNameArg() { result = index }
2270+
2271+
override DataFlow::Node getValueArg() { result = value }
2272+
2273+
override predicate nameAllowsNewline() { none() }
2274+
2275+
override predicate valueAllowsNewline() { none() }
2276+
}
2277+
2278+
/**
2279+
* A dict-like write to an item of an HTTP response, which is treated as a header write,
2280+
* such as `response[headerName] = value`
2281+
*/
2282+
class DjangoResponseSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
2283+
DataFlow::Node index;
2284+
DataFlow::Node value;
2285+
2286+
DjangoResponseSubscriptWrite() {
2287+
exists(SubscriptNode subscript |
2288+
// To give `this` a value, we need to choose between either LHS or RHS,
2289+
// and just go with the LHS
2290+
this.asCfgNode() = subscript
2291+
|
2292+
subscript.getObject() =
2293+
DjangoImpl::DjangoHttp::Response::HttpResponse::instance().asCfgNode() and
2294+
value.asCfgNode() = subscript.(DefinitionNode).getValue() and
2295+
index.asCfgNode() = subscript.getIndex()
2296+
)
2297+
}
2298+
2299+
override DataFlow::Node getNameArg() { result = index }
2300+
2301+
override DataFlow::Node getValueArg() { result = value }
2302+
2303+
override predicate nameAllowsNewline() { none() }
2304+
2305+
override predicate valueAllowsNewline() { none() }
2306+
}
22422307
}
22432308
}
22442309

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

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -361,28 +361,59 @@ module FastApi {
361361
}
362362

363363
/**
364-
* A call to `append` on a `headers` of a FastAPI Response, with the `Set-Cookie`
365-
* header-key.
364+
* A call to `append` on a `headers` of a FastAPI Response.
366365
*/
367-
private class HeadersAppendCookie extends Http::Server::CookieWrite::Range,
366+
private class HeadersAppend extends Http::Server::ResponseHeaderWrite::Range,
368367
DataFlow::MethodCallNode
369368
{
370-
HeadersAppendCookie() {
371-
exists(DataFlow::AttrRead headers, DataFlow::Node keyArg |
369+
HeadersAppend() {
370+
exists(DataFlow::AttrRead headers |
372371
headers.accesses(instance(), "headers") and
373-
this.calls(headers, "append") and
374-
keyArg in [this.getArg(0), this.getArgByName("key")] and
375-
keyArg.getALocalSource().asExpr().(StringLiteral).getText().toLowerCase() = "set-cookie"
372+
this.calls(headers, "append")
376373
)
377374
}
378375

379-
override DataFlow::Node getHeaderArg() {
376+
override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("key")] }
377+
378+
override DataFlow::Node getValueArg() {
380379
result in [this.getArg(1), this.getArgByName("value")]
381380
}
382381

383-
override DataFlow::Node getNameArg() { none() }
382+
override predicate nameAllowsNewline() { none() }
383+
384+
override predicate valueAllowsNewline() { none() }
385+
}
386+
387+
/**
388+
* A dict-like write to an item of the `headers` attribute on a HTTP response, such as
389+
* `response.headers[name] = value`.
390+
*/
391+
class HeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
392+
DataFlow::Node index;
393+
DataFlow::Node value;
394+
395+
HeaderSubscriptWrite() {
396+
exists(SubscriptNode subscript, DataFlow::AttrRead headerLookup |
397+
// To give `this` a value, we need to choose between either LHS or RHS,
398+
// and just go with the LHS
399+
this.asCfgNode() = subscript
400+
|
401+
headerLookup.accesses(instance(), "headers") and
402+
exists(DataFlow::Node subscriptObj | subscriptObj.asCfgNode() = subscript.getObject() |
403+
headerLookup.flowsTo(subscriptObj)
404+
) and
405+
value.asCfgNode() = subscript.(DefinitionNode).getValue() and
406+
index.asCfgNode() = subscript.getIndex()
407+
)
408+
}
409+
410+
override DataFlow::Node getNameArg() { result = index }
411+
412+
override DataFlow::Node getValueArg() { result = value }
413+
414+
override predicate nameAllowsNewline() { none() }
384415

385-
override DataFlow::Node getValueArg() { none() }
416+
override predicate valueAllowsNewline() { none() }
386417
}
387418
}
388419
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,50 @@ module Tornado {
6363

6464
override string getAsyncMethodName() { none() }
6565
}
66+
67+
/**
68+
* A dict-like write to an item of an `HTTPHeaders` object.
69+
*/
70+
private class TornadoHeaderSubscriptWrite extends Http::Server::ResponseHeaderWrite::Range {
71+
DataFlow::Node index;
72+
DataFlow::Node value;
73+
74+
TornadoHeaderSubscriptWrite() {
75+
exists(SubscriptNode subscript |
76+
subscript.getObject() = instance().asCfgNode() and
77+
value.asCfgNode() = subscript.(DefinitionNode).getValue() and
78+
index.asCfgNode() = subscript.getIndex() and
79+
this.asCfgNode() = subscript
80+
)
81+
}
82+
83+
override DataFlow::Node getNameArg() { result = index }
84+
85+
override DataFlow::Node getValueArg() { result = value }
86+
87+
override predicate nameAllowsNewline() { none() }
88+
89+
override predicate valueAllowsNewline() { none() }
90+
}
91+
92+
/**
93+
* A call to `HTTPHeaders.add`.
94+
*/
95+
private class TornadoHeadersAppendCall extends Http::Server::ResponseHeaderWrite::Range,
96+
DataFlow::MethodCallNode
97+
{
98+
TornadoHeadersAppendCall() { this.calls(instance(), "add") }
99+
100+
override DataFlow::Node getNameArg() { result = [this.getArg(0), this.getArgByName("name")] }
101+
102+
override DataFlow::Node getValueArg() {
103+
result in [this.getArg(1), this.getArgByName("value")]
104+
}
105+
106+
override predicate nameAllowsNewline() { none() }
107+
108+
override predicate valueAllowsNewline() { none() }
109+
}
66110
}
67111

68112
// ---------------------------------------------------------------------------
@@ -209,6 +253,25 @@ module Tornado {
209253
this.(DataFlow::AttrRead).getAttributeName() = "request"
210254
}
211255
}
256+
257+
/** A call to `RequestHandler.set_header` or `RequestHandler.add_header` */
258+
private class TornadoSetHeaderCall extends Http::Server::ResponseHeaderWrite::Range,
259+
DataFlow::MethodCallNode
260+
{
261+
TornadoSetHeaderCall() { this.calls(instance(), ["set_header", "add_header"]) }
262+
263+
override DataFlow::Node getNameArg() {
264+
result = [this.getArg(0), this.getArgByName("name")]
265+
}
266+
267+
override DataFlow::Node getValueArg() {
268+
result in [this.getArg(1), this.getArgByName("value")]
269+
}
270+
271+
override predicate nameAllowsNewline() { none() }
272+
273+
override predicate valueAllowsNewline() { none() }
274+
}
212275
}
213276

214277
/**

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

Lines changed: 0 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -51,56 +51,6 @@ module HttpHeaderInjection {
5151
}
5252
}
5353

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() {
61-
exists(Dict dict | DataFlow::localFlow(DataFlow::exprNode(dict), super.getBulkArg()) |
62-
item = dict.getAnItem()
63-
)
64-
}
65-
66-
override DataFlow::Node getNameArg() { result.asExpr() = item.getKey() }
67-
68-
override DataFlow::Node getValueArg() { result.asExpr() = item.getValue() }
69-
70-
override predicate nameAllowsNewline() {
71-
Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
72-
}
73-
74-
override predicate valueAllowsNewline() {
75-
Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
76-
}
77-
}
78-
79-
/** A tuple in a list for a bulk header update, considered as a single header update. */
80-
// TODO: We could instead consider bulk writes as sinks with implicit read steps as needed.
81-
private class HeaderBulkWriteListLiteral extends Http::Server::ResponseHeaderWrite::Range instanceof Http::Server::ResponseHeaderBulkWrite
82-
{
83-
Tuple item;
84-
85-
HeaderBulkWriteListLiteral() {
86-
exists(List list | DataFlow::localFlow(DataFlow::exprNode(list), super.getBulkArg()) |
87-
item = list.getAnElt()
88-
)
89-
}
90-
91-
override DataFlow::Node getNameArg() { result.asExpr() = item.getElt(0) }
92-
93-
override DataFlow::Node getValueArg() { result.asExpr() = item.getElt(1) }
94-
95-
override predicate nameAllowsNewline() {
96-
Http::Server::ResponseHeaderBulkWrite.super.nameAllowsNewline()
97-
}
98-
99-
override predicate valueAllowsNewline() {
100-
Http::Server::ResponseHeaderBulkWrite.super.valueAllowsNewline()
101-
}
102-
}
103-
10454
/**
10555
* A call to replace line breaks, considered as a sanitizer.
10656
*/

0 commit comments

Comments
 (0)