Skip to content

Commit bcfd02f

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #85 from esben-semmle/js/format-string-taint-step
Approved by xiemaisi
2 parents 44ae7b6 + eb356d8 commit bcfd02f

File tree

10 files changed

+162
-51
lines changed

10 files changed

+162
-51
lines changed

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212

1313
* Modelling of taint flow through the array operations `map` and `join` has been improved. This may give additional results for the security queries.
1414

15+
* The taint tracking library recognizes more ways in which taint propagates. In particular, some flow through string formatters is now recognized. This may give additional results for the security queries.
16+
1517
* The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries.
1618

1719
* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
@@ -37,6 +39,7 @@
3739
- [extend2](https://github.com/eggjs/extend2)
3840
- [fast-json-parse](https://github.com/mcollina/fast-json-parse)
3941
- [forge](https://github.com/digitalbazaar/forge)
42+
- [format-util](https://github.com/tmpfs/format-util)
4043
- [global](https://www.npmjs.com/package/global)
4144
- [he](https://github.com/mathiasbynens/he)
4245
- [html-entities](https://github.com/mdevils/node-html-entities)
@@ -58,13 +61,17 @@
5861
- [object.assign](https://github.com/ljharb/object.assign)
5962
- [object.defaults](https://github.com/jonschlinkert/object.defaults)
6063
- [parse-json](https://github.com/sindresorhus/parse-json)
61-
- [React Native](https://facebook.github.io/react-native/)
64+
- [printf](https://github.com/adaltas/node-printf)
65+
- [printj](https://github.com/SheetJS/printj)
6266
- [q](http://documentup.com/kriskowal/q/)
6367
- [ramda](https://ramdajs.com)
68+
- [React Native](https://facebook.github.io/react-native/)
6469
- [safe-json-parse](https://github.com/Raynos/safe-json-parse)
6570
- [sanitize](https://github.com/pocketly/node-sanitize)
6671
- [sanitizer](https://github.com/theSmaw/Caja-HTML-Sanitizer)
6772
- [smart-extend](https://github.com/danielkalen/smart-extend)
73+
- [sprintf.js](https://github.com/alexei/sprintf.js)
74+
- [string-template](https://github.com/Matt-Esch/string-template)
6875
- [underscore](https://underscorejs.org)
6976
- [util-extend](https://github.com/isaacs/util-extend)
7077
- [utils-merge](https://github.com/jaredhanson/utils-merge)

‎javascript/ql/src/javascript.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ import semmle.javascript.frameworks.React
6666
import semmle.javascript.frameworks.ReactNative
6767
import semmle.javascript.frameworks.Request
6868
import semmle.javascript.frameworks.SQL
69+
import semmle.javascript.frameworks.StringFormatters
6970
import semmle.javascript.frameworks.UriLibraries
7071
import semmle.javascript.frameworks.XmlParsers
7172
import semmle.javascript.frameworks.xUnit

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,27 @@ module TaintTracking {
408408
}
409409
}
410410

411+
/**
412+
* A taint propagating data flow edge arising from string formatting.
413+
*/
414+
private class StringFormattingTaintStep extends AdditionalTaintStep {
415+
416+
PrintfStyleCall call;
417+
418+
StringFormattingTaintStep() {
419+
this = call and
420+
call.returnsFormatted()
421+
}
422+
423+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
424+
succ = this and (
425+
pred = call.getFormatString()
426+
or
427+
pred = call.getFormatArgument(_)
428+
)
429+
}
430+
}
431+
411432
/**
412433
* A taint propagating data flow edge arising from JSON unparsing.
413434
*/

‎javascript/ql/src/semmle/javascript/frameworks/StringFormatters.qll

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/**
2+
* Provides classes for modeling string formatting libraries.
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* A printf-style call that substitutes the embedded format specifiers of a format string for the format arguments.
9+
*/
10+
abstract class PrintfStyleCall extends DataFlow::CallNode {
11+
/**
12+
* Gets the format string.
13+
*/
14+
abstract DataFlow::Node getFormatString();
15+
16+
/**
17+
* Gets the ith argument to the format string.
18+
*/
19+
abstract DataFlow::Node getFormatArgument(int i);
20+
21+
/**
22+
* Holds if this call returns the formatted string.
23+
*/
24+
abstract predicate returnsFormatted();
25+
}
26+
27+
private class LibraryFormatter extends PrintfStyleCall {
28+
29+
int formatIndex;
30+
31+
boolean returns;
32+
33+
LibraryFormatter() {
34+
// built-in Node.js functions
35+
exists (string mod, string meth |
36+
returns = false and
37+
mod = "console" and (
38+
(
39+
meth = "debug" or
40+
meth = "error" or
41+
meth = "info" or
42+
meth = "log" or
43+
meth = "trace" or
44+
meth = "warn"
45+
) and
46+
formatIndex = 0
47+
or
48+
meth = "assert" and formatIndex = 1
49+
)
50+
or
51+
returns = true and
52+
mod = "util" and (
53+
(meth = "format" or meth = "log") and formatIndex = 0
54+
or
55+
meth = "formatWithOptions" and formatIndex = 1
56+
)
57+
|
58+
// `console` and `util` are available both as modules...
59+
this = DataFlow::moduleMember(mod, meth).getACall()
60+
or
61+
// ...and as globals
62+
this = DataFlow::globalVarRef(mod).getAMemberCall(meth)
63+
)
64+
or
65+
returns = true and (
66+
// https://www.npmjs.com/package/printf
67+
this = DataFlow::moduleImport("printf").getACall() and
68+
formatIndex in [0..1]
69+
or
70+
// https://www.npmjs.com/package/printj
71+
exists (string fn | fn = "sprintf" or fn = "vsprintf" |
72+
this = DataFlow::moduleMember("printj", fn).getACall() and
73+
formatIndex = 0
74+
)
75+
or
76+
// https://www.npmjs.com/package/format-util
77+
this = DataFlow::moduleImport("format-util").getACall() and
78+
formatIndex = 0
79+
or
80+
// https://www.npmjs.com/package/string-template
81+
this = DataFlow::moduleImport("string-template").getACall() and
82+
formatIndex = 0
83+
or
84+
this = DataFlow::moduleImport("string-template/compile").getACall() and
85+
formatIndex = 0
86+
or
87+
// https://www.npmjs.com/package/sprintf-js
88+
exists (string meth | meth = "sprintf" or meth = "vsprintf" |
89+
this = DataFlow::moduleMember("sprintf-js", meth).getACall() and
90+
formatIndex = 0
91+
)
92+
)
93+
}
94+
95+
override DataFlow::Node getFormatString() {
96+
result = getArgument(formatIndex)
97+
}
98+
99+
override DataFlow::Node getFormatArgument(int i) {
100+
i >= 0 and
101+
result = getArgument(formatIndex + 1 + i)
102+
}
103+
104+
override predicate returnsFormatted() {
105+
returns = true
106+
}
107+
108+
}

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

Lines changed: 3 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -58,57 +58,10 @@ module TaintedFormatString {
5858
*/
5959
class FormatSink extends Sink {
6060
FormatSink() {
61-
exists (DataFlow::CallNode call, int argIdx |
62-
// built-in Node.js functions
63-
exists (string mod, string meth |
64-
mod = "console" and
65-
(meth = "debug" or meth = "error" or meth = "info" or
66-
meth = "log" or meth = "trace" or meth = "warn") and
67-
argIdx = 0
68-
or
69-
mod = "console" and meth = "assert" and argIdx = 1
70-
or
71-
mod = "util" and (meth = "format" or meth = "log") and argIdx = 0
72-
or
73-
mod = "util" and meth = "formatWithOptions" and argIdx = 1
74-
|
75-
// `console` and `util` are available both as modules...
76-
call = DataFlow::moduleMember(mod, meth).getACall()
77-
or
78-
// ...and as globals
79-
call = DataFlow::globalVarRef(mod).getAMemberCall(meth)
80-
)
81-
or
82-
// https://www.npmjs.com/package/printf
83-
call = DataFlow::moduleImport("printf").getACall() and
84-
argIdx in [0..1]
85-
or
86-
// https://www.npmjs.com/package/printj
87-
exists (string fn | fn = "sprintf" or fn = "vsprintf" |
88-
call = DataFlow::moduleMember("printj", fn).getACall() and
89-
argIdx = 0
90-
)
91-
or
92-
// https://www.npmjs.com/package/format-util
93-
call = DataFlow::moduleImport("format-util").getACall() and
94-
argIdx = 0
95-
or
96-
// https://www.npmjs.com/package/string-template
97-
call = DataFlow::moduleImport("string-template").getACall() and
98-
argIdx = 0
99-
or
100-
call = DataFlow::moduleImport("string-template/compile").getACall() and
101-
argIdx = 0
102-
or
103-
// https://www.npmjs.com/package/sprintf-js
104-
exists (string meth | meth = "sprintf" or meth = "vsprintf" |
105-
call = DataFlow::moduleMember("sprintf-js", meth).getACall() and
106-
argIdx = 0
107-
)
108-
|
109-
this = call.getArgument(argIdx) and
61+
exists(PrintfStyleCall printf |
62+
this = printf.getFormatString() and
11063
// exclude trivial case where there are no arguments to interpolate
111-
exists(call.getArgument(argIdx+1))
64+
exists(printf.getFormatArgument(_))
11265
)
11366
}
11467
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
| 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 |
22
| 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 |
3+
| 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 |
4+
| 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 |
35
| 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 |
46
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
57
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ edges
66
| etherpad.js:9:16:9:47 | req.que ... esponse | etherpad.js:9:16:9:53 | req.que ... e + ")" |
77
| etherpad.js:9:16:9:53 | req.que ... e + ")" | etherpad.js:9:5:9:53 | response |
88
| etherpad.js:11:3:11:3 | response | etherpad.js:11:12:11:19 | response |
9+
| formatting.js:4:9:4:29 | evil | formatting.js:6:43:6:46 | evil |
10+
| formatting.js:4:9:4:29 | evil | formatting.js:7:49:7:52 | evil |
11+
| formatting.js:4:16:4:29 | req.query.evil | formatting.js:4:9:4:29 | evil |
12+
| formatting.js:6:43:6:46 | evil | formatting.js:6:14:6:47 | util.fo ... , evil) |
13+
| formatting.js:7:49:7:52 | evil | formatting.js:7:14:7:53 | require ... , evil) |
914
| promises.js:5:3:5:59 | new Pro ... .data)) | promises.js:6:11:6:11 | x |
1015
| promises.js:5:44:5:57 | req.query.data | promises.js:5:3:5:59 | new Pro ... .data)) |
1116
| promises.js:5:44:5:57 | req.query.data | promises.js:6:11:6:11 | x |
@@ -18,6 +23,8 @@ edges
1823
#select
1924
| 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 |
2025
| 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 |
26+
| 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 |
27+
| formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | 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 |
2128
| 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 |
2229
| 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 |
2330
| 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 |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
| 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 |
2+
| 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 |
3+
| 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 |
24
| 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 |
35
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
46
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ WARNING: Type XssDataFlowConfiguration has been deprecated and may be removed in
33
WARNING: Type XssDataFlowConfiguration has been deprecated and may be removed in future (ReflectedXssWithCustomSanitizer_old.ql:20,6-30)
44
WARNING: Predicate flowsFrom has been deprecated and may be removed in future (ReflectedXssWithCustomSanitizer_old.ql:21,11-20)
55
| 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 |
6+
| 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 |
7+
| 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 |
68
| 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 |
79
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
810
| tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
var express = require('express');
2+
3+
express().get('/user/', function(req, res) {
4+
var evil = req.query.evil;
5+
res.send(console.log("<div>%s</div>", evil)); // OK (returns undefined)
6+
res.send(util.format("<div>%s</div>", evil)); // NOT OK
7+
res.send(require("printf")("<div>%s</div>", evil)); // NOT OK
8+
});

0 commit comments

Comments
 (0)