Skip to content

Commit 14de3bf

Browse files
RasmusWLRasmusWL
committed
Python: Model MarkupSafe PyPI package
Since expectation tests had so many changes from ConceptsTest, I'm going to do the changes for that on in a separate commit. The important part is the changes to taint-tracking, which is highlighted in this commit.
1 parent e1c4b8c commit 14de3bf

File tree

8 files changed

+171
-11
lines changed

8 files changed

+171
-11
lines changed

‎docs/codeql/support/reusables/frameworks.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,3 +172,4 @@ Python built-in support
172172
cryptography, Cryptography library
173173
pycryptodome, Cryptography library
174174
pycryptodomex, Cryptography library
175+
MarkupSafe, Escaping Library

‎python/ql/src/semmle/python/Frameworks.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,4 @@ private import semmle.python.frameworks.Tornado
2424
private import semmle.python.frameworks.Ujson
2525
private import semmle.python.frameworks.Yaml
2626
private import semmle.python.frameworks.Yarl
27+
private import semmle.python.frameworks.MarkupSafe

‎python/ql/src/semmle/python/frameworks/MarkupSafe.qll

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
/**
2+
* Provides classes modeling security-relevant aspects of the `MarkupSafe` PyPI package.
3+
* See https://markupsafe.palletsprojects.com/en/2.0.x/.
4+
*/
5+
6+
private import python
7+
private import semmle.python.dataflow.new.DataFlow
8+
private import semmle.python.dataflow.new.TaintTracking
9+
private import semmle.python.Concepts
10+
private import semmle.python.ApiGraphs
11+
12+
/**
13+
* Provides models for the `MarkupSafe` PyPI package.
14+
* See https://markupsafe.palletsprojects.com/en/2.0.x/.
15+
*/
16+
private module MarkupSafeModel {
17+
/**
18+
* Provides models for the `markupsafe.Markup` class
19+
*
20+
* See https://markupsafe.palletsprojects.com/en/2.0.x/escaping/#markupsafe.Markup.
21+
*/
22+
module Markup {
23+
/** Gets a reference to the `markupsafe.Markup` class. */
24+
API::Node classRef() {
25+
result = API::moduleImport("markupsafe").getMember("Markup")
26+
or
27+
result = API::moduleImport("flask").getMember("Markup")
28+
}
29+
30+
/**
31+
* A source of instances of `markupsafe.Markup`, extend this class to model new instances.
32+
*
33+
* This can include instantiations of the class, return values from function
34+
* calls, or a special parameter that will be set when functions are called by an external
35+
* library.
36+
*
37+
* Use the predicate `Markup::instance()` to get references to instances of `markupsafe.Markup`.
38+
*/
39+
abstract class InstanceSource extends DataFlow::LocalSourceNode { }
40+
41+
/** A direct instantiation of `markupsafe.Markup`. */
42+
private class ClassInstantiation extends InstanceSource, DataFlow::CallCfgNode {
43+
override CallNode node;
44+
45+
ClassInstantiation() { this = classRef().getACall() }
46+
}
47+
48+
/** Gets a reference to an instance of `markupsafe.Markup`. */
49+
private DataFlow::LocalSourceNode instance(DataFlow::TypeTracker t) {
50+
t.start() and
51+
result instanceof InstanceSource
52+
or
53+
exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
54+
}
55+
56+
/** Gets a reference to an instance of `markupsafe.Markup`. */
57+
DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
58+
59+
/** A string concatenation with a `markupsafe.Markup` involved. */
60+
class StringConcat extends Markup::InstanceSource, DataFlow::CfgNode {
61+
override BinaryExprNode node;
62+
63+
StringConcat() {
64+
node.getOp() instanceof Add and
65+
(
66+
instance().asCfgNode() = node.getLeft()
67+
or
68+
instance().asCfgNode() = node.getRight()
69+
)
70+
}
71+
}
72+
73+
/** A string format with `markupsafe.Markup` as the format string. */
74+
class StringFormat extends Markup::InstanceSource, DataFlow::CallCfgNode {
75+
StringFormat() {
76+
exists(DataFlow::AttrRead attr | this.getFunction() = attr |
77+
attr.getAttributeName() = "format" and
78+
attr.getObject() = instance()
79+
)
80+
}
81+
}
82+
83+
/** Taint propagation for `markupsafe.Markup`. */
84+
class AddtionalTaintSteps extends TaintTracking::AdditionalTaintStep {
85+
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
86+
nodeTo.(ClassInstantiation).getArg(0) = nodeFrom
87+
}
88+
}
89+
}
90+
91+
/** Any escaping performed via the `markupsafe` package. */
92+
abstract private class MarkupSafeEscape extends Escaping::Range {
93+
override string getKind() {
94+
// TODO: this package claims to escape for both HTML and XML, but for now we don't
95+
// model XML.
96+
result = Escaping::getHtmlKind()
97+
}
98+
}
99+
100+
/** A call to any of the escaping functions in `markupsafe` */
101+
private class MarkupSafeEscapeCall extends Markup::InstanceSource, MarkupSafeEscape,
102+
DataFlow::CallCfgNode {
103+
MarkupSafeEscapeCall() {
104+
this = API::moduleImport("markupsafe").getMember(["escape", "escape_silent"]).getACall()
105+
or
106+
this = Markup::classRef().getMember("escape").getACall()
107+
or
108+
this = API::moduleImport("flask").getMember("escape").getACall()
109+
}
110+
111+
override DataFlow::Node getAnInput() { result = this.getArg(0) }
112+
113+
override DataFlow::Node getOutput() { result = this }
114+
}
115+
116+
/**
117+
* An escape from string concatenation with a `markupsafe.Markup` involved.
118+
*
119+
* Only things that are not already a `markupsafe.Markup` instances will be escaped.
120+
*/
121+
private class MarkupEscapeFromStringConcat extends MarkupSafeEscape, Markup::StringConcat {
122+
override DataFlow::Node getAnInput() {
123+
result.asCfgNode() in [node.getLeft(), node.getRight()] and
124+
not result = Markup::instance()
125+
}
126+
127+
override DataFlow::Node getOutput() { result = this }
128+
}
129+
130+
/** A escape from string format with `markupsafe.Markup` as the format string. */
131+
private class MarkupEscapeFromStringFormat extends MarkupSafeEscape, Markup::StringFormat {
132+
override DataFlow::Node getAnInput() {
133+
result in [this.getArg(_), this.getArgByName(_)] and
134+
not result = Markup::instance()
135+
}
136+
137+
override DataFlow::Node getOutput() { result = this }
138+
}
139+
}

