Skip to content

Commit 6f7b2a2

Browse files
joefarebrotherjoefarebrother
committed
Add cookie flags to cookie write concept, and alter experimental queries to use them
1 parent ff8bb2b commit 6f7b2a2

7 files changed

Lines changed: 130 additions & 113 deletions

File tree

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,6 +1203,21 @@ module Http {
12031203
* Gets the argument, if any, specifying the cookie value.
12041204
*/
12051205
DataFlow::Node getValueArg() { result = super.getValueArg() }
1206+
1207+
/**
1208+
* Gets the value of the `Secure` flag of the cookie, if known.
1209+
*/
1210+
boolean getSecureFlag() { result = super.getSecureFlag() }
1211+
1212+
/**
1213+
* Gets the value of the `HttpOnly` flag of the cookie, if known
1214+
*/
1215+
boolean getHttpOnlyFlag() { result = super.getHttpOnlyFlag() }
1216+
1217+
/**
1218+
* Gets the value of the `SameSite` flag of the cookie, if known.
1219+
*/
1220+
boolean getSameSiteFlag() { result = super.getSameSiteFlag() }
12061221
}
12071222

12081223
/** Provides a class for modeling new cookie writes on HTTP responses. */
@@ -1231,6 +1246,21 @@ module Http {
12311246
* Gets the argument, if any, specifying the cookie value.
12321247
*/
12331248
abstract DataFlow::Node getValueArg();
1249+
1250+
/**
1251+
* Gets the value of the `Secure` flag of the cookie, if known.
1252+
*/
1253+
boolean getSecureFlag() { none() }
1254+
1255+
/**
1256+
* Gets the value of the `HttpOnly` flag of the cookie, if known
1257+
*/
1258+
boolean getHttpOnlyFlag() { none() }
1259+
1260+
/**
1261+
* Gets the value of the `SameSite` flag of the cookie, if known.
1262+
*/
1263+
boolean getSameSiteFlag() { none() }
12341264
}
12351265
}
12361266

‎python/ql/src/experimental/Security/CWE-614/InsecureCookie.ql‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,17 @@
1616
import python
1717
import semmle.python.dataflow.new.DataFlow
1818
import experimental.semmle.python.Concepts
19+
import semmle.python.Concepts
1920
import experimental.semmle.python.CookieHeader
2021

21-
from Cookie cookie, string alert
22+
from Http::Server::CookieWrite cookie, string alert
2223
where
23-
not cookie.isSecure() and
24+
cookie.getSecureFlag() = false and
2425
alert = "secure"
2526
or
26-
not cookie.isHttpOnly() and
27+
cookie.getHttpOnlyFlag() = false and
2728
alert = "httponly"
2829
or
29-
not cookie.isSameSite() and
30+
cookie.getSameSiteFlag() = false and
3031
alert = "samesite"
3132
select cookie, "Cookie is added without the '" + alert + "' flag properly set."

‎python/ql/src/experimental/semmle/python/Concepts.qll‎

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -278,55 +278,6 @@ class CsvWriter extends DataFlow::Node instanceof CsvWriter::Range {
278278
DataFlow::Node getAnInput() { result = super.getAnInput() }
279279
}
280280

