Skip to content

Commit 7265e94

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3578 from erik-krogh/HtmlGuard
Approved by asgerf
2 parents 712c53a + dfd35ae commit 7265e94

File tree

6 files changed

+267
-1
lines changed

6 files changed

+267
-1
lines changed

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,10 @@ private predicate barrierGuardBlocksNode(BarrierGuardNode guard, DataFlow::Node
438438
barrierGuardIsRelevant(guard) and
439439
exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
440440
nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
441-
guard.getEnclosingExpr() = cond.getTest() and
441+
(
442+
guard.getEnclosingExpr() = cond.getTest() or
443+
guard = cond.getTest().flow().getImmediatePredecessor+()
444+
) and
442445
outcome = cond.getOutcome() and
443446
barrierGuardBlocksAccessPath(guard, outcome, p, label) and
444447
cond.dominates(bb)

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,66 @@ module Shared {
7373
e = this.getBaseString().getEnclosingExpr() and outcome = this.getPolarity().booleanNot()
7474
}
7575
}
76+
77+
/**
78+
* A sanitizer guard that checks for the existence of HTML chars in a string.
79+
* E.g. `/["'&<>]/.exec(str)`.
80+
*/
81+
class ContainsHTMLGuard extends SanitizerGuard, DataFlow::MethodCallNode {
82+
DataFlow::RegExpCreationNode regExp;
83+
84+
ContainsHTMLGuard() {
85+
this.getMethodName() = ["test", "exec"] and
86+
this.getReceiver().getALocalSource() = regExp and
87+
regExp.getRoot() instanceof RegExpCharacterClass and
88+
forall(string s | s = ["\"", "&", "<", ">"] | regExp.getRoot().getAMatchedString() = s)
89+
}
90+
91+
override predicate sanitizes(boolean outcome, Expr e) {
92+
outcome = false and e = this.getArgument(0).asExpr()
93+
}
94+
}
95+
96+
/**
97+
* Holds if `str` is used in a switch-case that has cases matching HTML escaping.
98+
*/
99+
private predicate isUsedInHTMLEscapingSwitch(Expr str) {
100+
exists(SwitchStmt switch |
101+
// "\"".charCodeAt(0) == 34, "&".charCodeAt(0) == 38, "<".charCodeAt(0) == 60
102+
forall(int c | c = [34, 38, 60] | c = switch.getACase().getExpr().getIntValue()) and
103+
exists(DataFlow::MethodCallNode mcn | mcn.getMethodName() = "charCodeAt" |
104+
mcn.flowsToExpr(switch.getExpr()) and
105+
str = mcn.getReceiver().asExpr()
106+
)
107+
or
108+
forall(string c | c = ["\"", "&", "<"] | c = switch.getACase().getExpr().getStringValue()) and
109+
(
110+
exists(DataFlow::MethodCallNode mcn | mcn.getMethodName() = "charAt" |
111+
mcn.flowsToExpr(switch.getExpr()) and
112+
str = mcn.getReceiver().asExpr()
113+
)
114+
or
115+
exists(DataFlow::PropRead read | exists(read.getPropertyNameExpr()) |
116+
read.flowsToExpr(switch.getExpr()) and
117+
str = read.getBase().asExpr()
118+
)
119+
)
120+
)
121+
}
122+
123+
/**
124+
* Gets an Ssa variable that is used in a sanitizing switch statement.
125+
* The `pragma[noinline]` is to avoid materializing a cartesian product.
126+
*/
127+
pragma[noinline]
128+
private SsaVariable getAPathEscapedInSwitch() { isUsedInHTMLEscapingSwitch(result.getAUse()) }
129+
130+
/**
131+
* An expression that is sanitized by a switch-case.
132+
*/
133+
class IsEscapedInSwitchSanitizer extends Sanitizer {
134+
IsEscapedInSwitchSanitizer() { this.asExpr() = getAPathEscapedInSwitch().getAUse() }
135+
}
76136
}
77137

