Skip to content

Commit 86e31a5

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #447 from esben-semmle/js/indirect-sanitization
Approved by asger-semmle
2 parents 2f0e693 + eaad84b commit 86e31a5

File tree

7 files changed

+242
-10
lines changed

7 files changed

+242
-10
lines changed

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import javascript
1717
import semmle.javascript.dataflow.CallGraph
18+
private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps
1819
private import semmle.javascript.dataflow.InferredTypes
1920

2021
/**
@@ -809,6 +810,74 @@ module TaintTracking {
809810

810811
}
811812

813+
/**
814+
* A function that returns the result of a sanitizer check.
815+
*/
816+
private class SanitizingFunction extends Function {
817+
Parameter sanitizedParameter;
818+
819+
SanitizerGuardNode sanitizer;
820+
821+
boolean sanitizerOutcome;
822+
823+
SanitizingFunction() {
824+
exists(Expr e |
825+
exists(Expr returnExpr |
826+
returnExpr = sanitizer.asExpr()
827+
or
828+
// ad hoc support for conjunctions:
829+
returnExpr.(LogAndExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = true
830+
or
831+
// ad hoc support for disjunctions:
832+
returnExpr.(LogOrExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = false
833+
|
834+
exists(SsaExplicitDefinition ssa |
835+
ssa.getDef().getSource() = returnExpr and
836+
ssa.getVariable().getAUse() = getAReturnedExpr()
837+
)
838+
or
839+
returnExpr = getAReturnedExpr()
840+
) and
841+
DataFlow::parameterNode(sanitizedParameter).flowsToExpr(e) and
842+
sanitizer.sanitizes(sanitizerOutcome, e)
843+
) and
844+
getNumParameter() = 1 and
845+
sanitizedParameter = getParameter(0)
846+
}
847+
848+
/**
849+
* Holds if this function sanitizes argument `e` of call `call`, provided the call evaluates to `outcome`.
850+
*/
851+
predicate isSanitizingCall(DataFlow::CallNode call, Expr e, boolean outcome) {
852+
exists(DataFlow::Node arg |
853+
arg.asExpr() = e and
854+
arg = call.getArgument(0) and
855+
call.getNumArgument() = 1 and
856+
FlowSteps::argumentPassing(call, arg, this, sanitizedParameter) and
857+
outcome = sanitizerOutcome
858+
)
859+
}
860+
861+
/**
862+
* Holds if this function applies to the flow in `cfg`.
863+
*/
864+
predicate appliesTo(Configuration cfg) {
865+
cfg.isBarrierGuard(sanitizer)
866+
}
867+
}
868+
869+
/**
870+
* A call that sanitizes an argument.
871+
*/
872+
private class AdditionalSanitizingCall extends AdditionalSanitizerGuardNode, DataFlow::CallNode {
873+
SanitizingFunction f;
874+
875+
AdditionalSanitizingCall() { f.isSanitizingCall(this, _, _) }
876+
877+
override predicate sanitizes(boolean outcome, Expr e) { f.isSanitizingCall(this, e, outcome) }
878+
879+
override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) }
880+
}
812881

