Skip to content

Commit 62512dd

Browse files
erik-krogherik-krogh
committed
expand the js/exception-xss to handle more types of exceptional flow
1 parent 3e0045f commit 62512dd

4 files changed

Lines changed: 199 additions & 8 deletions

File tree

‎javascript/ql/src/semmle/javascript/StandardLibrary.qll‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,21 @@ private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
209209
}
210210
}
211211

212+
/**
213+
* A data flow edge from the exceptional return of the promise executor to the promise catch handler.
214+
*/
215+
class PromiseExceptionalStep extends DataFlow::AdditionalFlowStep {
216+
PromiseDefinition promise;
217+
PromiseExceptionalStep() {
218+
promise = this
219+
}
220+
221+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
222+
pred = promise.getExecutor().getExceptionalReturn() and
223+
succ = promise.getACatchHandler().getParameter(0)
224+
}
225+
}
226+
212227
/**
213228
* Holds if taint propagates from `pred` to `succ` through promises.
214229
*/

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

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ module ExceptionXss {
1010
import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom
1111
import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom
1212
import Xss as Xss
13-
13+
private import semmle.javascript.dataflow.InferredTypes
14+
1415
/**
1516
* Holds if `node` is unlikely to cause an exception containing sensitive information to be thrown.
1617
*/
@@ -24,16 +25,31 @@ module ExceptionXss {
2425
node = DataFlow::globalVarRef("console").getAMemberCall(_).getAnArgument()
2526
}
2627

28+
/**
29+
* Holds if `t` is `null` or `undefined`.
30+
*/
31+
private predicate isNullOrUndefined(InferredType t) {
32+
t = TTNull() or
33+
t = TTUndefined()
34+
}
35+
2736
/**
2837
* Holds if `node` can possibly cause an exception containing sensitive information to be thrown.
2938
*/
3039
predicate canThrowSensitiveInformation(DataFlow::Node node) {
31-
not isUnlikelyToThrowSensitiveInformation(node) and
40+
not isUnlikelyToThrowSensitiveInformation(node) and
3241
(
3342
// in the case of reflective calls the below ensures that both InvokeNodes have no known callee.
34-
forex(DataFlow::InvokeNode call | node = call.getAnArgument() | not exists(call.getACallee()))
43+
forex(DataFlow::InvokeNode call | call = getEnclosingCallNode(node) |
44+
not exists(call.getACallee())
45+
)
3546
or
3647
node.asExpr().getEnclosingStmt() instanceof ThrowStmt
48+
or
49+
exists(DataFlow::PropRef prop |
50+
node.getEnclosingExpr() = prop.getPropertyNameExpr() and
51+
isNullOrUndefined(prop.getBase().analyze().getAType())
52+
)
3753
)
3854
}
3955

@@ -47,6 +63,50 @@ module ExceptionXss {
4763
NotYetThrown() { this = "NotYetThrown" }
4864
}
4965

66+
// Consider using "if (err) {.. [do something with err] .. }" as an extra condition if there are too many FP's.
67+
class Callback extends DataFlow::FunctionNode {
68+
Callback() {
69+
exists(DataFlow::CallNode call | call.getLastArgument().getAFunctionValue() = this) and
70+
this.getNumParameter() = 2 and
71+
this.getParameter(0).getName().regexpMatch("err.*") // Using "e" was considered. But that matches too many jQuery methods where "element" is shortened as "e".
72+
}
73+
74+
DataFlow::Node getErrorParam() { result = this.getParameter(0) }
75+
}
76+
77+
DataFlow::CallNode getEnclosingCallNode(DataFlow::Node node) {
78+
result.getEnclosingExpr() = getEnclosingCall(node.getEnclosingExpr())
79+
}
80+
81+
InvokeExpr getEnclosingCall(Expr e) {
82+
exists(Expr arg | arg = result.getAnArgument() |
83+
e.getParentExpr*() = arg and
84+
not exists(Expr mid | mid = any(InvokeExpr i) or mid = any(Function f) |
85+
e.getParentExpr+() = mid and mid.getParentExpr+() = result
86+
)
87+
)
88+
}
89+
90+
// `someFunction(.. <pred> .., (<result>, value) => {...}).
91+
DataFlow::Node getCallbackErrorParam(DataFlow::Node pred) {
92+
exists(DataFlow::CallNode call, Callback callback |
93+
getEnclosingCallNode(pred) = call and
94+
call.getLastArgument() = callback and
95+
result = callback.getErrorParam() and
96+
not pred = callback
97+
)
98+
}
99+
100+
/**
101+
* Gets the DataFlow::Node where an exception would flow to if `pred` is used in some context
102+
* where an exception could potentially be thrown.
103+
*/
104+
DataFlow::Node getWhereExceptionWouldFlow(DataFlow::Node pred) {
105+
result = pred.asExpr().getExceptionTarget()
106+
or
107+
result = getCallbackErrorParam(pred)
108+
}
109+
50110
/**
51111
* A taint-tracking configuration for reasoning about XSS with possible exceptional flow.
52112
* Flow labels are used to ensure that we only report taint-flow that has been thrown in
@@ -65,16 +125,20 @@ module ExceptionXss {
65125

66126
override predicate isSanitizer(DataFlow::Node node) { node instanceof Xss::Shared::Sanitizer }
67127

128+
cached
68129
override predicate isAdditionalFlowStep(
69130
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl,
70131
DataFlow::FlowLabel outlbl
71132
) {
72-
inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
73-
succ = pred.asExpr().getExceptionTarget() and
74-
canThrowSensitiveInformation(pred)
133+
inlbl instanceof NotYetThrown and
134+
(outlbl.isTaint() or outlbl instanceof NotYetThrown) and
135+
canThrowSensitiveInformation(pred) and
136+
succ = getWhereExceptionWouldFlow(pred)
75137
or
76138
// All the usual taint-flow steps apply on data-flow before it has been thrown in an exception.
77-
this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown
139+
this.isAdditionalFlowStep(pred, succ) and
140+
inlbl instanceof NotYetThrown and
141+
outlbl instanceof NotYetThrown
78142
}
79143
}
80144
}

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ nodes
1616
| exception-xss.js:22:10:22:10 | e |
1717
| exception-xss.js:23:18:23:18 | e |
1818
| exception-xss.js:23:18:23:18 | e |
19+
| exception-xss.js:27:18:27:20 | foo |
20+
| exception-xss.js:28:10:28:10 | e |
21+
| exception-xss.js:29:18:29:18 | e |
22+
| exception-xss.js:29:18:29:18 | e |
1923
| exception-xss.js:33:11:33:22 | ["bar", foo] |
2024
| exception-xss.js:33:19:33:21 | foo |
2125
| exception-xss.js:34:10:34:10 | e |
@@ -59,6 +63,32 @@ nodes
5963
| exception-xss.js:129:10:129:10 | e |
6064
| exception-xss.js:130:18:130:18 | e |
6165
| exception-xss.js:130:18:130:18 | e |
66+
| exception-xss.js:136:11:136:23 | req.params.id |
67+
| exception-xss.js:136:11:136:23 | req.params.id |
68+
| exception-xss.js:136:27:136:31 | error |
69+
| exception-xss.js:138:19:138:23 | error |
70+
| exception-xss.js:138:19:138:23 | error |
71+
| exception-xss.js:146:9:146:38 | foo |
72+
| exception-xss.js:146:15:146:31 | document.location |
73+
| exception-xss.js:146:15:146:31 | document.location |
74+
| exception-xss.js:146:15:146:38 | documen ... .search |
75+
| exception-xss.js:148:33:148:35 | foo |
76+
| exception-xss.js:148:55:148:55 | e |
77+
| exception-xss.js:149:22:149:22 | e |
78+
| exception-xss.js:149:22:149:22 | e |
79+
| exception-xss.js:153:9:153:11 | foo |
80+
| exception-xss.js:154:13:154:13 | e |
81+
| exception-xss.js:155:19:155:19 | e |
82+
| exception-xss.js:155:19:155:19 | e |
83+
| exception-xss.js:159:14:159:16 | foo |
84+
| exception-xss.js:160:13:160:13 | e |
85+
| exception-xss.js:161:19:161:19 | e |
86+
| exception-xss.js:161:19:161:19 | e |
87+
| exception-xss.js:174:25:174:43 | exceptional return of inner(foo, resolve) |
88+
| exception-xss.js:174:31:174:33 | foo |
89+
| exception-xss.js:174:53:174:53 | e |
90+
| exception-xss.js:175:22:175:22 | e |
91+
| exception-xss.js:175:22:175:22 | e |
6292
| tst.js:298:9:298:16 | location |
6393
| tst.js:298:9:298:16 | location |
6494
| tst.js:299:10:299:10 | e |
@@ -73,6 +103,7 @@ edges
73103
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:9:11:9:13 | foo |
74104
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:15:9:15:11 | foo |
75105
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:21:11:21:13 | foo |
106+
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:27:18:27:20 | foo |
76107
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:33:19:33:21 | foo |
77108
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:46:16:46:18 | foo |
78109
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:81:16:81:18 | foo |
@@ -89,11 +120,16 @@ edges
89120
| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e |
90121
| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e |
91122
| exception-xss.js:21:11:21:13 | foo | exception-xss.js:21:11:21:21 | foo + "bar" |
123+
| exception-xss.js:21:11:21:13 | foo | exception-xss.js:22:10:22:10 | e |
92124
| exception-xss.js:21:11:21:21 | foo + "bar" | exception-xss.js:22:10:22:10 | e |
93125
| exception-xss.js:22:10:22:10 | e | exception-xss.js:23:18:23:18 | e |
94126
| exception-xss.js:22:10:22:10 | e | exception-xss.js:23:18:23:18 | e |
127+
| exception-xss.js:27:18:27:20 | foo | exception-xss.js:28:10:28:10 | e |
128+
| exception-xss.js:28:10:28:10 | e | exception-xss.js:29:18:29:18 | e |
129+
| exception-xss.js:28:10:28:10 | e | exception-xss.js:29:18:29:18 | e |
95130
| exception-xss.js:33:11:33:22 | ["bar", foo] | exception-xss.js:34:10:34:10 | e |
96131
| exception-xss.js:33:19:33:21 | foo | exception-xss.js:33:11:33:22 | ["bar", foo] |
132+
| exception-xss.js:33:19:33:21 | foo | exception-xss.js:34:10:34:10 | e |
97133
| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e |
98134
| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e |
99135
| exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) | exception-xss.js:47:10:47:10 | e |
@@ -111,6 +147,7 @@ edges
111147
| exception-xss.js:90:10:90:10 | e | exception-xss.js:91:18:91:18 | e |
112148
| exception-xss.js:95:11:95:22 | [foo, "bar"] | exception-xss.js:96:10:96:10 | e |
113149
| exception-xss.js:95:12:95:14 | foo | exception-xss.js:95:11:95:22 | [foo, "bar"] |
150+
| exception-xss.js:95:12:95:14 | foo | exception-xss.js:96:10:96:10 | e |
114151
| exception-xss.js:96:10:96:10 | e | exception-xss.js:97:18:97:18 | e |
115152
| exception-xss.js:96:10:96:10 | e | exception-xss.js:97:18:97:18 | e |
116153
| exception-xss.js:102:12:102:14 | foo | exception-xss.js:106:10:106:10 | e |
@@ -127,6 +164,30 @@ edges
127164
| exception-xss.js:128:11:128:52 | session ... ssion') | exception-xss.js:129:10:129:10 | e |
128165
| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e |
129166
| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e |
167+
| exception-xss.js:136:11:136:23 | req.params.id | exception-xss.js:136:27:136:31 | error |
168+
| exception-xss.js:136:11:136:23 | req.params.id | exception-xss.js:136:27:136:31 | error |
169+
| exception-xss.js:136:27:136:31 | error | exception-xss.js:138:19:138:23 | error |
170+
| exception-xss.js:136:27:136:31 | error | exception-xss.js:138:19:138:23 | error |
171+
| exception-xss.js:146:9:146:38 | foo | exception-xss.js:148:33:148:35 | foo |
172+
| exception-xss.js:146:9:146:38 | foo | exception-xss.js:153:9:153:11 | foo |
173+
| exception-xss.js:146:9:146:38 | foo | exception-xss.js:159:14:159:16 | foo |
174+
| exception-xss.js:146:9:146:38 | foo | exception-xss.js:174:31:174:33 | foo |
175+
| exception-xss.js:146:15:146:31 | document.location | exception-xss.js:146:15:146:38 | documen ... .search |
176+
| exception-xss.js:146:15:146:31 | document.location | exception-xss.js:146:15:146:38 | documen ... .search |
177+
| exception-xss.js:146:15:146:38 | documen ... .search | exception-xss.js:146:9:146:38 | foo |
178+
| exception-xss.js:148:33:148:35 | foo | exception-xss.js:148:55:148:55 | e |
179+
| exception-xss.js:148:55:148:55 | e | exception-xss.js:149:22:149:22 | e |
180+
| exception-xss.js:148:55:148:55 | e | exception-xss.js:149:22:149:22 | e |
181+
| exception-xss.js:153:9:153:11 | foo | exception-xss.js:154:13:154:13 | e |
182+
| exception-xss.js:154:13:154:13 | e | exception-xss.js:155:19:155:19 | e |
183+
| exception-xss.js:154:13:154:13 | e | exception-xss.js:155:19:155:19 | e |
184+
| exception-xss.js:159:14:159:16 | foo | exception-xss.js:160:13:160:13 | e |
185+
| exception-xss.js:160:13:160:13 | e | exception-xss.js:161:19:161:19 | e |
186+
| exception-xss.js:160:13:160:13 | e | exception-xss.js:161:19:161:19 | e |
187+
| exception-xss.js:174:25:174:43 | exceptional return of inner(foo, resolve) | exception-xss.js:174:53:174:53 | e |
188+
| exception-xss.js:174:31:174:33 | foo | exception-xss.js:174:25:174:43 | exceptional return of inner(foo, resolve) |
189+
| exception-xss.js:174:53:174:53 | e | exception-xss.js:175:22:175:22 | e |
190+
| exception-xss.js:174:53:174:53 | e | exception-xss.js:175:22:175:22 | e |
130191
| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e |
131192
| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e |
132193
| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e |
@@ -139,6 +200,7 @@ edges
139200
| exception-xss.js:11:18:11:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:11:18:11:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
140201
| exception-xss.js:17:18:17:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:17:18:17:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
141202
| exception-xss.js:23:18:23:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:23:18:23:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
203+
| exception-xss.js:29:18:29:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:29:18:29:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
142204
| exception-xss.js:35:18:35:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:35:18:35:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
143205
| exception-xss.js:48:18:48:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:48:18:48:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
144206
| exception-xss.js:83:18:83:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:83:18:83:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
@@ -147,5 +209,10 @@ edges
147209
| exception-xss.js:107:18:107:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:107:18:107:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
148210
| exception-xss.js:119:14:119:30 | "Exception: " + e | exception-xss.js:117:13:117:25 | req.params.id | exception-xss.js:119:14:119:30 | "Exception: " + e | Cross-site scripting vulnerability due to $@. | exception-xss.js:117:13:117:25 | req.params.id | user-provided value |
149211
| exception-xss.js:130:18:130:18 | e | exception-xss.js:125:48:125:64 | document.location | exception-xss.js:130:18:130:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:125:48:125:64 | document.location | user-provided value |
212+
| exception-xss.js:138:19:138:23 | error | exception-xss.js:136:11:136:23 | req.params.id | exception-xss.js:138:19:138:23 | error | Cross-site scripting vulnerability due to $@. | exception-xss.js:136:11:136:23 | req.params.id | user-provided value |
213+
| exception-xss.js:149:22:149:22 | e | exception-xss.js:146:15:146:31 | document.location | exception-xss.js:149:22:149:22 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:146:15:146:31 | document.location | user-provided value |
214+
| exception-xss.js:155:19:155:19 | e | exception-xss.js:146:15:146:31 | document.location | exception-xss.js:155:19:155:19 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:146:15:146:31 | document.location | user-provided value |
215+
| exception-xss.js:161:19:161:19 | e | exception-xss.js:146:15:146:31 | document.location | exception-xss.js:161:19:161:19 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:146:15:146:31 | document.location | user-provided value |
216+
| exception-xss.js:175:22:175:22 | e | exception-xss.js:146:15:146:31 | document.location | exception-xss.js:175:22:175:22 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:146:15:146:31 | document.location | user-provided value |
150217
| tst.js:300:20:300:20 | e | tst.js:298:9:298:16 | location | tst.js:300:20:300:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:298:9:298:16 | location | user-provided value |
151218
| tst.js:308:20:308:20 | e | tst.js:305:10:305:17 | location | tst.js:308:20:308:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:305:10:305:17 | location | user-provided value |

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

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
try {
2727
unknown({prop: foo});
2828
} catch(e) {
29-
$('myId').html(e); // We don't flag this for now.
29+
$('myId').html(e); // NOT OK!
3030
}
3131

3232
try {
@@ -130,3 +130,48 @@ app.get('/user/:id', function(req, res) {
130130
$('myId').html(e); // NOT OK
131131
}
132132
})();
133+
134+
135+
app.get('/user/:id', function(req, res) {
136+
unknown(req.params.id, (error, res) => {
137+
if (error) {
138+
$('myId').html(error); // NOT OK
139+
return;
140+
}
141+
$('myId').html(res); // OK (for now?)
142+
});
143+
});
144+
145+
(function () {
146+
var foo = document.location.search;
147+
148+
new Promise(resolve => unknown(foo, resolve)).catch((e) => {
149+
$('myId').html(e); // NOT OK
150+
});
151+
152+
try {
153+
null[foo];
154+
} catch(e) {
155+
$('myId').html(e); // NOT OK
156+
}
157+
158+
try {
159+
unknown()[foo];
160+
} catch(e) {
161+
$('myId').html(e); // NOT OK
162+
}
163+
164+
try {
165+
"foo"[foo]
166+
} catch(e) {
167+
$('myId').html(e); // OK
168+
}
169+
170+
function inner(tainted, resolve) {
171+
unknown(tainted, resolve);
172+
}
173+
174+
new Promise(resolve => inner(foo, resolve)).catch((e) => {
175+
$('myId').html(e); // NOT OK
176+
});
177+
})();

0 commit comments

Comments
 (0)