Skip to content

Commit 5bb308d

Browse files
erik-krogherik-krogh
committed
sanitize variables used in an HTML escaping switch-case
1 parent 1a2db10 commit 5bb308d

File tree

4 files changed

+208
-0
lines changed

4 files changed

+208
-0
lines changed

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,53 @@ module Shared {
9393
}
9494
}
9595

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+
private import semmle.javascript.dataflow.internal.AccessPaths as Paths
124+
125+
/**
126+
* Gets an access-path that is used in a sanitizing switch statement.
127+
* The `pragma[noinline]` is to avoid materializing a cartesian product of all access-paths.
128+
*/
129+
pragma[noinline]
130+
private Paths::AccessPath getAPathEscapedInSwitch() {
131+
exists(Expr str |
132+
isUsedInHTMLEscapingSwitch(str) and
133+
result.getAnInstance() = str
134+
)
135+
}
136+
137+
/**
138+
* An expression that is sanitized by a switch-case.
139+
*/
140+
class IsEscapedInSwitchSanitizer extends Sanitizer {
141+
IsEscapedInSwitchSanitizer() { this.asExpr() = getAPathEscapedInSwitch().getAnInstance() }
142+
}
96143
}
97144

98145
/** Provides classes and predicates for the DOM-based XSS query. */
@@ -348,6 +395,8 @@ module DomBasedXss {
348395

349396
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
350397

398+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
399+
351400
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
352401

353402
/**
@@ -484,6 +533,8 @@ module ReflectedXss {
484533

485534
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
486535

536+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
537+
487538
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
488539

489540
private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
@@ -519,6 +570,8 @@ module StoredXss {
519570

520571
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
521572

573+
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
574+
522575
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
523576

524577
private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }

‎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/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)