78138
/** Provides classes and predicates for the DOM-based XSS query. */
@@ -328,6 +388,8 @@ module DomBasedXss {
328388

329389
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
330390

391+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
392+
331393
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
332394

333395
/**
@@ -359,6 +421,8 @@ module DomBasedXss {
359421
)
360422
)
361423
}
424+
425+
private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
362426
}
363427

364428
/** Provides classes and predicates for the reflected XSS query. */
@@ -462,7 +526,11 @@ module ReflectedXss {
462526

463527
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
464528

529+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
530+
465531
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
532+
533+
private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
466534
}
467535

468536
/** Provides classes and predicates for the stored XSS query. */
@@ -495,7 +563,11 @@ module StoredXss {
495563

496564
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
497565

566+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
567+
498568
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
569+
570+
private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
499571
}
500572

501573
/** Provides classes and predicates for the XSS through DOM query. */

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ nodes
1919
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
2020
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
2121
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
22+
| ReflectedXssGood3.js:135:9:135:27 | url |
23+
| ReflectedXssGood3.js:135:15:135:27 | req.params.id |
24+
| ReflectedXssGood3.js:135:15:135:27 | req.params.id |
25+
| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
26+
| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
27+
| ReflectedXssGood3.js:139:24:139:26 | url |
2228
| etherpad.js:9:5:9:53 | response |
2329
| etherpad.js:9:16:9:30 | req.query.jsonp |
2430
| etherpad.js:9:16:9:30 | req.query.jsonp |
@@ -105,6 +111,11 @@ edges
105111
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
106112
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
107113
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
114+
| ReflectedXssGood3.js:135:9:135:27 | url | ReflectedXssGood3.js:139:24:139:26 | url |
115+
| ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:135:9:135:27 | url |
116+
| ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:135:9:135:27 | url |
117+
| ReflectedXssGood3.js:139:24:139:26 | url | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
118+
| ReflectedXssGood3.js:139:24:139:26 | url | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
108119
| etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
109120
| etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
110121
| etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
@@ -166,6 +177,7 @@ edges
166177
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
167178
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
168179
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
180+
| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | Cross-site scripting vulnerability due to $@. | ReflectedXssGood3.js:135:15:135:27 | req.params.id | user-provided value |
169181
| etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
170182
| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
171183
| formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssGood.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,39 @@ app.get('/echo', function(req, res) {
4949
res.setHeader('Content-Length', msg.length);
5050
res.end(msg);
5151
});
52+
53+
app.get('/user/:id', function(req, res) {
54+
const url = req.params.id;
55+
if (!/["'&<>]/.exec(url)) {
56+
res.send(url); // OK
57+
}
58+
});
59+
60+
function escapeHtml1 (str) {
61+
if (!/["'&<>]/.exec(str)) {
62+
return str;
63+
}
64+
}
65+
66+
app.get('/user/:id', function(req, res) {
67+
const url = req.params.id;
68+
69+
res.send(escapeHtml1(url)); // OK
70+
});
71+
72+
const matchHtmlRegExp = /["'&<>]/;
73+
function escapeHtml2 (string) {
74+
const str = '' + string;
75+
const match = matchHtmlRegExp.exec(str);
76+
77+
if (!match) {
78+
return str;
79+
}
80+
}
81+
82+
app.get('/user/:id', function(req, res) {
83+
const url = req.params.id;
84+
85+
res.send(escapeHtml2(url)); // OK
86+
});
87+

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssGood3.js

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
var express = require('express');
2+
3+
var app = express();
4+
5+
function escapeHtml1(string) {
6+
var str = "" + string;
7+
let escape;
8+
let html = '';
9+
let lastIndex = 0;
10+
11+
for (let index = 0; index < str.length; index++) {
12+
switch (str.charCodeAt(index)) {
13+
case 34: // "
14+
escape = '&quot;';
15+
break;
16+
case 38: // &
17+
escape = '&amp;';
18+
break;
19+
case 39: // '
20+
escape = '&#39;';
21+
break;
22+
case 60: // <
23+
escape = '&lt;';
24+
break;
25+
case 62: // >
26+
escape = '&gt;';
27+
break;
28+
default:
29+
continue;
30+
}
31+
32+
if (lastIndex !== index) {
33+
html += str.substring(lastIndex, index);
34+
}
35+
36+
lastIndex = index + 1;
37+
html += escape;
38+
}
39+
40+
return lastIndex !== index
41+
? html + str.substring(lastIndex, index)
42+
: html;
43+
}
44+
45+
function escapeHtml2(s) {
46+
var buf = "";
47+
while (i < s.length) {
48+
var ch = s[i++];
49+
switch (ch) {
50+
case '&':
51+
buf += '&amp;';
52+
break;
53+
case '<':
54+
buf += '&lt;';
55+
break;
56+
case '\"':
57+
buf += '&quot;';
58+
break;
59+
default:
60+
buf += ch;
61+
break;
62+
}
63+
}
64+
return buf;
65+
}
66+
67+
68+
function escapeHtml3(value) {
69+
var i = 0;
70+
var XMLChars = {
71+
AMP: 38, // "&"
72+
QUOT: 34, // "\""
73+
LT: 60, // "<"
74+
GT: 62, // ">"
75+
};
76+
77+
var parts = [value.substring(0, i)];
78+
while (i < length) {
79+
switch (ch) {
80+
case XMLChars.AMP:
81+
parts.push('&amp;');
82+
break;
83+
case XMLChars.QUOT:
84+
parts.push('&quot;');
85+
break;
86+
case XMLChars.LT:
87+
parts.push('&lt;');
88+
break;
89+
case XMLChars.GT:
90+
parts.push('&gt;');
91+
break;
92+
}
93+
++i;
94+
var j = i;
95+
while (i < length) {
96+
ch = value.charCodeAt(i);
97+
if (ch === XMLChars.AMP ||
98+
ch === XMLChars.QUOT || ch === XMLChars.LT ||
99+
ch === XMLChars.GT) {
100+
break;
101+
}
102+
i++;
103+
}
104+
if (j < i) {
105+
parts.push(value.substring(j, i));
106+
}
107+
}
108+
return parts.join('');
109+
}
110+
111+
112+
function escapeHtml4(s) {
113+
var buf = "";
114+
while (i < s.length) {
115+
var ch = s.chatAt(i++);
116+
switch (ch) {
117+
case '&':
118+
buf += '&amp;';
119+
break;
120+
case '<':
121+
buf += '&lt;';
122+
break;
123+
case '\"':
124+
buf += '&quot;';
125+
break;
126+
default:
127+
buf += ch;
128+
break;
129+
}
130+
}
131+
return buf;
132+
}
133+
134+
app.get('/user/:id', function (req, res) {
135+
const url = req.params.id;
136+
137+
res.send(escapeHtml1(url)); // OK
138+
res.send(escapeHtml2(url)); // OK
139+
res.send(escapeHtml3(url)); // OK - but FP
140+
res.send(escapeHtml4(url)); // OK
141+
});
142+

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
44
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
55
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
6+
| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | Cross-site scripting vulnerability due to $@. | ReflectedXssGood3.js:135:15:135:27 | req.params.id | user-provided value |
67
| exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
78
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
89
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |

0 commit comments

Comments
 (0)