Skip to content

Commit 4920557

Browse files
RasmusWLRasmusWL
authored
Merge pull request #13670 from jorgectf/seclab/xxe-sanitizer
Python: Add `markupsafe` as XXE sanitizer
2 parents 139585f + 64a86e8 commit 4920557

File tree

6 files changed

+67
-45
lines changed

6 files changed

+67
-45
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,9 @@ module Escaping {
680680
/** Gets the escape-kind for escaping a string so it can safely be included in HTML. */
681681
string getHtmlKind() { result = "html" }
682682

683+
/** Gets the escape-kind for escaping a string so it can safely be included in XML. */
684+
string getXmlKind() { result = "xml" }
685+
683686
/** Gets the escape-kind for escaping a string so it can safely be included in a regular expression. */
684687
string getRegexKind() { result = "regex" }
685688

@@ -710,6 +713,15 @@ class HtmlEscaping extends Escaping {
710713
HtmlEscaping() { super.getKind() = Escaping::getHtmlKind() }
711714
}
712715

716+
/**
717+
* An escape of a string so it can be safely included in
718+
* the body of an XML element, for example, replacing `&` and `<>` in
719+
* `<foo>&xxe;<foo>`.
720+
*/
721+
class XmlEscaping extends Escaping {
722+
XmlEscaping() { super.getKind() = Escaping::getXmlKind() }
723+
}
724+
713725
/**
714726
* An escape of a string so it can be safely included in
715727
* the body of a regex.

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ private module MarkupSafeModel {
8383
}
8484

8585
/** Taint propagation for `markupsafe.Markup`. */
86-
private class AddtionalTaintStep extends TaintTracking::AdditionalTaintStep {
86+
private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
8787
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
8888
nodeTo.(ClassInstantiation).getArg(0) = nodeFrom
8989
}
@@ -92,11 +92,7 @@ private module MarkupSafeModel {
9292

9393
/** Any escaping performed via the `markupsafe` package. */
9494
abstract private class MarkupSafeEscape extends Escaping::Range {
95-
override string getKind() {
96-
// TODO: this package claims to escape for both HTML and XML, but for now we don't
97-
// model XML.
98-
result = Escaping::getHtmlKind()
99-
}
95+
override string getKind() { result in [Escaping::getHtmlKind(), Escaping::getXmlKind()] }
10096
}
10197

10298
/** A call to any of the escaping functions in `markupsafe` */

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,11 @@ module Xxe {
4444
)
4545
}
4646
}
47+
48+
/**
49+
* An XML escaping, considered as a sanitizer.
50+
*/
51+
class XmlEscapingAsSanitizer extends Sanitizer {
52+
XmlEscapingAsSanitizer() { this = any(XmlEscaping esc).getOutput() }
53+
}
4754
}

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

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,40 +27,40 @@ def test():
2727
# as tainted even after it has been escaped in some place. This _might_ not be the
2828
# case since data-flow library has taint-steps from adjacent uses...
2929
ensure_tainted(ts) # $ tainted
30-
ensure_not_tainted(escape(ts)) # $ escapeInput=ts escapeKind=html escapeOutput=escape(..)
30+
ensure_not_tainted(escape(ts)) # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=escape(..)
3131
ensure_tainted(ts) # $ tainted
3232

