Skip to content

Commit 304b013

Browse files
esbenaesbena
committed
JS: query and tests for unsafe HTML expansion
1 parent 8e9e3c8 commit 304b013

4 files changed

Lines changed: 115 additions & 0 deletions

File tree

‎javascript/ql/src/Security/CWE-116/UnsafeHtmlExpansion.ql‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* @name Unsafe expansion of shorthand HTML tag
3+
* @description Using regular expressions to expand shorthand HTML
4+
* tags may lead to cross-site scripting vulnerabilities.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @precision very-high
8+
* @id js/unsafe-html-expansion
9+
* @tags correctness
10+
* security
11+
* external/cwe/cwe-079
12+
* external/cwe/cwe-116
13+
*/
14+
15+
import javascript
16+
17+
/**
18+
* A regular expression that captures the name and content of a shorthand HTML tag such as `<div id='foo'/>`.
19+
*/
20+
class ShorthandTagRecognizer extends RegExpLiteral {
21+
ShorthandTagRecognizer() {
22+
exists(RegExpSequence seq, RegExpGroup name, RegExpGroup content |
23+
// `/.../g`
24+
this.isGlobal() and
25+
this = seq.getLiteral() and
26+
// `/<.../`
27+
seq.getChild(0).getConstantValue() = "<" and
28+
// `/...\/>/`
29+
seq.getLastChild().getPredecessor().getConstantValue() = "/" and
30+
seq.getLastChild().getConstantValue() = ">" and
31+
// `/...((...)...).../`
32+
seq.getAChild() = content and
33+
content.getNumber() = 1 and
34+
name.getNumber() = 2 and
35+
name = content.getChild(0).(RegExpSequence).getChild(0) and
36+
// `/...(([a-z]+)...).../` or `/...(([a-z][...]*)...).../`
37+
exists(RegExpQuantifier quant | name.getAChild*() = quant |
38+
quant instanceof RegExpStar or
39+
quant instanceof RegExpPlus
40+
) and
41+
// `/...((...)[^>]*).../`
42+
exists(RegExpCharacterClass lazy |
43+
name.getSuccessor().(RegExpStar).getChild(0) = lazy and
44+
lazy.isInverted() and
45+
lazy.getAChild().getConstantValue() = ">"
46+
)
47+
)
48+
}
49+
50+
/**
51+
* Gets a data flow node that may refer to this regular expression.
52+
*/
53+
DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
54+
t.start() and
55+
result = this.flow()
56+
or
57+
exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
58+
}
59+
}
60+
61+
from ShorthandTagRecognizer regexp, StringReplaceCall replace
62+
where
63+
regexp.ref(DataFlow::TypeTracker::end()).flowsTo(replace.getArgument(0)) and
64+
replace.getRawReplacement().mayHaveStringValue("<$1></$2>")
65+
select replace,
66+
"This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings.",
67+
regexp, "this regular expression"

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
| UnsafeHtmlExpansion.js:6:2:9:2 | html.re ... nded\\n\\t) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:7:3:7:95 | /<(?!ar ... )\\/>/gi | this regular expression |
2+
| UnsafeHtmlExpansion.js:10:2:10:68 | html.re ... panded) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:10:15:10:57 | /<(([a- ... )\\/>/gi | this regular expression |
3+
| UnsafeHtmlExpansion.js:13:2:16:2 | html.re ... nded\\n\\t) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:14:3:14:75 | /<(?!ar ... )\\/>/gi | this regular expression |
4+
| UnsafeHtmlExpansion.js:17:2:17:48 | html.re ... panded) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:17:15:17:37 | /<(([\\w ... )\\/>/gi | this regular expression |
5+
| UnsafeHtmlExpansion.js:20:2:23:2 | html.re ... nded\\n\\t) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:21:3:21:76 | /<(?!ar ... )\\/>/gi | this regular expression |
6+
| UnsafeHtmlExpansion.js:24:2:24:49 | html.re ... panded) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:24:15:24:38 | /<(([\\w ... )\\/>/gi | this regular expression |
7+
| UnsafeHtmlExpansion.js:26:2:26:39 | html.re ... panded) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:2:23:2:45 | /<(([\\w ... )\\/>/gi | this regular expression |
8+
| UnsafeHtmlExpansion.js:30:2:30:37 | html.re ... panded) | This HTML tag expansion may disable earlier sanitizations as $@ may match unintended strings. | UnsafeHtmlExpansion.js:2:23:2:45 | /<(([\\w ... )\\/>/gi | this regular expression |

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
(function(){
2+
let defaultPattern = /<(([\w:]+)[^>]*)\/>/gi;
3+
let expanded = "<$1></$2>";
4+
5+
// lib1
6+
html.replace(
7+
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([a-z][^\/\0>\x20\t\r\n\f]*)[^>]*)\/>/gi,
8+
expanded
9+
); // NOT OK
10+
html.replace(/<(([a-z][^\/\0>\x20\t\r\n\f]*)[^>]*)\/>/gi, expanded); // NOT OK
11+
12+
// lib2
13+
html.replace(
14+
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:]+)[^>]*)\/>/gi,
15+
expanded
16+
); // NOT OK
17+
html.replace(/<(([\w:]+)[^>]*)\/>/gi, expanded); // NOT OK
18+
19+
// lib3
20+
html.replace(
21+
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:-]+)[^>]*)\/>/gi,
22+
expanded
23+
); // NOT OK
24+
html.replace(/<(([\w:-]+)[^>]*)\/>/gi, expanded); // NOT OK
25+
26+
html.replace(defaultPattern, expanded); // NOT OK
27+
function getPattern() {
28+
return defaultPattern;
29+
}
30+
html.replace(getPattern(), expanded); // NOT OK
31+
32+
function getExpanded() {
33+
return expanded;
34+
}
35+
html.replace(defaultPattern, getExpanded()); // NOT OK (but not tracking the expansion string)
36+
html.replace(defaultPattern, something); // OK (possibly)
37+
defaultPattern.match(something); // OK (possibly)
38+
getPattern().match(something); // OK (possibly)
39+
});

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/UnsafeHtmlExpansion.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-116/UnsafeHtmlExpansion.ql

0 commit comments

Comments
 (0)