281-
/**
282-
* A data-flow node that sets a cookie in an HTTP response.
283-
*
284-
* Extend this class to refine existing API models. If you want to model new APIs,
285-
* extend `Cookie::Range` instead.
286-
*/
287-
class Cookie extends Http::Server::CookieWrite instanceof Cookie::Range {
288-
/**
289-
* Holds if this cookie is secure.
290-
*/
291-
predicate isSecure() { super.isSecure() }
292-
293-
/**
294-
* Holds if this cookie is HttpOnly.
295-
*/
296-
predicate isHttpOnly() { super.isHttpOnly() }
297-
298-
/**
299-
* Holds if the cookie is SameSite
300-
*/
301-
predicate isSameSite() { super.isSameSite() }
302-
}
303-
304-
/** Provides a class for modeling new cookie writes on HTTP responses. */
305-
module Cookie {
306-
/**
307-
* A data-flow node that sets a cookie in an HTTP response.
308-
*
309-
* Extend this class to model new APIs. If you want to refine existing API models,
310-
* extend `Cookie` instead.
311-
*/
312-
abstract class Range extends Http::Server::CookieWrite::Range {
313-
/**
314-
* Holds if this cookie is secure.
315-
*/
316-
abstract predicate isSecure();
317-
318-
/**
319-
* Holds if this cookie is HttpOnly.
320-
*/
321-
abstract predicate isHttpOnly();
322-
323-
/**
324-
* Holds if the cookie is SameSite.
325-
*/
326-
abstract predicate isSameSite();
327-
}
328-
}
329-
330281
/** Provides classes for modeling JWT encoding-related APIs. */
331282
module JwtEncoding {
332283
/**

‎python/ql/src/experimental/semmle/python/CookieHeader.qll‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ import semmle.python.Concepts
2727
* * `isSameSite()` predicate would fail.
2828
* * `getName()` and `getValue()` results would be `"name=value; Secure;"`.
2929
*/
30-
class CookieHeader extends Cookie::Range instanceof Http::Server::ResponseHeaderWrite {
30+
class CookieHeader extends Http::Server::CookieWrite::Range instanceof Http::Server::ResponseHeaderWrite
31+
{
3132
CookieHeader() {
3233
exists(StringLiteral str |
3334
str.getText() = "Set-Cookie" and
@@ -37,31 +38,40 @@ class CookieHeader extends Cookie::Range instanceof Http::Server::ResponseHeader
3738
)
3839
}
3940

40-
override predicate isSecure() {
41-
exists(StringLiteral str |
42-
str.getText().regexpMatch(".*; *Secure;.*") and
43-
DataFlow::exprNode(str)
44-
.(DataFlow::LocalSourceNode)
45-
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
46-
)
41+
override boolean getSecureFlag() {
42+
if
43+
exists(StringLiteral str |
44+
str.getText().regexpMatch(".*; *Secure;.*") and
45+
DataFlow::exprNode(str)
46+
.(DataFlow::LocalSourceNode)
47+
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
48+
)
49+
then result = true
50+
else result = false
4751
}
4852

49-
override predicate isHttpOnly() {
50-
exists(StringLiteral str |
51-
str.getText().regexpMatch(".*; *HttpOnly;.*") and
52-
DataFlow::exprNode(str)
53-
.(DataFlow::LocalSourceNode)
54-
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
55-
)
53+
override boolean getHttpOnlyFlag() {
54+
if
55+
exists(StringLiteral str |
56+
str.getText().regexpMatch(".*; *HttpOnly;.*") and
57+
DataFlow::exprNode(str)
58+
.(DataFlow::LocalSourceNode)
59+
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
60+
)
61+
then result = true
62+
else result = false
5663
}
5764

58-
override predicate isSameSite() {
59-
exists(StringLiteral str |
60-
str.getText().regexpMatch(".*; *SameSite=(Strict|Lax);.*") and
61-
DataFlow::exprNode(str)
62-
.(DataFlow::LocalSourceNode)
63-
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
64-
)
65+
override boolean getSameSiteFlag() {
66+
if
67+
exists(StringLiteral str |
68+
str.getText().regexpMatch(".*; *SameSite=(Strict|Lax);.*") and
69+
DataFlow::exprNode(str)
70+
.(DataFlow::LocalSourceNode)
71+
.flowsTo(this.(Http::Server::ResponseHeaderWrite).getValueArg())
72+
)
73+
then result = true
74+
else result = false
6575
}
6676

6777
override DataFlow::Node getNameArg() {

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

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ private module ExperimentalPrivateDjango {
107107
* * `isHttpOnly()` predicate would succeed.
108108
* * `isSameSite()` predicate would succeed.
109109
*/
110-
class DjangoResponseSetCookieCall extends DataFlow::MethodCallNode, Cookie::Range {
110+
class DjangoResponseSetCookieCall extends DataFlow::MethodCallNode,
111+
Http::Server::CookieWrite::Range
112+
{
111113
DjangoResponseSetCookieCall() {
112114
this.calls(PrivateDjango::DjangoImpl::DjangoHttp::Response::HttpResponse::instance(),
113115
"set_cookie")
@@ -121,25 +123,34 @@ private module ExperimentalPrivateDjango {
121123
result in [this.getArg(1), this.getArgByName("value")]
122124
}
123125

124-
override predicate isSecure() {
125-
DataFlow::exprNode(any(True t))
126-
.(DataFlow::LocalSourceNode)
127-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("secure"))
126+
override boolean getSecureFlag() {
127+
if
128+
DataFlow::exprNode(any(True t))
129+
.(DataFlow::LocalSourceNode)
130+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("secure"))
131+
then result = true
132+
else result = false
128133
}
129134

130-
override predicate isHttpOnly() {
131-
DataFlow::exprNode(any(True t))
132-
.(DataFlow::LocalSourceNode)
133-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("httponly"))
135+
override boolean getHttpOnlyFlag() {
136+
if
137+
DataFlow::exprNode(any(True t))
138+
.(DataFlow::LocalSourceNode)
139+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("httponly"))
140+
then result = true
141+
else result = false
134142
}
135143

136-
override predicate isSameSite() {
137-
exists(StringLiteral str |
138-
str.getText() in ["Strict", "Lax"] and
139-
DataFlow::exprNode(str)
140-
.(DataFlow::LocalSourceNode)
141-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("samesite"))
142-
)
144+
override boolean getSameSiteFlag() {
145+
if
146+
exists(StringLiteral str |
147+
str.getText() in ["Strict", "Lax"] and
148+
DataFlow::exprNode(str)
149+
.(DataFlow::LocalSourceNode)
150+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("samesite"))
151+
)
152+
then result = true
153+
else result = false
143154
}
144155

145156
override DataFlow::Node getHeaderArg() { none() }

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

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import python
77
private import semmle.python.frameworks.Flask
88
private import semmle.python.dataflow.new.DataFlow
99
private import experimental.semmle.python.Concepts
10+
private import semmle.python.Concepts
1011
private import semmle.python.ApiGraphs
1112
private import semmle.python.frameworks.Flask
1213

@@ -31,30 +32,40 @@ module ExperimentalFlask {
3132
* * `isHttpOnly()` predicate would succeed.
3233
* * `isSameSite()` predicate would succeed.
3334
*/
34-
class FlaskSetCookieCall extends Cookie::Range instanceof Flask::FlaskResponseSetCookieCall {
35+
class FlaskSetCookieCall extends Http::Server::CookieWrite::Range instanceof Flask::FlaskResponseSetCookieCall
36+
{
3537
override DataFlow::Node getNameArg() { result = this.getNameArg() }
3638

3739
override DataFlow::Node getValueArg() { result = this.getValueArg() }
3840

39-
override predicate isSecure() {
40-
DataFlow::exprNode(any(True t))
41-
.(DataFlow::LocalSourceNode)
42-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("secure"))
41+
override boolean getSecureFlag() {
42+
if
43+
DataFlow::exprNode(any(True t))
44+
.(DataFlow::LocalSourceNode)
45+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("secure"))
46+
then result = true
47+
else result = false
4348
}
4449

45-
override predicate isHttpOnly() {
46-
DataFlow::exprNode(any(True t))
47-
.(DataFlow::LocalSourceNode)
48-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("httponly"))
50+
override boolean getHttpOnlyFlag() {
51+
if
52+
DataFlow::exprNode(any(True t))
53+
.(DataFlow::LocalSourceNode)
54+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("httponly"))
55+
then result = true
56+
else result = false
4957
}
5058

51-
override predicate isSameSite() {
52-
exists(StringLiteral str |
53-
str.getText() in ["Strict", "Lax"] and
54-
DataFlow::exprNode(str)
55-
.(DataFlow::LocalSourceNode)
56-
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("samesite"))
57-
)
59+
override boolean getSameSiteFlag() {
60+
if
61+
exists(StringLiteral str |
62+
str.getText() in ["Strict", "Lax"] and
63+
DataFlow::exprNode(str)
64+
.(DataFlow::LocalSourceNode)
65+
.flowsTo(this.(DataFlow::CallCfgNode).getArgByName("samesite"))
66+
)
67+
then result = true
68+
else result = false
5869
}
5970

6071
override DataFlow::Node getHeaderArg() { none() }

‎python/ql/src/experimental/semmle/python/security/injection/CookieInjection.qll‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import python
22
import experimental.semmle.python.Concepts
3+
import semmle.python.Concepts
34
import semmle.python.dataflow.new.DataFlow
45
import semmle.python.dataflow.new.TaintTracking
56
import semmle.python.dataflow.new.RemoteFlowSources
@@ -8,16 +9,16 @@ class CookieSink extends DataFlow::Node {
89
string flag;
910

1011
CookieSink() {
11-
exists(Cookie cookie |
12-
this in [cookie.getNameArg(), cookie.getValueArg()] and
12+
exists(Http::Server::CookieWrite cookie |
13+
this in [cookie.getNameArg(), cookie.getValueArg(), cookie.getHeaderArg()] and
1314
(
14-
not cookie.isSecure() and
15+
cookie.getSecureFlag() = false and
1516
flag = "secure"
1617
or
17-
not cookie.isHttpOnly() and
18+
cookie.getHttpOnlyFlag() = false and
1819
flag = "httponly"
1920
or
20-
not cookie.isSameSite() and
21+
cookie.getSameSiteFlag() = false and
2122
flag = "samesite"
2223
)
2324
)
@@ -33,7 +34,9 @@ private module CookieInjectionConfig implements DataFlow::ConfigSig {
3334
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3435

3536
predicate isSink(DataFlow::Node sink) {
36-
exists(Cookie c | sink in [c.getNameArg(), c.getValueArg()])
37+
exists(Http::Server::CookieWrite c |
38+
sink in [c.getNameArg(), c.getValueArg(), c.getHeaderArg()]
39+
)
3740
}
3841
}
3942

0 commit comments

Comments
 (0)