3333
ensure_tainted(
3434
ts, # $ tainted
3535
m_unsafe, # $ tainted
36-
m_unsafe + SAFE, # $ escapeInput=SAFE escapeKind=html escapeOutput=BinaryExpr MISSING: tainted
37-
SAFE + m_unsafe, # $ escapeInput=SAFE escapeKind=html escapeOutput=BinaryExpr MISSING: tainted
38-
m_unsafe.format(SAFE), # $ escapeInput=SAFE escapeKind=html escapeOutput=m_unsafe.format(..) MISSING: tainted
39-
m_unsafe % SAFE, # $ escapeInput=SAFE escapeKind=html escapeOutput=BinaryExpr MISSING: tainted
40-
m_unsafe + ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr MISSING: tainted
36+
m_unsafe + SAFE, # $ escapeInput=SAFE escapeKind=html escapeKind=xml escapeOutput=BinaryExpr MISSING: tainted
37+
SAFE + m_unsafe, # $ escapeInput=SAFE escapeKind=html escapeKind=xml escapeOutput=BinaryExpr MISSING: tainted
38+
m_unsafe.format(SAFE), # $ escapeInput=SAFE escapeKind=html escapeKind=xml escapeOutput=m_unsafe.format(..) MISSING: tainted
39+
m_unsafe % SAFE, # $ escapeInput=SAFE escapeKind=html escapeKind=xml escapeOutput=BinaryExpr MISSING: tainted
40+
m_unsafe + ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr MISSING: tainted
4141

4242
m_safe.format(m_unsafe), # $ tainted
4343
m_safe % m_unsafe, # $ tainted
4444

45-
escape(ts).unescape(), # $ escapeInput=ts escapeKind=html escapeOutput=escape(..) MISSING: tainted
46-
escape_silent(ts).unescape(), # $ escapeInput=ts escapeKind=html escapeOutput=escape_silent(..) MISSING: tainted
45+
escape(ts).unescape(), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=escape(..) MISSING: tainted
46+
escape_silent(ts).unescape(), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=escape_silent(..) MISSING: tainted
4747
)
4848

4949
ensure_not_tainted(
50-
escape(ts), # $ escapeInput=ts escapeKind=html escapeOutput=escape(..)
51-
escape_silent(ts), # $ escapeInput=ts escapeKind=html escapeOutput=escape_silent(..)
50+
escape(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=escape(..)
51+
escape_silent(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=escape_silent(..)
5252

53-
Markup.escape(ts), # $ escapeInput=ts escapeKind=html escapeOutput=Markup.escape(..)
53+
Markup.escape(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=Markup.escape(..)
5454

5555
m_safe,
56-
m_safe + ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr
57-
ts + m_safe, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr
58-
m_safe.format(ts), # $ escapeInput=ts escapeKind=html escapeOutput=m_safe.format(..)
59-
m_safe % ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr
60-
61-
escape(ts) + ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr escapeOutput=escape(..)
62-
escape_silent(ts) + ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr escapeOutput=escape_silent(..)
63-
Markup.escape(ts) + ts, # $ escapeInput=ts escapeKind=html escapeOutput=BinaryExpr escapeOutput=Markup.escape(..)
56+
m_safe + ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr
57+
ts + m_safe, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr
58+
m_safe.format(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=m_safe.format(..)
59+
m_safe % ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr
60+
61+
escape(ts) + ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr escapeOutput=escape(..)
62+
escape_silent(ts) + ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr escapeOutput=escape_silent(..)
63+
Markup.escape(ts) + ts, # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=BinaryExpr escapeOutput=Markup.escape(..)
6464
)
6565

6666
# flask re-exports these, as:
@@ -73,8 +73,8 @@ def test():
7373
)
7474

7575
ensure_not_tainted(
76-
flask.escape(ts), # $ escapeInput=ts escapeKind=html escapeOutput=flask.escape(..)
77-
flask.Markup.escape(ts), # $ escapeInput=ts escapeKind=html escapeOutput=flask.Markup.escape(..)
76+
flask.escape(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=flask.escape(..)
77+
flask.Markup.escape(ts), # $ escapeInput=ts escapeKind=html escapeKind=xml escapeOutput=flask.Markup.escape(..)
7878
)
7979

8080

‎python/ql/test/query-tests/Security/CWE-611-Xxe/Xxe.expected

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,25 @@
11
edges
22
| test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:1:26:1:32 | GSSA Variable request |
3-
| test.py:1:26:1:32 | GSSA Variable request | test.py:8:19:8:25 | ControlFlowNode for request |
4-
| test.py:1:26:1:32 | GSSA Variable request | test.py:19:19:19:25 | ControlFlowNode for request |
5-
| test.py:8:19:8:25 | ControlFlowNode for request | test.py:8:19:8:30 | ControlFlowNode for Attribute |
6-
| test.py:8:19:8:30 | ControlFlowNode for Attribute | test.py:8:19:8:45 | ControlFlowNode for Subscript |
7-
| test.py:8:19:8:45 | ControlFlowNode for Subscript | test.py:9:34:9:44 | ControlFlowNode for xml_content |
8-
| test.py:19:19:19:25 | ControlFlowNode for request | test.py:19:19:19:30 | ControlFlowNode for Attribute |
9-
| test.py:19:19:19:30 | ControlFlowNode for Attribute | test.py:19:19:19:45 | ControlFlowNode for Subscript |
10-
| test.py:19:19:19:45 | ControlFlowNode for Subscript | test.py:30:34:30:44 | ControlFlowNode for xml_content |
3+
| test.py:1:26:1:32 | GSSA Variable request | test.py:9:19:9:25 | ControlFlowNode for request |
4+
| test.py:1:26:1:32 | GSSA Variable request | test.py:20:19:20:25 | ControlFlowNode for request |
5+
| test.py:9:19:9:25 | ControlFlowNode for request | test.py:9:19:9:30 | ControlFlowNode for Attribute |
6+
| test.py:9:19:9:30 | ControlFlowNode for Attribute | test.py:9:19:9:45 | ControlFlowNode for Subscript |
7+
| test.py:9:19:9:45 | ControlFlowNode for Subscript | test.py:10:34:10:44 | ControlFlowNode for xml_content |
8+
| test.py:20:19:20:25 | ControlFlowNode for request | test.py:20:19:20:30 | ControlFlowNode for Attribute |
9+
| test.py:20:19:20:30 | ControlFlowNode for Attribute | test.py:20:19:20:45 | ControlFlowNode for Subscript |
10+
| test.py:20:19:20:45 | ControlFlowNode for Subscript | test.py:31:34:31:44 | ControlFlowNode for xml_content |
1111
nodes
1212
| test.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
1313
| test.py:1:26:1:32 | GSSA Variable request | semmle.label | GSSA Variable request |
14-
| test.py:8:19:8:25 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
15-
| test.py:8:19:8:30 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
16-
| test.py:8:19:8:45 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
17-
| test.py:9:34:9:44 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
18-
| test.py:19:19:19:25 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
19-
| test.py:19:19:19:30 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
20-
| test.py:19:19:19:45 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
21-
| test.py:30:34:30:44 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
14+
| test.py:9:19:9:25 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
15+
| test.py:9:19:9:30 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
16+
| test.py:9:19:9:45 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
17+
| test.py:10:34:10:44 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
18+
| test.py:20:19:20:25 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
19+
| test.py:20:19:20:30 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
20+
| test.py:20:19:20:45 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
21+
| test.py:31:34:31:44 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
2222
subpaths
2323
#select
24-
| test.py:9:34:9:44 | ControlFlowNode for xml_content | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:9:34:9:44 | ControlFlowNode for xml_content | XML parsing depends on a $@ without guarding against external entity expansion. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
25-
| test.py:30:34:30:44 | ControlFlowNode for xml_content | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:30:34:30:44 | ControlFlowNode for xml_content | XML parsing depends on a $@ without guarding against external entity expansion. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
24+
| test.py:10:34:10:44 | ControlFlowNode for xml_content | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:10:34:10:44 | ControlFlowNode for xml_content | XML parsing depends on a $@ without guarding against external entity expansion. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
25+
| test.py:31:34:31:44 | ControlFlowNode for xml_content | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:31:34:31:44 | ControlFlowNode for xml_content | XML parsing depends on a $@ without guarding against external entity expansion. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |

‎python/ql/test/query-tests/Security/CWE-611-Xxe/test.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from flask import Flask, request
22
import lxml.etree
3+
import markupsafe
34

45
app = Flask(__name__)
56

@@ -28,3 +29,9 @@ def super_vuln_handler():
2829
huge_tree=True,
2930
)
3031
return lxml.etree.fromstring(xml_content, parser=parser).text
32+
33+
@app.route("/sanitized-handler")
34+
def sanitized_handler():
35+
xml_content = request.args['xml_content']
36+
xml_content = markupsafe.escape(xml_content)
37+
return lxml.etree.fromstring(xml_content).text

0 commit comments

Comments
 (0)