Skip to content

Commit a8470a9

Browse files
author
Max Schaefer
committed
JavaScript: Generalise ConstantComparison sanitisers.
In addition to treating comparisons with literals as sanitisers, we now also treat comparisons with variables that have a single assignment as sanitisers. Proving that such a variable is actually a constant is not easy, but for this use case a simple approximation works fine.
1 parent 45a35a8 commit a8470a9

File tree

6 files changed

+55
-3
lines changed

6 files changed

+55
-3
lines changed

‎change-notes/1.21/analysis-javascript.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
* The security queries now track data flow through Base64 decoders such as the Node.js `Buffer` class, the DOM function `atob`, and a number of npm packages intcluding [`abab`](https://www.npmjs.com/package/abab), [`atob`](https://www.npmjs.com/package/atob), [`btoa`](https://www.npmjs.com/package/btoa), [`base-64`](https://www.npmjs.com/package/base-64), [`js-base64`](https://www.npmjs.com/package/js-base64), [`Base64.js`](https://www.npmjs.com/package/Base64) and [`base64-js`](https://www.npmjs.com/package/base64-js).
1515

16+
* The security queries now treat comparisons with symbolic constants as sanitizers, resulting in fewer false positives.
17+
1618
* TypeScript 3.4 features are now supported.
1719

1820

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -743,13 +743,26 @@ module TaintTracking {
743743
override predicate appliesTo(Configuration cfg) { any() }
744744
}
745745

746+
/** Gets a variable that is defined exactly once. */
747+
private Variable singleDef() {
748+
strictcount(result.getADefinition()) = 1
749+
}
750+
746751
/** A check of the form `if(x == 'some-constant')`, which sanitizes `x` in its "then" branch. */
747752
class ConstantComparison extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
748753
Expr x;
749754

750755
override EqualityTest astNode;
751756

752-
ConstantComparison() { astNode.hasOperands(x, any(ConstantExpr c)) }
757+
ConstantComparison() {
758+
exists(Expr const |
759+
astNode.hasOperands(x, const) |
760+
// either the other operand is a constant
761+
const instanceof ConstantExpr or
762+
// or it's an access to a variable that probably acts as a symbolic constant
763+
const = singleDef().getAnAccess()
764+
)
765+
}
753766

754767
override predicate sanitizes(boolean outcome, Expr e) {
755768
outcome = astNode.getPolarity() and x = e

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,15 @@
2525
| tst.js:137:16:137:36 | o.hasOw ... ty(v.p) | ExampleConfiguration | true | tst.js:137:33:137:35 | v.p |
2626
| tst.js:139:16:139:41 | o.hasOw ... "p.q"]) | ExampleConfiguration | true | tst.js:139:33:139:40 | v["p.q"] |
2727
| tst.js:148:9:148:27 | v == "white-listed" | ExampleConfiguration | true | tst.js:148:9:148:9 | v |
28+
| tst.js:148:9:148:27 | v == "white-listed" | ExampleConfiguration | true | tst.js:148:14:148:27 | "white-listed" |
29+
| tst.js:154:9:154:27 | "white-listed" != v | ExampleConfiguration | false | tst.js:154:9:154:22 | "white-listed" |
2830
| tst.js:154:9:154:27 | "white-listed" != v | ExampleConfiguration | false | tst.js:154:27:154:27 | v |
2931
| tst.js:160:9:160:30 | v === " ... sted-1" | ExampleConfiguration | true | tst.js:160:9:160:9 | v |
32+
| tst.js:160:9:160:30 | v === " ... sted-1" | ExampleConfiguration | true | tst.js:160:15:160:30 | "white-listed-1" |
3033
| tst.js:160:35:160:56 | v === " ... sted-2" | ExampleConfiguration | true | tst.js:160:35:160:35 | v |
34+
| tst.js:160:35:160:56 | v === " ... sted-2" | ExampleConfiguration | true | tst.js:160:41:160:56 | "white-listed-2" |
3135
| tst.js:166:9:166:16 | v == !!0 | ExampleConfiguration | true | tst.js:166:9:166:9 | v |
36+
| tst.js:166:9:166:16 | v == !!0 | ExampleConfiguration | true | tst.js:166:14:166:16 | !!0 |
3237
| tst.js:184:9:184:21 | ~o.indexOf(v) | ExampleConfiguration | true | tst.js:184:20:184:20 | v |
3338
| tst.js:190:10:190:22 | ~o.indexOf(v) | ExampleConfiguration | true | tst.js:190:21:190:21 | v |
3439
| tst.js:202:9:202:26 | o.indexOf(v) <= -1 | ExampleConfiguration | false | tst.js:202:19:202:19 | v |
@@ -44,21 +49,27 @@
4449
| tst.js:264:9:264:12 | g(v) | ExampleConfiguration | true | tst.js:264:11:264:11 | v |
4550
| tst.js:271:25:271:45 | whiteli ... ains(z) | ExampleConfiguration | true | tst.js:271:44:271:44 | z |
4651
| tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:16:281:17 | x2 |
52+
| tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:22:281:25 | null |
4753
| tst.js:281:30:281:51 | whiteli ... ins(x2) | ExampleConfiguration | true | tst.js:281:49:281:50 | x2 |
4854
| tst.js:283:9:283:13 | f2(v) | ExampleConfiguration | true | tst.js:283:12:283:12 | v |
4955
| tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:16:290:17 | x3 |
56+
| tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:22:290:25 | null |
5057
| tst.js:290:30:290:51 | whiteli ... ins(x3) | ExampleConfiguration | true | tst.js:290:49:290:50 | x3 |
5158
| tst.js:299:17:299:38 | whiteli ... ins(x4) | ExampleConfiguration | true | tst.js:299:36:299:37 | x4 |
5259
| tst.js:308:18:308:39 | whiteli ... ins(x5) | ExampleConfiguration | true | tst.js:308:37:308:38 | x5 |
5360
| tst.js:317:26:317:47 | whiteli ... ins(x6) | ExampleConfiguration | true | tst.js:317:45:317:46 | x6 |
5461
| tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:25:327:26 | x7 |
62+
| tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:31:327:34 | null |
5563
| tst.js:327:39:327:60 | whiteli ... ins(x7) | ExampleConfiguration | true | tst.js:327:58:327:59 | x7 |
5664
| tst.js:330:9:330:13 | f7(v) | ExampleConfiguration | true | tst.js:330:12:330:12 | v |
5765
| tst.js:337:25:337:46 | whiteli ... ins(x8) | ExampleConfiguration | true | tst.js:337:44:337:45 | x8 |
5866
| tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:16:338:17 | x8 |
67+
| tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:22:338:25 | null |
5968
| tst.js:347:29:347:50 | whiteli ... ins(x9) | ExampleConfiguration | true | tst.js:347:48:347:49 | x9 |
6069
| tst.js:356:16:356:27 | x10 !== null | ExampleConfiguration | false | tst.js:356:16:356:18 | x10 |
70+
| tst.js:356:16:356:27 | x10 !== null | ExampleConfiguration | false | tst.js:356:24:356:27 | null |
6171
| tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:32:356:34 | x10 |
72+
| tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:40:356:48 | undefined |
6273
| tst.js:358:9:358:14 | f10(v) | ExampleConfiguration | false | tst.js:358:13:358:13 | v |
6374
| tst.js:370:9:370:29 | o.p == ... listed" | ExampleConfiguration | true | tst.js:370:9:370:11 | o.p |
6475
| tst.js:377:11:377:32 | o[p] == ... listed" | ExampleConfiguration | true | tst.js:377:11:377:14 | o[p] |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ nodes
3939
| tst2.js:6:12:6:15 | q: r |
4040
| tst2.js:7:12:7:12 | p |
4141
| tst2.js:8:12:8:12 | r |
42+
| tst2.js:14:7:14:24 | p |
43+
| tst2.js:14:9:14:9 | p |
44+
| tst2.js:18:12:18:12 | p |
45+
| tst2.js:21:14:21:14 | p |
4246
edges
4347
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
4448
| etherpad.js:9:5:9:53 | response | etherpad.js:11:3:11:3 | response |
@@ -77,6 +81,9 @@ edges
7781
| tst2.js:6:7:6:30 | r | tst2.js:8:12:8:12 | r |
7882
| tst2.js:6:9:6:9 | p | tst2.js:6:7:6:30 | p |
7983
| tst2.js:6:12:6:15 | q: r | tst2.js:6:7:6:30 | r |
84+
| tst2.js:14:7:14:24 | p | tst2.js:18:12:18:12 | p |
85+
| tst2.js:14:7:14:24 | p | tst2.js:21:14:21:14 | p |
86+
| tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p |
8087
#select
8188
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
8289
| 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 |
@@ -90,3 +97,5 @@ edges
9097
| promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
9198
| tst2.js:7:12:7:12 | p | tst2.js:6:9:6:9 | p | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
9299
| tst2.js:8:12:8:12 | r | tst2.js:6:12:6:15 | q: r | tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |
100+
| tst2.js:18:12:18:12 | p | tst2.js:14:9:14:9 | p | tst2.js:18:12:18:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
101+
| tst2.js:21:14:21:14 | p | tst2.js:14:9:14:9 | p | tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,5 @@
88
| promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
99
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
1010
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |
11+
| tst2.js:18:12:18:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
12+
| tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,21 @@ var app = express();
44

55
app.get('/user/:id', function(req, res) {
66
let { p, q: r } = req.params;
7-
res.send(p);
8-
res.send(r);
7+
res.send(p); // NOT OK
8+
res.send(r); // NOT OK
9+
});
10+
11+
const aKnownValue = "foo";
12+
13+
app.get('/bar', function(req, res) {
14+
let { p } = req.params;
15+
16+
if (p == aKnownValue)
17+
res.send(p); // OK
18+
res.send(p); // NOT OK
19+
20+
if (p != aKnownValue)
21+
res.send(p); // NOT OK
22+
else
23+
res.send(p); // OK
924
});

0 commit comments

Comments
 (0)