Skip to content

Commit 62c2fe6

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #16933 from joefarebrother/python-cookie-concept-promote
Python: Promote the insecure cookie query from experimental
2 parents e222b49 + 24df548 commit 62c2fe6

File tree

39 files changed

+347
-490
lines changed

39 files changed

+347
-490
lines changed

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

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,6 +1203,77 @@ module Http {
12031203
* Gets the argument, if any, specifying the cookie value.
12041204
*/
12051205
DataFlow::Node getValueArg() { result = super.getValueArg() }
1206+
1207+
/**
1208+
* Holds if the `Secure` flag of the cookie is known to have a value of `b`.
1209+
*/
1210+
predicate hasSecureFlag(boolean b) { super.hasSecureFlag(b) }
1211+
1212+
/**
1213+
* Holds if the `HttpOnly` flag of the cookie is known to have a value of `b`.
1214+
*/
1215+
predicate hasHttpOnlyFlag(boolean b) { super.hasHttpOnlyFlag(b) }
1216+
1217+
/**
1218+
* Holds if the `SameSite` attribute of the cookie is known to have a value of `v`.
1219+
*/
1220+
predicate hasSameSiteAttribute(CookieWrite::SameSiteValue v) { super.hasSameSiteAttribute(v) }
1221+
}
1222+
1223+
/**
1224+
* A dataflow call node to a method that sets a cookie in an http response,
1225+
* and has common keyword arguments `secure`, `httponly`, and `samesite` to set the attributes of the cookie.
1226+
*
1227+
* See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
1228+
*/
1229+
abstract class SetCookieCall extends CookieWrite::Range, DataFlow::CallCfgNode {
1230+
override predicate hasSecureFlag(boolean b) {
1231+
super.hasSecureFlag(b)
1232+
or
1233+
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
1234+
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
1235+
b = bool.booleanValue()
1236+
)
1237+
or
1238+
not exists(this.getArgByName("secure")) and
1239+
not exists(this.getKwargs()) and
1240+
b = false
1241+
}
1242+
1243+
override predicate hasHttpOnlyFlag(boolean b) {
1244+
super.hasHttpOnlyFlag(b)
1245+
or
1246+
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
1247+
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
1248+
b = bool.booleanValue()
1249+
)
1250+
or
1251+
not exists(this.getArgByName("httponly")) and
1252+
not exists(this.getKwargs()) and
1253+
b = false
1254+
}
1255+
1256+
override predicate hasSameSiteAttribute(CookieWrite::SameSiteValue v) {
1257+
super.hasSameSiteAttribute(v)
1258+
or
1259+
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
1260+
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
1261+
(
1262+
str.getText().toLowerCase() = "strict" and
1263+
v instanceof CookieWrite::SameSiteStrict
1264+
or
1265+
str.getText().toLowerCase() = "lax" and
1266+
v instanceof CookieWrite::SameSiteLax
1267+
or
1268+
str.getText().toLowerCase() = "none" and
1269+
v instanceof CookieWrite::SameSiteNone
1270+
)
1271+
)
1272+
or
1273+
not exists(this.getArgByName("samesite")) and
1274+
not exists(this.getKwargs()) and
1275+
v instanceof CookieWrite::SameSiteLax // Lax is the default
1276+
}
12061277
}
12071278

12081279
/** Provides a class for modeling new cookie writes on HTTP responses. */
@@ -1231,6 +1302,91 @@ module Http {
12311302
* Gets the argument, if any, specifying the cookie value.
12321303
*/
12331304
abstract DataFlow::Node getValueArg();
1305+
1306+
/**
1307+
* Holds if the `Secure` flag of the cookie is known to have a value of `b`.
1308+
*/
1309+
predicate hasSecureFlag(boolean b) {
1310+
exists(StringLiteral sl |
1311+
// `sl` is likely a substring of the header
1312+
TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
1313+
sl.getText().regexpMatch("(?i).*;\\s*secure(;.*|\\s*)") and
1314+
b = true
1315+
or
1316+
// `sl` is the entire header
1317+
DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
1318+
not sl.getText().regexpMatch("(?i).*;\\s*secure(;.*|\\s*)") and
1319+
b = false
1320+
)
1321+
}
1322+
1323+
/**
1324+
* Holds if the `HttpOnly` flag of the cookie is known to have a value of `b`.
1325+
*/
1326+
predicate hasHttpOnlyFlag(boolean b) {
1327+
exists(StringLiteral sl |
1328+
// `sl` is likely a substring of the header
1329+
TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
1330+
sl.getText().regexpMatch("(?i).*;\\s*httponly(;.*|\\s*)") and
1331+
b = true
1332+
or
1333+
// `sl` is the entire header
1334+
DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
1335+
not sl.getText().regexpMatch("(?i).*;\\s*httponly(;.*|\\s*)") and
1336+
b = false
1337+
)
1338+
}
1339+
1340+
/**
1341+
* Holds if the `SameSite` flag of the cookie is known to have a value of `v`.
1342+
*/
1343+
predicate hasSameSiteAttribute(SameSiteValue v) {
1344+
exists(StringLiteral sl |
1345+
// `sl` is likely a substring of the header
1346+
TaintTracking::localTaint(DataFlow::exprNode(sl), this.getHeaderArg()) and
1347+
(
1348+
sl.getText().regexpMatch("(?i).*;\\s*samesite=strict(;.*|\\s*)") and
1349+
v instanceof SameSiteStrict
1350+
or
1351+
sl.getText().regexpMatch("(?i).*;\\s*samesite=lax(;.*|\\s*)") and
1352+
v instanceof SameSiteLax
1353+
or
1354+
sl.getText().regexpMatch("(?i).*;\\s*samesite=none(;.*|\\s*)") and
1355+
v instanceof SameSiteNone
1356+
)
1357+
or
1358+
// `sl` is the entire header
1359+
DataFlow::localFlow(DataFlow::exprNode(sl), this.getHeaderArg()) and
1360+
not sl.getText().regexpMatch("(?i).*;\\s*samesite=(strict|lax|none)(;.*|\\s*)") and
1361+
v instanceof SameSiteLax // Lax is the default
1362+
)
1363+
}
1364+
}
1365+
1366+
private newtype TSameSiteValue =
1367+
TSameSiteStrict() or
1368+
TSameSiteLax() or
1369+
TSameSiteNone()
1370+
1371+
/** A possible value for the SameSite attribute of a cookie. */
1372+
class SameSiteValue extends TSameSiteValue {
1373+
/** Gets a string representation of this value. */
1374+
string toString() { none() }
1375+
}
1376+
1377+
/** A `Strict` value of the `SameSite` attribute. */
1378+
class SameSiteStrict extends SameSiteValue, TSameSiteStrict {
1379+
override string toString() { result = "Strict" }
1380+
}
1381+
1382+
/** A `Lax` value of the `SameSite` attribute. */
1383+
class SameSiteLax extends SameSiteValue, TSameSiteLax {
1384+
override string toString() { result = "Lax" }
1385+
}
1386+
1387+
/** A `None` value of the `SameSite` attribute. */
1388+
class SameSiteNone extends SameSiteValue, TSameSiteNone {
1389+
override string toString() { result = "None" }
12341390
}
12351391
}
12361392

‎python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,12 @@ class CallCfgNode extends CfgNode, LocalSourceNode {
219219

220220
/** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */
221221
Node getArgByName(string name) { result.asCfgNode() = node.getArgByName(name) }
222+
223+
/** Gets the data-flow node corresponding to the first tuple (*) argument of the call corresponding to this data-flow node, if any. */
224+
Node getStarArg() { result.asCfgNode() = node.getStarArg() }
225+
226+
/** Gets the data-flow node corresponding to a dictionary (**) argument of the call corresponding to this data-flow node, if any. */
227+
Node getKwargs() { result.asCfgNode() = node.getKwargs() }
222228
}
223229

224230
/**

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -653,8 +653,7 @@ module AiohttpWebModel {
653653
/**
654654
* A call to `set_cookie` on a HTTP Response.
655655
*/
656-
class AiohttpResponseSetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::CallCfgNode
657-
{
656+
class AiohttpResponseSetCookieCall extends Http::Server::SetCookieCall {
658657
AiohttpResponseSetCookieCall() {
659658
this = aiohttpResponseInstance().getMember("set_cookie").getACall()
660659
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2170,7 +2170,7 @@ module PrivateDjango {
21702170
/**
21712171
* A call to `set_cookie` on a HTTP Response.
21722172
*/
2173-
class DjangoResponseSetCookieCall extends Http::Server::CookieWrite::Range,
2173+
class DjangoResponseSetCookieCall extends Http::Server::SetCookieCall,
21742174
DataFlow::MethodCallNode
21752175
{
21762176
DjangoResponseSetCookieCall() {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ module FastApi {
348348
/**
349349
* A call to `set_cookie` on a FastAPI Response.
350350
*/
351-
private class SetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::MethodCallNode {
351+
private class SetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
352352
SetCookieCall() { this.calls(instance(), "set_cookie") }
353353

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

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -583,9 +583,7 @@ module Flask {
583583
*
584584
* See https://flask.palletsprojects.com/en/2.0.x/api/#flask.Response.set_cookie
585585
*/
586-
class FlaskResponseSetCookieCall extends Http::Server::CookieWrite::Range,
587-
DataFlow::MethodCallNode
588-
{
586+
class FlaskResponseSetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
589587
FlaskResponseSetCookieCall() { this.calls(Flask::Response::instance(), "set_cookie") }
590588

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ module Pyramid {
255255
}
256256

257257
/** A call to `response.set_cookie`. */
258-
private class SetCookieCall extends Http::Server::CookieWrite::Range, DataFlow::MethodCallNode {
258+
private class SetCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
259259
SetCookieCall() { this.calls(instance(), "set_cookie") }
260260

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,7 @@ module Tornado {
592592
*
593593
* See https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.set_cookie
594594
*/
595-
class TornadoRequestHandlerSetCookieCall extends Http::Server::CookieWrite::Range,
595+
class TornadoRequestHandlerSetCookieCall extends Http::Server::SetCookieCall,
596596
DataFlow::MethodCallNode
597597
{
598598
TornadoRequestHandlerSetCookieCall() {

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,7 @@ private module Twisted {
235235
*
236236
* See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http.Request.html#addCookie
237237
*/
238-
class TwistedRequestAddCookieCall extends Http::Server::CookieWrite::Range,
239-
DataFlow::MethodCallNode
240-
{
238+
class TwistedRequestAddCookieCall extends Http::Server::SetCookieCall, DataFlow::MethodCallNode {
241239
TwistedRequestAddCookieCall() { this.calls(Twisted::Request::instance(), "addCookie") }
242240

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

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@
44
<qhelp>
55

66
<overview>
7-
<p>Setting the 'secure' flag on a cookie to <code>False</code> can cause it to be sent in cleartext.
8-
Setting the 'httponly' flag on a cookie to <code>False</code> may allow attackers access it via JavaScript.
9-
Setting the 'samesite' flag on a cookie to <code>'None'</code> will make the cookie to be sent in third-party
10-
contexts which may be attacker-controlled.</p>
7+
<p>Cookies without the <code>Secure</code> flag set may be transmittd using HTTP instead of HTTPS, which leaves it vulnerable to being read by a third party.</p>
8+
<p>Cookies without the <code>HttpOnly</code> flag set are accessible to JavaScript running in the same origin. In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script.</p>
9+
<p>Cookies with the <code>SameSite</code> attribute set to <code>'None'</code> will be sent with cross-origin requests, which can be controlled by third-party JavaScript code and allow for Cross-Site Request Forgery (CSRF) attacks.</p>
1110
</overview>
1211

1312
<recommendation>
@@ -18,9 +17,8 @@ contexts which may be attacker-controlled.</p>
1817
</recommendation>
1918

2019
<example>
21-
<p>This example shows two ways of adding a cookie to a Flask response. The first way uses <code>set_cookie</code>'s
22-
secure flag and the second adds the secure flag in the cookie's raw value.</p>
23-
<sample src="InsecureCookie.py" />
20+
<p>In the following examples, the cases marked GOOD show secure cookie attributes being set; whereas in the cases marked BAD they are not set.</p>
21+
<sample src="examples/InsecureCookie.py" />
2422
</example>
2523

2624
<references>

0 commit comments

Comments
 (0)