hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 11:15:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

support html attribute concatenations with single quotes

Browse Source
This commit is contained in:
Erik Krogh Kristensen
2021-01-29 10:20:29 +01:00
parent 1c56c30eba
commit 3f1e81533c
6 changed files with 79 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll
View File

@@ -23,7 +23,7 @@ abstract class IncompleteBlacklistSanitizer extends DataFlow::Node {
* Describes the characters represented by `rep`.
*/
string describeCharacters(string rep) {
rep = "\"" and result = "quotes"
rep = ["\"", "'"] and result = "quotes"
or
rep = "&" and result = "ampersands"
or
@@ -86,6 +86,12 @@ module HtmlSanitization {
chain.getAReplacementString() = """
)
or
result = "'" and
(
chain.getAReplacedString() = result or
chain.getAReplacementString() = "'"
)
or
result = "&" and
(
chain.getAReplacedString() = result or
@@ -123,11 +129,7 @@ module HtmlSanitization {
// replaces `<` and `>`
getALikelyReplacedCharacter(chain) = "<" and
getALikelyReplacedCharacter(chain) = ">" and
(
unsanitized = "\""
or
unsanitized = "&"
)
unsanitized = ["\"", "'", "&"]
or
// replaces '&' and either `<` or `>`
getALikelyReplacedCharacter(chain) = "&" and

2
javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll
View File

@@ -14,7 +14,7 @@ module IncompleteHtmlAttributeSanitization {
private module Label {
class Quote extends DataFlow::FlowLabel {
Quote() { this = "\"" }
Quote() { this = ["\"", "'"] }
}
class Ampersand extends DataFlow::FlowLabel {

15
javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll
View File

@@ -49,11 +49,15 @@ module IncompleteHtmlAttributeSanitization {
*/
class HtmlAttributeConcatenation extends StringOps::ConcatenationLeaf {
string lhs;
string quote;
HtmlAttributeConcatenation() {
lhs = this.getPreviousLeaf().getStringValue().regexpCapture("(?s)(.*)=\"[^\"]*", 1) and
quote = ["\"", "'"] and
exists(string prev | prev = this.getPreviousLeaf().getStringValue() |
lhs = prev.regexpCapture("(?s)(.*)=" + quote + "[^" + quote + "=<>]*", 1)
) and
(
this.getNextLeaf().getStringValue().regexpMatch(".*\".*") or
this.getNextLeaf().getStringValue().regexpMatch(".*" + quote + ".*") or
this instanceof StringOps::HtmlConcatenationLeaf
)
}
@@ -62,6 +66,11 @@ module IncompleteHtmlAttributeSanitization {
* Holds if the attribute value is interpreted as JavaScript source code.
*/
predicate isInterpretedAsJavaScript() { lhs.regexpMatch("(?i)(.* )?on[a-z]+") }
/**
* Gets the quote symbol (" or ') that is used to mark the attribute value.
*/
string getQuote() { result = quote }
}
/**
@@ -74,7 +83,7 @@ module IncompleteHtmlAttributeSanitization {
override string getADangerousCharacter() {
isInterpretedAsJavaScript() and result = "&"
or
result = "\""
result = getQuote()
}
}

21
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected
View File

@@ -2,6 +2,7 @@
| tst.js:206:2:206:24 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:207:2:207:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:208:2:208:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:208:2:208:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:210:2:210:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
@@ -9,21 +10,41 @@
| tst.js:212:2:212:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:216:2:216:93 | s().rep ... &#34;') | This HTML sanitizer does not sanitize quotes |
| tst.js:217:2:217:93 | s().rep ... &#39;') | This HTML sanitizer does not sanitize quotes |
| tst.js:223:2:223:107 | s().rep ... &amp;') | This HTML sanitizer does not sanitize quotes |
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:244:9:244:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:245:9:245:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:245:9:245:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:246:9:246:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:249:9:249:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:250:9:250:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:251:9:251:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:253:21:253:45 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:253:21:253:45 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:254:32:254:56 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:254:32:254:56 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:255:26:255:50 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:255:26:255:50 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:256:15:256:39 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:256:15:256:39 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:261:10:261:81 | value.r ... '&gt;') | This HTML sanitizer does not sanitize quotes |
| tst.js:270:61:270:85 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:270:61:270:85 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize ampersands |
| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize quotes |
| tst.js:277:9:277:29 | arr2.re ... "/g,"") | This HTML sanitizer does not sanitize ampersands |
| tst.js:277:9:277:29 | arr2.re ... "/g,"") | This HTML sanitizer does not sanitize quotes |
| tst.js:284:6:284:30 | x.repla ... quot;') | This HTML sanitizer does not sanitize quotes |
| tst.js:294:7:294:31 | y.repla ... quot;') | This HTML sanitizer does not sanitize quotes |
| tst.js:300:10:300:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands |
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:302:10:302:34 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:303:10:303:34 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes |
| tst.js:304:9:304:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |
| tst.js:305:10:305:34 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes |

25
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected
View File

@@ -8,6 +8,9 @@ nodes
| 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,'') |
| 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,'') |
| 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, '') |
@@ -23,10 +26,23 @@ nodes
| tst.js:275:9:275:11 | arr |
| tst.js:275:9:275:21 | arr.join(" ") |
| tst.js:275:9:275:21 | arr.join(" ") |
| 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,'') |
| 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,'') |
| 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,'') |
| 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, '') |
edges
| tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') |
| tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') |
| tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') |
| tst.js:250:9:250:33 | s().rep ... ]/g,'') | tst.js:250:9:250:33 | s().rep ... ]/g,'') |
| tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') |
| tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') |
| tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') |
@@ -35,11 +51,20 @@ edges
| tst.js:274:12:274:94 | s().val ... g , '') | tst.js:274:6:274:94 | arr |
| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") |
| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") |
| tst.js:300:10:300:33 | s().rep ... ]/g,'') | tst.js:300:10:300:33 | s().rep ... ]/g,'') |
| tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') |
| tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') |
| tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') |
#select
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |

15
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js
View File

@@ -214,7 +214,7 @@ function typicalBadHtmlSanitizers(s) {
var s = s().replace(/</g, '');
s = s.replace(/>/g, ''); // NOT OK
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/"/g, '&#34;'); // OK
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/'/g, '&#39;'); // NOT OK
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(/&/g, '&amp;').replace(/'/g, '&#39;'); // OK - single quotes or double quotes both work
s().replace(/</g, '&lt;').replace(/>/g, '&gt').replace(RE, function(match) {/* ... */ }); // OK (probably)
@@ -247,8 +247,8 @@ function incompleteHtmlAttributeSanitization() {
'="' + s().replace(/[&"]/g,'') + '"'; // OK
'="' + s().replace(/[<>&']/g,'') + '"'; // NOT OK
"='" + s().replace(/[<>&"]/g,'') + "'"; // OK (but given the context, it is probably not fine)
"='" + s().replace(/[<>&']/g,'') + "'"; // NOT OK (but given the context, it is probably fine)
"='" + s().replace(/[<>&"]/g,'') + "'"; // NOT OK
"='" + s().replace(/[<>&']/g,'') + "'"; // OK
'onFunkyEvent="' + s().replace(/[<>"]/g, '') + '"'; // NOT OK
'<div noise onFunkyEvent="' + s().replace(/[<>"]/g, '') + '"'; // NOT OK
@@ -295,3 +295,12 @@ function moreIncompleteHtmlAttributeSanitization() {
}
'onclick="' + y + '"'; // OK
}
function incompleteHtmlAttributeSanitization2() {
'=\'' + s().replace(/[&<>]/g,'') + '\''; // NOT OK
'=\'' + s().replace(/[<>]/g,'') + '\''; // NOT OK
'=\'' + s().replace(/[&<>"]/g,'') + '\''; // NOT OK
'=\'' + s().replace(/[<>&]/g, '') + '\''; // NOT OK
'="' + s().replace(/[<>&"]/g,'') + '"'; // OK
'=\'' + s().replace(/[<>&']/g,'') + '\''; // OK
}