813882
/**
814883
* An equality test on `e.origin` or `e.source` where `e` is a `postMessage` event object,

‎javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,25 @@
3838
| tst.js:226:9:226:26 | -1 >= o.indexOf(v) | ExampleConfiguration | false | tst.js:226:25:226:25 | v |
3939
| tst.js:236:9:236:24 | isWhitelisted(v) | ExampleConfiguration | true | tst.js:236:23:236:23 | v |
4040
| tst.js:240:9:240:28 | config.allowValue(v) | ExampleConfiguration | true | tst.js:240:27:240:27 | v |
41+
| tst.js:252:16:252:36 | whiteli ... ains(x) | ExampleConfiguration | true | tst.js:252:35:252:35 | x |
42+
| tst.js:254:9:254:12 | f(v) | ExampleConfiguration | true | tst.js:254:11:254:11 | v |
43+
| tst.js:261:25:261:45 | whiteli ... ains(y) | ExampleConfiguration | true | tst.js:261:44:261:44 | y |
44+
| tst.js:264:9:264:12 | g(v) | ExampleConfiguration | true | tst.js:264:11:264:11 | v |
45+
| tst.js:271:25:271:45 | whiteli ... ains(z) | ExampleConfiguration | true | tst.js:271:44:271:44 | z |
46+
| tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:16:281:17 | x2 |
47+
| tst.js:281:30:281:51 | whiteli ... ins(x2) | ExampleConfiguration | true | tst.js:281:49:281:50 | x2 |
48+
| tst.js:283:9:283:13 | f2(v) | ExampleConfiguration | true | tst.js:283:12:283:12 | v |
49+
| tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:16:290:17 | x3 |
50+
| tst.js:290:30:290:51 | whiteli ... ins(x3) | ExampleConfiguration | true | tst.js:290:49:290:50 | x3 |
51+
| tst.js:299:17:299:38 | whiteli ... ins(x4) | ExampleConfiguration | true | tst.js:299:36:299:37 | x4 |
52+
| tst.js:308:18:308:39 | whiteli ... ins(x5) | ExampleConfiguration | true | tst.js:308:37:308:38 | x5 |
53+
| tst.js:317:26:317:47 | whiteli ... ins(x6) | ExampleConfiguration | true | tst.js:317:45:317:46 | x6 |
54+
| tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:25:327:26 | x7 |
55+
| tst.js:327:39:327:60 | whiteli ... ins(x7) | ExampleConfiguration | true | tst.js:327:58:327:59 | x7 |
56+
| tst.js:330:9:330:13 | f7(v) | ExampleConfiguration | true | tst.js:330:12:330:12 | v |
57+
| tst.js:337:25:337:46 | whiteli ... ins(x8) | ExampleConfiguration | true | tst.js:337:44:337:45 | x8 |
58+
| tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:16:338:17 | x8 |
59+
| tst.js:347:29:347:50 | whiteli ... ins(x9) | ExampleConfiguration | true | tst.js:347:48:347:49 | x9 |
60+
| tst.js:356:16:356:27 | x10 !== null | ExampleConfiguration | false | tst.js:356:16:356:18 | x10 |
61+
| tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:32:356:34 | x10 |
62+
| tst.js:358:9:358:14 | f10(v) | ExampleConfiguration | false | tst.js:358:13:358:13 | v |

‎javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,23 @@
3636
| tst.js:227:14:227:14 | v | tst.js:199:13:199:20 | SOURCE() |
3737
| tst.js:239:14:239:14 | v | tst.js:235:13:235:20 | SOURCE() |
3838
| tst.js:243:14:243:14 | v | tst.js:235:13:235:20 | SOURCE() |
39+
| tst.js:249:10:249:10 | v | tst.js:248:13:248:20 | SOURCE() |
40+
| tst.js:257:14:257:14 | v | tst.js:248:13:248:20 | SOURCE() |
41+
| tst.js:267:14:267:14 | v | tst.js:248:13:248:20 | SOURCE() |
42+
| tst.js:275:14:275:14 | v | tst.js:248:13:248:20 | SOURCE() |
43+
| tst.js:277:14:277:14 | v | tst.js:248:13:248:20 | SOURCE() |
44+
| tst.js:286:14:286:14 | v | tst.js:248:13:248:20 | SOURCE() |
45+
| tst.js:293:14:293:14 | v | tst.js:248:13:248:20 | SOURCE() |
46+
| tst.js:295:14:295:14 | v | tst.js:248:13:248:20 | SOURCE() |
47+
| tst.js:302:14:302:14 | v | tst.js:248:13:248:20 | SOURCE() |
48+
| tst.js:304:14:304:14 | v | tst.js:248:13:248:20 | SOURCE() |
49+
| tst.js:311:14:311:14 | v | tst.js:248:13:248:20 | SOURCE() |
50+
| tst.js:313:14:313:14 | v | tst.js:248:13:248:20 | SOURCE() |
51+
| tst.js:321:14:321:14 | v | tst.js:248:13:248:20 | SOURCE() |
52+
| tst.js:323:14:323:14 | v | tst.js:248:13:248:20 | SOURCE() |
53+
| tst.js:333:14:333:14 | v | tst.js:248:13:248:20 | SOURCE() |
54+
| tst.js:341:14:341:14 | v | tst.js:248:13:248:20 | SOURCE() |
55+
| tst.js:343:14:343:14 | v | tst.js:248:13:248:20 | SOURCE() |
56+
| tst.js:350:14:350:14 | v | tst.js:248:13:248:20 | SOURCE() |
57+
| tst.js:352:14:352:14 | v | tst.js:248:13:248:20 | SOURCE() |
58+
| tst.js:359:14:359:14 | v | tst.js:248:13:248:20 | SOURCE() |

‎javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,9 @@
3131
| tst.js:229:14:229:14 | v | ExampleConfiguration |
3232
| tst.js:237:14:237:14 | v | ExampleConfiguration |
3333
| tst.js:241:14:241:14 | v | ExampleConfiguration |
34+
| tst.js:255:14:255:14 | v | ExampleConfiguration |
35+
| tst.js:265:14:265:14 | v | ExampleConfiguration |
36+
| tst.js:284:14:284:14 | v | ExampleConfiguration |
37+
| tst.js:331:14:331:14 | v | ExampleConfiguration |
38+
| tst.js:356:16:356:27 | x10 | ExampleConfiguration |
39+
| tst.js:361:14:361:14 | v | ExampleConfiguration |

‎javascript/ql/test/library-tests/TaintBarriers/tst.js‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,3 +243,122 @@ function adhocWhitelisting() {
243243
SINK(v);
244244

245245
}
246+
247+
function IndirectSanitizer () {
248+
var v = SOURCE();
249+
SINK(v);
250+
251+
function f(x) {
252+
return whitelist.contains(x);
253+
}
254+
if (f(v)) {
255+
SINK(v);
256+
} else {
257+
SINK(v);
258+
}
259+
260+
function g(y) {
261+
var sanitized = whitelist.contains(y);
262+
return sanitized;
263+
}
264+
if (g(v)) {
265+
SINK(v);
266+
} else {
267+
SINK(v);
268+
}
269+
270+
function h(z) {
271+
var sanitized = whitelist.contains(z);
272+
return somethingElse();
273+
}
274+
if (h(v)) {
275+
SINK(v);
276+
} else {
277+
SINK(v);
278+
}
279+
280+
function f2(x2) {
281+
return x2 != null && whitelist.contains(x2);
282+
}
283+
if (f2(v)) {
284+
SINK(v);
285+
} else {
286+
SINK(v);
287+
}
288+
289+
function f3(x3) {
290+
return x3 == null || whitelist.contains(x3);
291+
}
292+
if (f3(v)) {
293+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
294+
} else {
295+
SINK(v);
296+
}
297+
298+
function f4(x4) {
299+
return !whitelist.contains(x4);
300+
}
301+
if (f4(v)) {
302+
SINK(v);
303+
} else {
304+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
305+
}
306+
307+
function f5(x5) {
308+
return !!whitelist.contains(x5);
309+
}
310+
if (f5(v)) {
311+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
312+
} else {
313+
SINK(v);
314+
}
315+
316+
function f6(x6) {
317+
var sanitized = !whitelist.contains(x6);
318+
return !sanitized;
319+
}
320+
if (f6(v)) {
321+
SINK(v);
322+
} else {
323+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
324+
}
325+
326+
function f7(x7) {
327+
var sanitized = x7 != null && whitelist.contains(x7);
328+
return sanitized;
329+
}
330+
if (f7(v)) {
331+
SINK(v);
332+
} else {
333+
SINK(v);
334+
}
335+
336+
function f8(x8) {
337+
var sanitized = whitelist.contains(x8);
338+
return x8 != null && sanitized;
339+
}
340+
if (f8(v)) {
341+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
342+
} else {
343+
SINK(v);
344+
}
345+
346+
function f9(x9) {
347+
return unknown() && whitelist.contains(x9) && unknown();
348+
}
349+
if (f9(v)) {
350+
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
351+
} else {
352+
SINK(v);
353+
}
354+
355+
function f10(x10) {
356+
return x10 !== null || x10 !== undefined;
357+
}
358+
if (f10(v)) {
359+
SINK(v);
360+
} else {
361+
SINK(v);
362+
}
363+
364+
}

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22
| express.js:12:26:12:44 | req.param("target") | Untrusted URL redirection due to $@. | express.js:12:26:12:44 | req.param("target") | user-provided value |
33
| express.js:33:18:33:23 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value |
44
| express.js:35:16:35:21 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value |
5-
| express.js:44:16:44:108 | (req.pa ... ntacts" | Untrusted URL redirection due to $@. | express.js:44:69:44:87 | req.param('action') | user-provided value |
6-
| express.js:53:26:53:28 | url | Untrusted URL redirection due to $@. | express.js:48:16:48:28 | req.params[0] | user-provided value |
7-
| express.js:78:16:78:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:78:19:78:37 | req.param("target") | user-provided value |
8-
| express.js:94:18:94:23 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |
9-
| express.js:101:16:101:21 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |
10-
| express.js:122:16:122:72 | [req.qu ... oin('') | Untrusted URL redirection due to $@. | express.js:122:17:122:30 | req.query.page | user-provided value |
5+
| express.js:40:16:40:108 | (req.pa ... ntacts" | Untrusted URL redirection due to $@. | express.js:40:69:40:87 | req.param('action') | user-provided value |
6+
| express.js:49:26:49:28 | url | Untrusted URL redirection due to $@. | express.js:44:16:44:28 | req.params[0] | user-provided value |
7+
| express.js:74:16:74:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:74:19:74:37 | req.param("target") | user-provided value |
8+
| express.js:90:18:90:23 | target | Untrusted URL redirection due to $@. | express.js:83:16:83:34 | req.param("target") | user-provided value |
9+
| express.js:97:16:97:21 | target | Untrusted URL redirection due to $@. | express.js:83:16:83:34 | req.param("target") | user-provided value |
10+
| express.js:118:16:118:72 | [req.qu ... oin('') | Untrusted URL redirection due to $@. | express.js:118:17:118:30 | req.query.page | user-provided value |
1111
| node.js:7:34:7:39 | target | Untrusted URL redirection due to $@. | node.js:6:26:6:32 | req.url | user-provided value |
1212
| node.js:15:34:15:45 | '/' + target | Untrusted URL redirection due to $@. | node.js:11:26:11:32 | req.url | user-provided value |
1313
| node.js:32:34:32:55 | target ... =" + me | Untrusted URL redirection due to $@. | node.js:29:26:29:32 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@ app.get('/some/path', function(req, res) {
3535
res.redirect(target);
3636
});
3737

38-
function isLocalURL(target) {
39-
return new RegExp("^/(?![/\\])|^~/").exec(target);
40-
}
41-
4238
app.get('/foo', function(req, res) {
4339
// BAD: may be a global redirection
4440
res.redirect((req.param('action') && req.param('action') != "") ? req.param('action') : "/google_contacts")

0 commit comments

Comments
 (0)