Skip to content

Commit 825531d

Browse files
guillaume-dequenneguillaume-dequenne
authored
SONARHTML-183 Rule S5247: Disabling auto-escaping in template engines is security-sensitive (#257)
1 parent aee1b2c commit 825531d

File tree

9 files changed

+249
-0
lines changed

9 files changed

+249
-0
lines changed

‎its/ruling/src/test/resources/expected/Web-S5247.json

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
{
2+
"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/dashboard.html": [
3+
28,
4+
31
5+
],
6+
"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/includes/singlequeuestatus.html": [
7+
6,
8+
8
9+
],
10+
"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/patch.html": [
11+
9,
12+
9,
13+
15,
14+
16
15+
],
16+
"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/queuestatus.html": [
17+
35,
18+
36,
19+
66
20+
],
21+
"project:external_webkit-jb-mr1/Tools/QueueStatusServer/templates/recentstatus.html": [
22+
46
23+
]
24+
}

‎sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/AbstractPageCheck.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ protected final void createViolation(Node node, String message) {
104104
node.getEndColumnPosition()));
105105
}
106106

107+
protected final void createViolation(int startLine, int startColumn, int endLine, int endColumn , String message) {
108+
getHtmlSourceCode().addIssue(new PreciseHtmlIssue(ruleKey, startLine, message, startColumn, endLine, endColumn));
109+
}
110+
107111
protected final void createViolation(int line, String message) {
108112
getHtmlSourceCode().addIssue(
109113
new HtmlIssue(ruleKey, line == 0 ? null : line, message)

‎sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheck.java

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/*
2+
* SonarSource HTML analyzer :: Sonar Plugin
3+
* Copyright (c) 2010-2023 SonarSource SA and Matthijs Galesloot
4+
* [email protected]
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.sonar.plugins.html.checks.security;
19+
20+
import java.util.regex.Matcher;
21+
import java.util.regex.Pattern;
22+
import java.util.stream.Collectors;
23+
import org.sonar.check.Rule;
24+
import org.sonar.plugins.html.checks.AbstractPageCheck;
25+
import org.sonar.plugins.html.node.TextNode;
26+
27+
/*
28+
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.
29+
Any more elaborate support of these engines should be done in a dedicated sensor with a dedicated parser.
30+
*/
31+
@Rule(key = "S5247")
32+
public class DisabledAutoescapeCheck extends AbstractPageCheck {
33+
34+
private static final String MESSAGE = "Make sure disabling auto-escaping feature is safe here.";
35+
private static final Pattern pattern = Pattern.compile("\\{%\\s*autoescape\\s+(false|off)\\s*%\\}|\\|safe\\s*\\}\\}");
36+
37+
@Override
38+
public void characters(TextNode textNode) {
39+
Matcher matcher = pattern.matcher(textNode.getCode());
40+
if (!matcher.find()) {
41+
return;
42+
}
43+
var lines = textNode.getCode().lines()
44+
.collect(Collectors.toList());
45+
boolean raisedWithPreciseLocation = false;
46+
for (int i=0; i<lines.size(); i++) {
47+
Matcher lineMatcher = pattern.matcher(lines.get(i));
48+
while (lineMatcher.find()) {
49+
int issueLine = i + textNode.getStartLinePosition();
50+
int startColumn = i != 0 ? lineMatcher.start() : (textNode.getStartColumnPosition() + lineMatcher.start());
51+
int endColumn = i != 0 ? lineMatcher.end() : (textNode.getStartColumnPosition() + lineMatcher.end());
52+
createViolation(issueLine, startColumn, issueLine, endColumn, MESSAGE);
53+
raisedWithPreciseLocation = true;
54+
}
55+
}
56+
if (!raisedWithPreciseLocation) {
57+
// If no precise location can be found for the issue (the node only contains violations spanning multiple lines), the issue is raised on the whole text node
58+
createViolation(textNode, MESSAGE);
59+
}
60+
}
61+
}

‎sonar-html-plugin/src/main/java/org/sonar/plugins/html/rules/CheckClasses.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.sonar.plugins.html.checks.dependencies.LibraryDependencyCheck;
3737
import org.sonar.plugins.html.checks.header.HeaderCheck;
3838
import org.sonar.plugins.html.checks.header.MultiplePageDirectivesCheck;
39+
import org.sonar.plugins.html.checks.security.DisabledAutoescapeCheck;
3940
import org.sonar.plugins.html.checks.scripting.JspScriptletCheck;
4041
import org.sonar.plugins.html.checks.scripting.LongJavaScriptCheck;
4142
import org.sonar.plugins.html.checks.scripting.NestedJavaScriptCheck;
@@ -139,6 +140,7 @@ public final class CheckClasses {
139140
DoctypePresenceCheck.class,
140141
TableHeaderHasIdOrScopeCheck.class,
141142
InputWithoutLabelCheck.class,
143+
DisabledAutoescapeCheck.class,
142144
ImgWithoutWidthOrHeightCheck.class,
143145
PageWithoutFaviconCheck.class,
144146
TodoCommentCheck.class,

‎sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.html

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<p>To reduce the risk of cross-site scripting attacks, templating systems, such as <code>Twig</code>, <code>Django</code>, <code>Smarty</code>,
2+
<code>Groovy's template engine</code>, allow configuration of automatic variable escaping before rendering templates. When escape occurs, characters
3+
that make sense to the browser (eg: &lt;a&gt;) will be transformed/replaced with escaped/sanitized values (eg: &amp; lt;a&amp; gt; ).</p>
4+
<p>Auto-escaping is not a magic feature to annihilate all cross-site scripting attacks, it depends on <a
5+
href="https://twig.symfony.com/doc/3.x/filters/escape.html">the strategy applied</a> and the context, for example a "html auto-escaping" strategy
6+
(which only transforms html characters into <a href="https://developer.mozilla.org/en-US/docs/Glossary/Entity">html entities</a>) will not be relevant
7+
when variables are used in a <a href="https://en.wikipedia.org/wiki/HTML_attribute">html attribute</a> because '<code>:</code>' character is not
8+
escaped and thus an attack as below is possible:</p>
9+
<pre>
10+
&lt;a href="{{ myLink }}"&gt;link&lt;/a&gt; // myLink = javascript:alert(document.cookie)
11+
&lt;a href="javascript:alert(document.cookie)"&gt;link&lt;/a&gt; // JS injection (XSS attack)
12+
</pre>
13+
<h2>Ask Yourself Whether</h2>
14+
<ul>
15+
<li> Templates are used to render web content and
16+
<ul>
17+
<li> dynamic variables in templates come from untrusted locations or are user-controlled inputs </li>
18+
<li> there is no local mechanism in place to sanitize or validate the inputs. </li>
19+
</ul> </li>
20+
</ul>
21+
<p>There is a risk if you answered yes to any of those questions.</p>
22+
<h2>Recommended Secure Coding Practices</h2>
23+
<p>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
24+
one.</p>
25+
<h2>Sensitive Code Example</h2>
26+
<pre>
27+
&lt;!-- Django templates --&gt;
28+
&lt;p&gt;{{ variable|safe }}&lt;/p&gt;&lt;!-- Sensitive --&gt;
29+
{% autoescape off %}&lt;!-- Sensitive --&gt;
30+
31+
&lt;!-- Jinja2 templates --&gt;
32+
&lt;p&gt;{{ variable|safe }}&lt;/p&gt;&lt;!-- Sensitive --&gt;
33+
{% autoescape false %}&lt;!-- Sensitive --&gt;
34+
</pre>
35+
<h2>See</h2>
36+
<ul>
37+
<li> <a href="https://owasp.org/Top10/A03_2021-Injection/">OWASP Top 10 2021 Category A3</a> - Injection </li>
38+
<li> <a href="https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.md">OWASP Cheat
39+
Sheet</a> - XSS Prevention Cheat Sheet </li>
40+
<li> <a href="https://owasp.org/www-project-top-ten/2017/A7_2017-Cross-Site_Scripting_(XSS)">OWASP Top 10 2017 Category A7</a> - Cross-Site
41+
Scripting (XSS) </li>
42+
<li> <a href="https://cwe.mitre.org/data/definitions/79">MITRE, CWE-79</a> - Improper Neutralization of Input During Web Page Generation
43+
('Cross-site Scripting') </li>
44+
</ul>
45+

‎sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5247.json

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
{
2+
"title": "Disabling auto-escaping in template engines is security-sensitive",
3+
"type": "SECURITY_HOTSPOT",
4+
"code": {
5+
"impacts": {
6+
"SECURITY": "MEDIUM"
7+
},
8+
"attribute": "COMPLETE"
9+
},
10+
"status": "ready",
11+
"remediation": {
12+
"func": "Constant\/Issue",
13+
"constantCost": "5min"
14+
},
15+
"tags": [
16+
"cwe"
17+
],
18+
"defaultSeverity": "Major",
19+
"ruleSpecification": "RSPEC-5247",
20+
"sqKey": "S5247",
21+
"scope": "Main",
22+
"securityStandards": {
23+
"CWE": [
24+
79
25+
],
26+
"OWASP": [
27+
"A7"
28+
],
29+
"OWASP Top 10 2021": [
30+
"A3"
31+
],
32+
"PCI DSS 3.2": [
33+
"6.5.7"
34+
],
35+
"PCI DSS 4.0": [
36+
"6.2.4"
37+
],
38+
"ASVS 4.0": [
39+
"5.3.3"
40+
]
41+
}
42+
}

‎sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/Sonar_way_profile.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
"S4084",
1818
"S4645",
1919
"S5148",
20+
"S5247",
2021
"S5254",
2122
"S5255",
2223
"S5256",

‎sonar-html-plugin/src/test/java/org/sonar/plugins/html/checks/security/DisabledAutoescapeCheckTest.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/*
2+
* SonarSource HTML analyzer :: Sonar Plugin
3+
* Copyright (c) 2010-2023 SonarSource SA and Matthijs Galesloot
4+
* [email protected]
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.sonar.plugins.html.checks.security;
19+
20+
import java.io.File;
21+
import org.junit.jupiter.api.Test;
22+
import org.junit.jupiter.api.extension.RegisterExtension;
23+
import org.sonar.plugins.html.checks.CheckMessagesVerifierRule;
24+
import org.sonar.plugins.html.checks.TestHelper;
25+
import org.sonar.plugins.html.visitor.HtmlSourceCode;
26+
27+
class DisabledAutoescapeCheckTest {
28+
@RegisterExtension
29+
public CheckMessagesVerifierRule checkMessagesVerifier = new CheckMessagesVerifierRule();
30+
31+
@Test
32+
void test_jinja() {
33+
HtmlSourceCode sourceCode = TestHelper.scan(new File("src/test/resources/checks/jinjaAutoescape.html"), new DisabledAutoescapeCheck());
34+
35+
checkMessagesVerifier.verify(sourceCode.getIssues())
36+
.next().atLocation(6, 35, 6, 57).withMessage("Make sure disabling auto-escaping feature is safe here.")
37+
.next().atLocation(7, 35, 7, 55).withMessage("Make sure disabling auto-escaping feature is safe here.")
38+
.next().atLocation(8, 35, 8, 55).withMessage("Make sure disabling auto-escaping feature is safe here.")
39+
.next().atLocation(9, 34, 11, 40).withMessage("Make sure disabling auto-escaping feature is safe here.")
40+
.next().atLocation(12, 42, 12, 49).withMessage("Make sure disabling auto-escaping feature is safe here.")
41+
.next().atLocation(13, 43, 13, 51).withMessage("Make sure disabling auto-escaping feature is safe here.")
42+
.next().atLocation(16, 12, 16, 32).withMessage("Make sure disabling auto-escaping feature is safe here.")
43+
.next().atLocation(19, 12, 19, 32).withMessage("Make sure disabling auto-escaping feature is safe here.")
44+
.next().atLocation(19, 60, 19, 80).withMessage("Make sure disabling auto-escaping feature is safe here.");
45+
}
46+
}

‎sonar-html-plugin/src/test/resources/checks/jinjaAutoescape.html

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<main class="container">
5+
<center>
6+
<p style="font-size:2em;"> {% autoescape false %}{{xss}}{% endautoescape %} </p>
7+
<p style="font-size:2em;"> {% autoescape off %}{{xss2}}{% endautoescape %} </p>
8+
<p style="font-size:2em;"> {%autoescape false%}{{xss3}}{% endautoescape %} </p>
9+
<p style="font-size:2em;"> {%autoescape
10+
false%}
11+
{{xss4}}{% endautoescape %} </p>
12+
<p style="font-size:2em;"> {{hello|safe}}</p>
13+
<p style="font-size:2em;"> {{hello2|safe }}</p>
14+
<p style="font-size:2em;"> {{thisissafe}}</p>
15+
<p style="font-size:2em;"> bla
16+
{%autoescape false%}{{xss4}}{% endautoescape %}
17+
bla </p>
18+
<p style="font-size:2em;"> bla
19+
{%autoescape false%}{{xss5}}{% endautoescape %} {%autoescape false%}{{xss6}}{% endautoescape %}
20+
bla </p>
21+
</center>
22+
</main>
23+
</body>
24+
</html>

0 commit comments

Comments
 (0)