Skip to content

Commit e7d649f

Browse files
jorgectfjorgectf
committed
Make Cookie concept extend HTTP::Server::CookieWrite 
1 parent 83e3de1 commit e7d649f

File tree

6 files changed

+24
-38
lines changed

6 files changed

+24
-38
lines changed

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

Lines changed: 6 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ private import semmle.python.dataflow.new.DataFlow
1313
private import semmle.python.dataflow.new.RemoteFlowSources
1414
private import semmle.python.dataflow.new.TaintTracking
1515
private import experimental.semmle.python.Frameworks
16+
private import semmle.python.Concepts
1617

1718
/** Provides classes for modeling log related APIs. */
1819
module LogOutput {
@@ -303,35 +304,21 @@ class HeaderDeclaration extends DataFlow::Node {
303304
* Extend this class to refine existing API models. If you want to model new APIs,
304305
* extend `Cookie::Range` instead.
305306
*/
306-
class Cookie extends DataFlow::Node {
307-
Cookie::Range range;
308-
309-
Cookie() { this = range }
310-
307+
class Cookie extends HTTP::Server::CookieWrite instanceof Cookie::Range {
311308
/**
312309
* Holds if this cookie is secure.
313310
*/
314-
predicate isSecure() { range.isSecure() }
311+
predicate isSecure() { super.isSecure() }
315312

316313
/**
317314
* Holds if this cookie is HttpOnly.
318315
*/
319-
predicate isHttpOnly() { range.isHttpOnly() }
316+
predicate isHttpOnly() { super.isHttpOnly() }
320317

321318
/**
322319
* Holds if the cookie is SameSite
323320
*/
324-
predicate isSameSite() { range.isSameSite() }
325-
326-
/**
327-
* Gets the argument containing the header name.
328-
*/
329-
DataFlow::Node getName() { result = range.getName() }
330-
331-
/**
332-
* Gets the argument containing the header value.
333-
*/
334-
DataFlow::Node getValue() { result = range.getValue() }
321+
predicate isSameSite() { super.isSameSite() }
335322
}
336323

337324
/** Provides a class for modeling new cookie writes on HTTP responses. */
@@ -342,7 +329,7 @@ module Cookie {
342329
* Extend this class to model new APIs. If you want to refine existing API models,
343330
* extend `Cookie` instead.
344331
*/
345-
abstract class Range extends DataFlow::Node {
332+
abstract class Range extends HTTP::Server::CookieWrite::Range {
346333
/**
347334
* Holds if this cookie is secure.
348335
*/
@@ -357,15 +344,5 @@ module Cookie {
357344
* Holds if the cookie is SameSite.
358345
*/
359346
abstract predicate isSameSite();
360-
361-
/**
362-
* Gets the argument containing the header name.
363-
*/
364-
abstract DataFlow::Node getName();
365-
366-
/**
367-
* Gets the argument containing the header value.
368-
*/
369-
abstract DataFlow::Node getValue();
370347
}
371348
}

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import experimental.semmle.python.Concepts
2626
* * `isSameSite()` predicate would fail.
2727
* * `getName()` and `getValue()` results would be `"name=value; Secure;"`.
2828
*/
29-
class CookieHeader extends HeaderDeclaration, Cookie::Range {
29+
class CookieHeader extends Cookie::Range instanceof HeaderDeclaration {
3030
CookieHeader() {
3131
this instanceof HeaderDeclaration and
3232
this.(HeaderDeclaration).getNameArg().asExpr().(Str_).getS() = "Set-Cookie"
@@ -49,7 +49,9 @@ class CookieHeader extends HeaderDeclaration, Cookie::Range {
4949
.regexpMatch(".*; *SameSite=(Strict|Lax);.*")
5050
}
5151

52-
override DataFlow::Node getName() { result = this.(HeaderDeclaration).getValueArg() }
52+
override DataFlow::Node getNameArg() { result = this.(HeaderDeclaration).getValueArg() }
5353

54-
override DataFlow::Node getValue() { result = this.(HeaderDeclaration).getValueArg() }
54+
override DataFlow::Node getValueArg() { result = this.(HeaderDeclaration).getValueArg() }
55+
56+
override DataFlow::Node getHeaderArg() { none() }
5557
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,9 @@ private module PrivateDjango {
109109
class DjangoSetCookieCall extends DataFlow::CallCfgNode, Cookie::Range {
110110
DjangoSetCookieCall() { this = baseClassRef().getMember("set_cookie").getACall() }
111111

112-
override DataFlow::Node getName() { result = this.getArg(0) }
112+
override DataFlow::Node getNameArg() { result = this.getArg(0) }
113113

114-
override DataFlow::Node getValue() { result = this.getArgByName("value") }
114+
override DataFlow::Node getValueArg() { result = this.getArgByName("value") }
115115

116116
override predicate isSecure() {
117117
DataFlow::exprNode(any(True t))
@@ -128,6 +128,8 @@ private module PrivateDjango {
128128
override predicate isSameSite() {
129129
this.getArgByName("samesite").asExpr().(Str_).getS() in ["Strict", "Lax"]
130130
}
131+
132+
override DataFlow::Node getHeaderArg() { none() }
131133
}
132134
}
133135
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,9 @@ module ExperimentalFlask {
111111
.getACall()
112112
}
113113

114-
override DataFlow::Node getName() { result = this.getArg(0) }
114+
override DataFlow::Node getNameArg() { result = this.getArg(0) }
115115

116-
override DataFlow::Node getValue() { result = this.getArgByName("value") }
116+
override DataFlow::Node getValueArg() { result = this.getArgByName("value") }
117117

118118
override predicate isSecure() {
119119
DataFlow::exprNode(any(True t))
@@ -130,5 +130,7 @@ module ExperimentalFlask {
130130
override predicate isSameSite() {
131131
this.getArgByName("samesite").asExpr().(Str_).getS() in ["Strict", "Lax"]
132132
}
133+
134+
override DataFlow::Node getHeaderArg() { none() }
133135
}
134136
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class CookieSink extends DataFlow::Node {
99

1010
CookieSink() {
1111
exists(Cookie cookie |
12-
this in [cookie.getName(), cookie.getValue()] and
12+
this in [cookie.getNameArg(), cookie.getValueArg()] and
1313
(
1414
not cookie.isSecure() and
1515
flag = "secure"
@@ -35,6 +35,6 @@ class CookieInjectionFlowConfig extends TaintTracking::Configuration {
3535
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3636

3737
override predicate isSink(DataFlow::Node sink) {
38-
exists(Cookie c | sink in [c.getName(), c.getValue()])
38+
exists(Cookie c | sink in [c.getNameArg(), c.getValueArg()])
3939
}
4040
}

‎python/ql/test/experimental/query-tests/Security/CWE-614/CookieInjection.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
| django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | user-supplied input | and its httponly flag is not properly set |
22
| django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | user-supplied input | and its samesite flag is not properly set |
33
| django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:19:21:19:55 | ControlFlowNode for Attribute() | user-supplied input | and its secure flag is not properly set |
4+
| django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | and its httponly flag is not properly set |
5+
| django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | and its samesite flag is not properly set |
6+
| django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | Cookie is constructed from a | django_bad.py:20:21:20:56 | ControlFlowNode for Attribute() | user-supplied input | and its secure flag is not properly set |
47
| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | and its httponly flag is not properly set |
58
| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | and its samesite flag is not properly set |
69
| django_bad.py:27:30:27:124 | ControlFlowNode for Fstring | Cookie is constructed from a | django_bad.py:27:33:27:67 | ControlFlowNode for Attribute() | user-supplied input | and its secure flag is not properly set |

0 commit comments

Comments
 (0)