Skip to content

Commit 3f1e815

Browse files
erik-krogherik-krogh
committed
support html attribute concatenations with single quotes
1 parent 1c56c30 commit 3f1e815

File tree

6 files changed

+79
-13
lines changed

6 files changed

+79
-13
lines changed

‎javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ abstract class IncompleteBlacklistSanitizer extends DataFlow::Node {
2323
* Describes the characters represented by `rep`.
2424
*/
2525
string describeCharacters(string rep) {
26-
rep = "\"" and result = "quotes"
26+
rep = ["\"", "'"] and result = "quotes"
2727
or
2828
rep = "&" and result = "ampersands"
2929
or
@@ -86,6 +86,12 @@ module HtmlSanitization {
8686
chain.getAReplacementString() = """
8787
)
8888
or
89+
result = "'" and
90+
(
91+
chain.getAReplacedString() = result or
92+
chain.getAReplacementString() = "'"
93+
)
94+
or
8995
result = "&" and
9096
(
9197
chain.getAReplacedString() = result or
@@ -123,11 +129,7 @@ module HtmlSanitization {
123129
// replaces `<` and `>`
124130
getALikelyReplacedCharacter(chain) = "<" and
125131
getALikelyReplacedCharacter(chain) = ">" and
126-
(
127-
unsanitized = "\""
128-
or
129-
unsanitized = "&"
130-
)
132+
unsanitized = ["\"", "'", "&"]
131133
or
132134
// replaces '&' and either `<` or `>`
133135
getALikelyReplacedCharacter(chain) = "&" and

‎javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ module IncompleteHtmlAttributeSanitization {
1414

1515
private module Label {
1616
class Quote extends DataFlow::FlowLabel {
17-
Quote() { this = "\"" }
17+
Quote() { this = ["\"", "'"] }
1818
}
1919

2020
class Ampersand extends DataFlow::FlowLabel {

‎javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,15 @@ module IncompleteHtmlAttributeSanitization {
4949
*/
5050
class HtmlAttributeConcatenation extends StringOps::ConcatenationLeaf {
5151
string lhs;
52+
string quote;
5253

5354
HtmlAttributeConcatenation() {
54-
lhs = this.getPreviousLeaf().getStringValue().regexpCapture("(?s)(.*)=\"[^\"]*", 1) and
55+
quote = ["\"", "'"] and
56+
exists(string prev | prev = this.getPreviousLeaf().getStringValue() |
57+
lhs = prev.regexpCapture("(?s)(.*)=" + quote + "[^" + quote + "=<>]*", 1)
58+
) and
5559
(
56-
this.getNextLeaf().getStringValue().regexpMatch(".*\".*") or
60+
this.getNextLeaf().getStringValue().regexpMatch(".*" + quote + ".*") or
5761
this instanceof StringOps::HtmlConcatenationLeaf
5862
)
5963
}
@@ -62,6 +66,11 @@ module IncompleteHtmlAttributeSanitization {
6266
* Holds if the attribute value is interpreted as JavaScript source code.
6367
*/
6468
predicate isInterpretedAsJavaScript() { lhs.regexpMatch("(?i)(.* )?on[a-z]+") }
69+
70+
/**
71+
* Gets the quote symbol (" or ') that is used to mark the attribute value.
72+
*/
73+
string getQuote() { result = quote }
6574
}
6675

6776
/**
@@ -74,7 +83,7 @@ module IncompleteHtmlAttributeSanitization {
7483
override string getADangerousCharacter() {
7584
isInterpretedAsJavaScript() and result = "&"
7685
or
77-
result = "\""
86+
result = getQuote()
7887
}
7988
}
8089

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,28 +2,49 @@
22
| tst.js:206:2:206:24 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
33
| tst.js:207:2:207:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
44
| tst.js:208:2:208:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
5+
| tst.js:208:2:208:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
56
| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
67
| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
78
| tst.js:210:2:210:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
89
| tst.js:211:2:211:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
910
| tst.js:212:2:212:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
1011
| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize ampersands |
1112
| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize quotes |
13+
| tst.js:216:2:216:93 | s().rep ... &#34;') | This HTML sanitizer does not sanitize quotes |
1214
| tst.js:217:2:217:93 | s().rep ... &#39;') | This HTML sanitizer does not sanitize quotes |
15+
| tst.js:223:2:223:107 | s().rep ... &amp;') | This HTML sanitizer does not sanitize quotes |
1316
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
1417
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
1518
| tst.js:244:9:244:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
1619
| tst.js:245:9:245:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
20+
| tst.js:245:9:245:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
21+
| tst.js:246:9:246:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
1722
| tst.js:249:9:249:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
23+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
1824
| tst.js:251:9:251:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
1925
| tst.js:253:21:253:45 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
26+
| tst.js:253:21:253:45 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
2027
| tst.js:254:32:254:56 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
28+
| tst.js:254:32:254:56 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
2129
| tst.js:255:26:255:50 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
30+
| tst.js:255:26:255:50 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
2231
| tst.js:256:15:256:39 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
32+
| tst.js:256:15:256:39 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
2333
| tst.js:261:10:261:81 | value.r ... '&gt;') | This HTML sanitizer does not sanitize quotes |
2434
| tst.js:270:61:270:85 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
35+
| tst.js:270:61:270:85 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
2536
| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
2637
| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
2738
| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize ampersands |
2839
| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize quotes |
2940
| tst.js:277:9:277:29 | arr2.re ... "/g,"") | This HTML sanitizer does not sanitize ampersands |
41+
| tst.js:277:9:277:29 | arr2.re ... "/g,"") | This HTML sanitizer does not sanitize quotes |
42+
| tst.js:284:6:284:30 | x.repla ... quot;') | This HTML sanitizer does not sanitize quotes |
43+
| tst.js:294:7:294:31 | y.repla ... quot;') | This HTML sanitizer does not sanitize quotes |
44+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
45+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
46+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
47+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
48+
| tst.js:303:10:303:34 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
49+
| tst.js:304:9:304:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
50+
| tst.js:305:10:305:34 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ nodes
88
| tst.js:249:9:249:33 | s().rep ... ]/g,'') |
99
| tst.js:249:9:249:33 | s().rep ... ]/g,'') |
1010
| tst.js:249:9:249:33 | s().rep ... ]/g,'') |
11+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') |
12+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') |
13+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') |
1114
| tst.js:253:21:253:45 | s().rep ... /g, '') |
1215
| tst.js:253:21:253:45 | s().rep ... /g, '') |
1316
| tst.js:253:21:253:45 | s().rep ... /g, '') |
@@ -23,10 +26,23 @@ nodes
2326
| tst.js:275:9:275:11 | arr |
2427
| tst.js:275:9:275:21 | arr.join(" ") |
2528
| tst.js:275:9:275:21 | arr.join(" ") |
29+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') |
30+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') |
31+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') |
32+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') |
33+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') |
34+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') |
35+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') |
36+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') |
37+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') |
38+
| tst.js:303:10:303:34 | s().rep ... /g, '') |
39+
| tst.js:303:10:303:34 | s().rep ... /g, '') |
40+
| tst.js:303:10:303:34 | s().rep ... /g, '') |
2641
edges
2742
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') |
2843
| tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') |
2944
| tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') |
45+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') | tst.js:250:9:250:33 | s().rep ... ]/g,'') |
3046
| tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') |
3147
| tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') |
3248
| tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') |
@@ -35,11 +51,20 @@ edges
3551
| tst.js:274:12:274:94 | s().val ... g , '') | tst.js:274:6:274:94 | arr |
3652
| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") |
3753
| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") |
54+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') | tst.js:300:10:300:33 | s().rep ... ]/g,'') |
55+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') |
56+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') |
57+
| tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') |
3858
#select
3959
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:243:9:243:31 | s().rep ... ]/g,'') | this final HTML sanitizer step |
4060
| tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:244:9:244:33 | s().rep ... /g, '') | this final HTML sanitizer step |
4161
| tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:249:9:249:33 | s().rep ... ]/g,'') | this final HTML sanitizer step |
62+
| tst.js:250:9:250:33 | s().rep ... ]/g,'') | tst.js:250:9:250:33 | s().rep ... ]/g,'') | tst.js:250:9:250:33 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:250:9:250:33 | s().rep ... ]/g,'') | this final HTML sanitizer step |
4263
| tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:253:21:253:45 | s().rep ... /g, '') | this final HTML sanitizer step |
4364
| tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:254:32:254:56 | s().rep ... /g, '') | this final HTML sanitizer step |
4465
| tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:270:61:270:85 | s().rep ... /g, '') | this final HTML sanitizer step |
4566
| tst.js:275:9:275:21 | arr.join(" ") | tst.js:274:12:274:94 | s().val ... g , '') | tst.js:275:9:275:21 | arr.join(" ") | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:274:12:274:94 | s().val ... g , '') | this final HTML sanitizer step |
67+
| tst.js:300:10:300:33 | s().rep ... ]/g,'') | tst.js:300:10:300:33 | s().rep ... ]/g,'') | tst.js:300:10:300:33 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:300:10:300:33 | s().rep ... ]/g,'') | this final HTML sanitizer step |
68+
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:301:10:301:32 | s().rep ... ]/g,'') | this final HTML sanitizer step |
69+
| tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:302:10:302:34 | s().rep ... ]/g,'') | this final HTML sanitizer step |
70+
| tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:303:10:303:34 | s().rep ... /g, '') | this final HTML sanitizer step |

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ function typicalBadHtmlSanitizers(s) {
214214
var s = s().replace(/</g, '');
215215
s = s.replace(/>/g, ''); // NOT OK
216216
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/"/g, '&#34;'); // OK
217-
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/'/g, '&#39;'); // NOT OK
217+
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/'/g, '&#39;'); // OK - single quotes or double quotes both work
218218

219219
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(RE, function(match) {/* ... */ }); // OK (probably)
220220

@@ -247,8 +247,8 @@ function incompleteHtmlAttributeSanitization() {
247247
'="' + s().replace(/[&"]/g,'') + '"'; // OK
248248

249249
'="' + s().replace(/[<>&']/g,'') + '"'; // NOT OK
250-
"='" + s().replace(/[<>&"]/g,'') + "'"; // OK (but given the context, it is probably not fine)
251-
"='" + s().replace(/[<>&']/g,'') + "'"; // NOT OK (but given the context, it is probably fine)
250+
"='" + s().replace(/[<>&"]/g,'') + "'"; // NOT OK
251+
"='" + s().replace(/[<>&']/g,'') + "'"; // OK
252252

253253
'onFunkyEvent="' + s().replace(/[<>"]/g, '') + '"'; // NOT OK
254254
'<div noise onFunkyEvent="' + s().replace(/[<>"]/g, '') + '"'; // NOT OK
@@ -295,3 +295,12 @@ function moreIncompleteHtmlAttributeSanitization() {
295295
}
296296
'onclick="' + y + '"'; // OK
297297
}
298+
299+
function incompleteHtmlAttributeSanitization2() {
300+
'=\'' + s().replace(/[&<>]/g,'') + '\''; // NOT OK
301+
'=\'' + s().replace(/[<>]/g,'') + '\''; // NOT OK
302+
'=\'' + s().replace(/[&<>"]/g,'') + '\''; // NOT OK
303+
'=\'' + s().replace(/[<>&]/g, '') + '\''; // NOT OK
304+
'="' + s().replace(/[<>&"]/g,'') + '"'; // OK
305+
'=\'' + s().replace(/[<>&']/g,'') + '\''; // OK
306+
}

0 commit comments

Comments
 (0)