‎python/ql/src/semmle/python/security/dataflow/ReflectedXSS.qll

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,4 +29,13 @@ class ReflectedXssConfiguration extends TaintTracking::Configuration {
2929
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
3030
guard instanceof StringConstCompare
3131
}
32+
33+
// TODO: For now, since there is not an `isSanitizingStep` member-predicate part of a
34+
// `TaintTracking::Configuration`, we use treat the output is a taint-sanitizer. This
35+
// is slightly imprecise, which you can see in the `m_unsafe + SAFE` test-case in
36+
// python/ql/test/library-tests/frameworks/markupsafe/taint_test.py
37+
//
38+
// However, it is better than `getAnInput()`. Due to use-use flow, that would remove
39+
// the taint-flow to `SINK()` in `some_escape(tainted); SINK(tainted)`.
40+
override predicate isSanitizer(DataFlow::Node node) { node = any(HtmlEscaping esc).getOutput() }
3241
}

‎python/ql/test/library-tests/frameworks/markupsafe/ConceptsTest.expected

Whitespace-only changes.

‎python/ql/test/library-tests/frameworks/markupsafe/ConceptsTest.ql

Lines changed: 0 additions & 2 deletions
This file was deleted.

‎python/ql/test/library-tests/frameworks/markupsafe/InlineTaintTest.ql

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,13 @@
11
import experimental.meta.InlineTaintTest
2+
import semmle.python.Concepts
3+
4+
class HtmlSpecialization extends TestTaintTrackingConfiguration {
5+
// TODO: For now, since there is not an `isSanitizingStep` member-predicate part of a
6+
// `TaintTracking::Configuration`, we use treat the output is a taint-sanitizer. This
7+
// is slightly imprecise, which you can see in the `m_unsafe + SAFE` test-case in
8+
// python/ql/test/library-tests/frameworks/markupsafe/taint_test.py
9+
//
10+
// However, it is better than `getAnInput()`. Due to use-use flow, that would remove
11+
// the taint-flow to `SINK()` in `some_escape(tainted); SINK(tainted)`.
12+
override predicate isSanitizer(DataFlow::Node node) { node = any(HtmlEscaping esc).getOutput() }
13+
}

‎python/ql/test/library-tests/frameworks/markupsafe/taint_test.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,11 @@ def test():
3232

3333
ensure_tainted(
3434
ts, # $ tainted
35-
m_unsafe, # $ MISSING: tainted
35+
m_unsafe, # $ tainted
3636
m_unsafe + SAFE, # $ MISSING: tainted
3737
SAFE + m_unsafe, # $ MISSING: tainted
3838
m_unsafe.format(SAFE), # $ MISSING: tainted
39-
m_unsafe + ts, # $ tainted
39+
m_unsafe + ts, # $ MISSING: tainted
4040

4141
m_safe.format(m_unsafe), # $ MISSING: tainted
4242

@@ -51,13 +51,13 @@ def test():
5151
Markup.escape(ts),
5252

5353
m_safe,
54-
m_safe + ts, # $ SPURIOUS: tainted
55-
ts + m_safe, # $ SPURIOUS: tainted
56-
m_safe.format(ts), # $ SPURIOUS: tainted
54+
m_safe + ts,
55+
ts + m_safe,
56+
m_safe.format(ts),
5757

58-
escape(ts) + ts, # $ SPURIOUS: tainted
59-
escape_silent(ts) + ts, # $ SPURIOUS: tainted
60-
Markup.escape(ts) + ts, # $ SPURIOUS: tainted
58+
escape(ts) + ts,
59+
escape_silent(ts) + ts,
60+
Markup.escape(ts) + ts,
6161
)
6262

6363
# flask re-exports these, as:
@@ -66,7 +66,7 @@ def test():
6666
import flask
6767

6868
ensure_tainted(
69-
flask.Markup(ts), # $ MISSING: tainted
69+
flask.Markup(ts), # $ tainted
7070
)
7171

7272
ensure_not_tainted(

0 commit comments

Comments
 (0)