From 825531dc50b6b4ef6529543d373e88eac476806c Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Fri, 24 Nov 2023 15:13:39 +0100 Subject: [PATCH] SONARHTML-183 Rule S5247: Disabling auto-escaping in template engines is security-sensitive (#257) --- .../test/resources/expected/Web-S5247.json | 24 ++++++++ .../html/checks/AbstractPageCheck.java | 4 ++ .../security/DisabledAutoescapeCheck.java | 61 +++++++++++++++++++ .../plugins/html/rules/CheckClasses.java | 2 + .../org/sonar/l10n/web/rules/Web/S5247.html | 45 ++++++++++++++ .../org/sonar/l10n/web/rules/Web/S5247.json | 42 +++++++++++++ .../l10n/web/rules/Web/Sonar_way_profile.json | 1 + .../security/DisabledAutoescapeCheckTest.java | 46 ++++++++++++++ .../resources/checks/jinjaAutoescape.html | 24 ++++++++ 9 files changed, 249 insertions(+) create mode 100644 its/ruling/src/test/resources/expected/Web-S5247.json create mode 100644 sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheck.java create mode 100644 sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.html create mode 100644 sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.json create mode 100644 sonar-html-plugin/src/test/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheckTest.java create mode 100644 sonar-html-plugin/src/test/resources/checks/jinjaAutoescape.html diff --git a/its/ruling/src/test/resources/expected/Web-S5247.json b/its/ruling/src/test/resources/expected/Web-S5247.json new file mode 100644 index 000000000..3f313cf83 --- /dev/null +++ b/its/ruling/src/test/resources/expected/Web-S5247.json @@ -0,0 +1,24 @@ +{ +"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/dashboard.html": [ +28, +31 +], +"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/includes/singlequeuestatus.html": [ +6, +8 +], +"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/patch.html": [ +9, +9, +15, +16 +], +"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/queuestatus.html": [ +35, +36, +66 +], +"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/recentstatus.html": [ +46 +] +} diff --git a/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/AbstractPageCheck.java b/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/AbstractPageCheck.java index a3f628da0..17806077d 100644 --- a/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/AbstractPageCheck.java +++ b/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/AbstractPageCheck.java @@ -104,6 +104,10 @@ protected final void createViolation(Node node, String message) { node.getEndColumnPosition())); } + protected final void createViolation(int startLine, int startColumn, int endLine, int endColumn , String message) { + getHtmlSourceCode().addIssue(new PreciseHtmlIssue(ruleKey, startLine, message, startColumn, endLine, endColumn)); + } + protected final void createViolation(int line, String message) { getHtmlSourceCode().addIssue( new HtmlIssue(ruleKey, line == 0 ? null : line, message) diff --git a/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheck.java b/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheck.java new file mode 100644 index 000000000..00962e639 --- /dev/null +++ b/sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheck.java @@ -0,0 +1,61 @@ +/* + * SonarSource HTML analyzer :: Sonar Plugin + * Copyright (c) 2010-2023 SonarSource SA and Matthijs Galesloot + * sonarqube@googlegroups.com + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.sonar.plugins.html.checks.security; + +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.sonar.check.Rule; +import org.sonar.plugins.html.checks.AbstractPageCheck; +import org.sonar.plugins.html.node.TextNode; + +/* + Note: While this rule covers Jinja2/Django templates, the HTML analyzer is not meant to explicitly support these templating engine and will only cover basic cases. + Any more elaborate support of these engines should be done in a dedicated sensor with a dedicated parser. + */ +@Rule(key = "S5247") +public class DisabledAutoescapeCheck extends AbstractPageCheck { + + private static final String MESSAGE = "Make sure disabling auto-escaping feature is safe here."; + private static final Pattern pattern = Pattern.compile("\\{%\\s*autoescape\\s+(false|off)\\s*%\\}|\\|safe\\s*\\}\\}"); + + @Override + public void characters(TextNode textNode) { + Matcher matcher = pattern.matcher(textNode.getCode()); + if (!matcher.find()) { + return; + } + var lines = textNode.getCode().lines() + .collect(Collectors.toList()); + boolean raisedWithPreciseLocation = false; + for (int i=0; iTo reduce the risk of cross-site scripting attacks, templating systems, such as Twig, Django, Smarty, +Groovy's template engine, allow configuration of automatic variable escaping before rendering templates. When escape occurs, characters +that make sense to the browser (eg: <a>) will be transformed/replaced with escaped/sanitized values (eg: & lt;a& gt; ).

+

Auto-escaping is not a magic feature to annihilate all cross-site scripting attacks, it depends on the strategy applied and the context, for example a "html auto-escaping" strategy +(which only transforms html characters into html entities) will not be relevant +when variables are used in a html attribute because ':' character is not +escaped and thus an attack as below is possible:

+
+<a href="{{ myLink }}">link</a> // myLink = javascript:alert(document.cookie)
+<a href="javascript:alert(document.cookie)">link</a> // JS injection (XSS attack)
+
+

Ask Yourself Whether

+ +

There is a risk if you answered yes to any of those questions.

+

Recommended Secure Coding Practices

+

Enable auto-escaping by default and continue to review the use of inputs in order to be sure that the chosen auto-escaping strategy is the right +one.

+

Sensitive Code Example

+
+<!-- Django templates -->
+<p>{{ variable|safe }}</p><!-- Sensitive -->
+{% autoescape off %}<!-- Sensitive -->
+
+<!-- Jinja2 templates -->
+<p>{{ variable|safe }}</p><!-- Sensitive -->
+{% autoescape false %}<!-- Sensitive -->
+
+

See

+ + diff --git a/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.json b/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.json new file mode 100644 index 000000000..13bcb46f5 --- /dev/null +++ b/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.json @@ -0,0 +1,42 @@ +{ + "title": "Disabling auto-escaping in template engines is security-sensitive", + "type": "SECURITY_HOTSPOT", + "code": { + "impacts": { + "SECURITY": "MEDIUM" + }, + "attribute": "COMPLETE" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "cwe" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-5247", + "sqKey": "S5247", + "scope": "Main", + "securityStandards": { + "CWE": [ + 79 + ], + "OWASP": [ + "A7" + ], + "OWASP Top 10 2021": [ + "A3" + ], + "PCI DSS 3.2": [ + "6.5.7" + ], + "PCI DSS 4.0": [ + "6.2.4" + ], + "ASVS 4.0": [ + "5.3.3" + ] + } +} diff --git a/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/Sonar_way_profile.json b/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/Sonar_way_profile.json index e63f67201..f594a0308 100644 --- a/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/Sonar_way_profile.json +++ b/sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/Sonar_way_profile.json @@ -17,6 +17,7 @@ "S4084", "S4645", "S5148", + "S5247", "S5254", "S5255", "S5256", diff --git a/sonar-html-plugin/src/test/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheckTest.java b/sonar-html-plugin/src/test/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheckTest.java new file mode 100644 index 000000000..f303985bb --- /dev/null +++ b/sonar-html-plugin/src/test/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheckTest.java @@ -0,0 +1,46 @@ +/* + * SonarSource HTML analyzer :: Sonar Plugin + * Copyright (c) 2010-2023 SonarSource SA and Matthijs Galesloot + * sonarqube@googlegroups.com + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.sonar.plugins.html.checks.security; + +import java.io.File; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.sonar.plugins.html.checks.CheckMessagesVerifierRule; +import org.sonar.plugins.html.checks.TestHelper; +import org.sonar.plugins.html.visitor.HtmlSourceCode; + +class DisabledAutoescapeCheckTest { + @RegisterExtension + public CheckMessagesVerifierRule checkMessagesVerifier = new CheckMessagesVerifierRule(); + + @Test + void test_jinja() { + HtmlSourceCode sourceCode = TestHelper.scan(new File("src/test/resources/checks/jinjaAutoescape.html"), new DisabledAutoescapeCheck()); + + checkMessagesVerifier.verify(sourceCode.getIssues()) + .next().atLocation(6, 35, 6, 57).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(7, 35, 7, 55).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(8, 35, 8, 55).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(9, 34, 11, 40).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(12, 42, 12, 49).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(13, 43, 13, 51).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(16, 12, 16, 32).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(19, 12, 19, 32).withMessage("Make sure disabling auto-escaping feature is safe here.") + .next().atLocation(19, 60, 19, 80).withMessage("Make sure disabling auto-escaping feature is safe here."); + } +} diff --git a/sonar-html-plugin/src/test/resources/checks/jinjaAutoescape.html b/sonar-html-plugin/src/test/resources/checks/jinjaAutoescape.html new file mode 100644 index 000000000..c743434c7 --- /dev/null +++ b/sonar-html-plugin/src/test/resources/checks/jinjaAutoescape.html @@ -0,0 +1,24 @@ + + + +
+
+

{% autoescape false %}{{xss}}{% endautoescape %}

+

{% autoescape off %}{{xss2}}{% endautoescape %}

+

{%autoescape false%}{{xss3}}{% endautoescape %}

+

{%autoescape + false%} + {{xss4}}{% endautoescape %}

+

{{hello|safe}}

+

{{hello2|safe }}

+

{{thisissafe}}

+

bla + {%autoescape false%}{{xss4}}{% endautoescape %} + bla

+

bla + {%autoescape false%}{{xss5}}{% endautoescape %} {%autoescape false%}{{xss6}}{% endautoescape %} + bla

+
+
+ +