From a0f8e2f0b14bc94fb700aae1a49814abf2663224 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 12 May 2022 12:52:03 +0000 Subject: [PATCH 1/3] Python: Modernise py/jinja2/autoescape-false A simple rewrite to use API graphs instead. The handling of falsy values is potentially a bit more restrictive now, as it only accounts for local flow. We should probably figure out a better way of capturing this pattern, but I felt that this was out of scope for the present PR. --- .../Security/CWE-079/Jinja2WithoutEscaping.ql | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql index 1ab93ae4a2b..1ed9b863c8c 100644 --- a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql +++ b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql @@ -12,6 +12,8 @@ */ import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.ApiGraphs /* * Jinja 2 Docs: @@ -25,25 +27,25 @@ import python * safe1_tmpl = Template('Hello {{ name }}!', autoescape=True) */ -ClassValue jinja2EnvironmentOrTemplate() { - result = Value::named("jinja2.Environment") +private API::Node jinja2EnvironmentOrTemplate() { + result = API::moduleImport("jinja2").getMember("Environment") or - result = Value::named("jinja2.Template") + result = API::moduleImport("jinja2").getMember("Template") } -ControlFlowNode getAutoEscapeParameter(CallNode call) { result = call.getArgByName("autoescape") } +DataFlow::Node getAutoEscapeParameter(DataFlow::CallCfgNode call) { + result = call.getArgByName("autoescape") +} -from CallNode call +from DataFlow::CallCfgNode call where - call.getFunction().pointsTo(jinja2EnvironmentOrTemplate()) and - not exists(call.getNode().getStarargs()) and - not exists(call.getNode().getKwargs()) and + call = jinja2EnvironmentOrTemplate().getACall() and + not exists(call.asCfgNode().(CallNode).getNode().getStarargs()) and + not exists(call.asCfgNode().(CallNode).getNode().getKwargs()) and ( not exists(getAutoEscapeParameter(call)) or - exists(Value isFalse | - getAutoEscapeParameter(call).pointsTo(isFalse) and - isFalse.getDefiniteBooleanValue() = false - ) + any(DataFlow::LocalSourceNode n | n.asExpr().(ImmutableLiteral).booleanValue() = false) + .flowsTo(getAutoEscapeParameter(call)) ) select call, "Using jinja2 templates with autoescape=False can potentially allow XSS attacks." From ba8d73c2be19e44b6de1433b6ad2f696f99ee28a Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 17 May 2022 12:00:17 +0000 Subject: [PATCH 2/3] Python: Use `API::CallNode` --- python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql index 1ed9b863c8c..52c050ae52f 100644 --- a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql +++ b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql @@ -37,7 +37,7 @@ DataFlow::Node getAutoEscapeParameter(DataFlow::CallCfgNode call) { result = call.getArgByName("autoescape") } -from DataFlow::CallCfgNode call +from API::CallNode call where call = jinja2EnvironmentOrTemplate().getACall() and not exists(call.asCfgNode().(CallNode).getNode().getStarargs()) and From ea32299ab095fb799ba0ca1000bd00abcb4a3405 Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 17 May 2022 13:14:55 +0000 Subject: [PATCH 3/3] Python: Use API-graph flow for boolean tracking Introduces a false positive, but arguably that false positive should have been there with the local flow as well. --- .../src/Security/CWE-079/Jinja2WithoutEscaping.ql | 13 ++++++------- .../Jinja2WithoutEscaping.expected | 1 + 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql index 52c050ae52f..820937f8649 100644 --- a/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql +++ b/python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql @@ -33,19 +33,18 @@ private API::Node jinja2EnvironmentOrTemplate() { result = API::moduleImport("jinja2").getMember("Template") } -DataFlow::Node getAutoEscapeParameter(DataFlow::CallCfgNode call) { - result = call.getArgByName("autoescape") -} - from API::CallNode call where call = jinja2EnvironmentOrTemplate().getACall() and not exists(call.asCfgNode().(CallNode).getNode().getStarargs()) and not exists(call.asCfgNode().(CallNode).getNode().getKwargs()) and ( - not exists(getAutoEscapeParameter(call)) + not exists(call.getArgByName("autoescape")) or - any(DataFlow::LocalSourceNode n | n.asExpr().(ImmutableLiteral).booleanValue() = false) - .flowsTo(getAutoEscapeParameter(call)) + call.getKeywordParameter("autoescape") + .getAValueReachingRhs() + .asExpr() + .(ImmutableLiteral) + .booleanValue() = false ) select call, "Using jinja2 templates with autoescape=False can potentially allow XSS attacks." diff --git a/python/ql/test/query-tests/Security/CWE-079-Jinja2WithoutEscaping/Jinja2WithoutEscaping.expected b/python/ql/test/query-tests/Security/CWE-079-Jinja2WithoutEscaping/Jinja2WithoutEscaping.expected index 1813aa1a50d..918fbdf604d 100644 --- a/python/ql/test/query-tests/Security/CWE-079-Jinja2WithoutEscaping/Jinja2WithoutEscaping.expected +++ b/python/ql/test/query-tests/Security/CWE-079-Jinja2WithoutEscaping/Jinja2WithoutEscaping.expected @@ -2,4 +2,5 @@ | jinja2_escaping.py:41:5:41:29 | ControlFlowNode for Environment() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. | | jinja2_escaping.py:43:1:43:3 | ControlFlowNode for E() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. | | jinja2_escaping.py:44:1:44:15 | ControlFlowNode for E() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. | +| jinja2_escaping.py:50:13:50:40 | ControlFlowNode for Environment() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. | | jinja2_escaping.py:53:15:53:43 | ControlFlowNode for Template() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |