Skip to content

Commit b34f444

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#6254 from erik-krogh/json2csv
Approved by asgerf
2 parents cb1b227 + bef7e61 commit b34f444

File tree

11 files changed

+205
-4
lines changed

11 files changed

+205
-4
lines changed

‎javascript/change-notes/2021-06-24-json.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
lgtm,codescanning
2+
* The dataflow libraries now model dataflow through more JSON utility libraries.
3+
Affected packages are
4+
[json2csv](https://npmjs.com/package/json2csv),
5+
[json5](https://npmjs.com/package/json5),
6+
[prettyjson](https://npmjs.com/package/prettyjson),
7+
[flatted](https://npmjs.com/package/flatted),
8+
[teleport-javascript](https://npmjs.com/package/teleport-javascript),
9+
[replicator](https://npmjs.com/package/replicator),
10+
[safe-stable-stringify](https://npmjs.com/package/safe-stable-stringify),
11+
[fclone](https://npmjs.com/package/fclone),
12+
[json-cycle](https://npmjs.com/package/json-cycle),
13+
[strip-json-comments](https://npmjs.com/package/strip-json-comments),
14+
[fast-json-stringify](https://npmjs.com/package/fast-json-stringify)

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,11 +178,15 @@ private class ExtendCallTaintStep extends TaintTracking::SharedTaintStep {
178178
private import semmle.javascript.dataflow.internal.PreCallGraphStep
179179

180180
/**
181-
* A step for the `clone` package.
181+
* A step through a cloning library, such as `clone` or `fclone`.
182182
*/
183183
private class CloneStep extends PreCallGraphStep {
184184
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
185-
exists(DataFlow::CallNode call | call = DataFlow::moduleImport("clone").getACall() |
185+
exists(DataFlow::CallNode call |
186+
call = DataFlow::moduleImport(["clone", "fclone"]).getACall()
187+
or
188+
call = DataFlow::moduleMember("json-cycle", ["decycle", "retrocycle"]).getACall()
189+
|
186190
pred = call.getArgument(0) and
187191
succ = call
188192
)

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ private class PlainJsonParserCall extends JsonParserCall {
2626
PlainJsonParserCall() {
2727
exists(DataFlow::SourceNode callee | this = callee.getACall() |
2828
callee = DataFlow::globalVarRef("JSON").getAPropertyRead("parse") or
29+
callee =
30+
DataFlow::moduleMember(["json3", "json5", "flatted", "teleport-javascript", "json-cycle"],
31+
"parse") or
32+
callee = API::moduleImport("replicator").getInstance().getMember("decode").getAnImmediateUse() or
2933
callee = DataFlow::moduleImport("parse-json") or
3034
callee = DataFlow::moduleImport("json-parse-better-errors") or
3135
callee = DataFlow::moduleImport("json-safe-parse") or
@@ -74,3 +78,15 @@ private class JsonParserCallWithCallback extends JsonParserCall {
7478

7579
override DataFlow::SourceNode getOutput() { result = getCallback(1).getParameter(1) }
7680
}
81+
82+
/**
83+
* A taint step through the `strip-json-comments` library.
84+
*/
85+
private class StripJsonCommentsStep extends TaintTracking::SharedTaintStep {
86+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
87+
exists(API::CallNode call | call = API::moduleImport("strip-json-comments").getACall() |
88+
pred = call.getArgument(0) and
89+
succ = call
90+
)
91+
}
92+
}

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

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,15 @@ class JsonStringifyCall extends DataFlow::CallNode {
1111
JsonStringifyCall() {
1212
exists(DataFlow::SourceNode callee | this = callee.getACall() |
1313
callee = DataFlow::globalVarRef("JSON").getAPropertyRead("stringify") or
14-
callee = DataFlow::moduleMember("json3", "stringify") or
14+
callee =
15+
DataFlow::moduleMember(["json3", "json5", "flatted", "teleport-javascript", "json-cycle"],
16+
"stringify") or
17+
callee = API::moduleImport("replicator").getInstance().getMember("encode").getAnImmediateUse() or
1518
callee =
1619
DataFlow::moduleImport([
1720
"json-stringify-safe", "json-stable-stringify", "stringify-object",
1821
"fast-json-stable-stringify", "fast-safe-stringify", "javascript-stringify",
19-
"js-stringify"
22+
"js-stringify", "safe-stable-stringify", "fast-json-stringify"
2023
]) or
2124
// require("util").inspect() and similar
2225
callee = DataFlow::moduleMember("util", "inspect") or
@@ -34,3 +37,38 @@ class JsonStringifyCall extends DataFlow::CallNode {
3437
*/
3538
DataFlow::SourceNode getOutput() { result = this }
3639
}
40+
41+
/**
42+
* A taint step through the [`json2csv`](https://www.npmjs.com/package/json2csv) library.
43+
*/
44+
class JSON2CSVTaintStep extends TaintTracking::SharedTaintStep {
45+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
46+
exists(API::CallNode call |
47+
call =
48+
API::moduleImport("json2csv")
49+
.getMember("Parser")
50+
.getInstance()
51+
.getMember("parse")
52+
.getACall()
53+
|
54+
pred = call.getArgument(0) and
55+
succ = call
56+
)
57+
}
58+
}
59+
60+
/**
61+
* A step through the [`prettyjson`](https://www.npmjs.com/package/prettyjson) library.
62+
* This is not quite a `JSON.stringify` call, as it e.g. does not wrap keys in double quotes.
63+
* It's therefore modelled as a taint-step rather than as a `JSON.stringify` call.
64+
*/
65+
class PrettyJSONTaintStep extends TaintTracking::SharedTaintStep {
66+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
67+
exists(API::CallNode call |
68+
call = API::moduleImport("prettyjson").getMember("render").getACall()
69+
|
70+
pred = call.getArgument(0) and
71+
succ = call
72+
)
73+
}
74+
}

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,15 @@ typeInferenceMismatch
9090
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:16:8:16:38 | require ... source) |
9191
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:17:8:17:39 | require ... source) |
9292
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:18:8:18:40 | require ... source) |
93+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:21:8:21:46 | new jso ... source) |
94+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:24:8:24:43 | json5.s ... ource)) |
95+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:27:8:27:47 | flatted ... ource)) |
96+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:30:8:30:49 | telepor ... ource)) |
97+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:34:8:34:51 | replica ... ource)) |
98+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:36:8:36:47 | require ... source) |
99+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:39:8:39:37 | jc.stri ... ource)) |
100+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:42:8:42:51 | JSON.st ... urce))) |
101+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:45:8:45:23 | fastJson(source) |
93102
| json-stringify.js:3:15:3:22 | source() | json-stringify.js:8:8:8:31 | jsonStr ... (taint) |
94103
| nested-props.js:4:13:4:20 | source() | nested-props.js:5:10:5:14 | obj.x |
95104
| nested-props.js:9:18:9:25 | source() | nested-props.js:10:10:10:16 | obj.x.y |

‎javascript/ql/test/library-tests/TaintTracking/json-stringify.js

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,31 @@ function foo() {
1616
sink(require("util").inspect(source)); // NOT OK
1717
sink(require("pretty-format")(source)); // NOT OK
1818
sink(require("object-inspect")(source)); // NOT OK
19+
20+
const json2csv = require('json2csv');
21+
sink(new json2csv.Parser(opts).parse(source)); // NOT OK
22+
23+
const json5 = require('json5');
24+
sink(json5.stringify(json5.parse(source))); // NOT OK
25+
26+
const flatted = require('flatted');
27+
sink(flatted.stringify(flatted.parse(source))); // NOT OK
28+
29+
const teleport = require('teleport-javascript');
30+
sink(teleport.stringify(teleport.parse(source))); // NOT OK
31+
32+
const Replicator = require('replicator');
33+
const replicator = new Replicator();
34+
sink(replicator.encode(replicator.decode(source))); // NOT OK
35+
36+
sink(require("safe-stable-stringify")(source)); // NOT OK
37+
38+
const jc = require('json-cycle');
39+
sink(jc.stringify(jc.parse(source))); // NOT OK
40+
41+
const stripper = require("strip-json-comments");
42+
sink(JSON.stringify(JSON.parse(stripper(source)))); // NOT OK
43+
44+
const fastJson = require('fast-json-stringify');
45+
sink(fastJson(source)); // NOT OK
1946
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,22 @@ nodes
190190
| tst2.js:49:36:49:36 | p |
191191
| tst2.js:51:12:51:17 | unsafe |
192192
| tst2.js:51:12:51:17 | unsafe |
193+
| tst2.js:57:7:57:24 | p |
194+
| tst2.js:57:9:57:9 | p |
195+
| tst2.js:57:9:57:9 | p |
196+
| tst2.js:60:11:60:11 | p |
197+
| tst2.js:63:12:63:12 | p |
198+
| tst2.js:63:12:63:12 | p |
199+
| tst2.js:64:12:64:18 | other.p |
200+
| tst2.js:64:12:64:18 | other.p |
201+
| tst2.js:69:7:69:24 | p |
202+
| tst2.js:69:9:69:9 | p |
203+
| tst2.js:69:9:69:9 | p |
204+
| tst2.js:72:11:72:11 | p |
205+
| tst2.js:75:12:75:12 | p |
206+
| tst2.js:75:12:75:12 | p |
207+
| tst2.js:76:12:76:18 | other.p |
208+
| tst2.js:76:12:76:18 | other.p |
193209
| tst3.js:5:7:5:24 | p |
194210
| tst3.js:5:9:5:9 | p |
195211
| tst3.js:5:9:5:9 | p |
@@ -359,6 +375,20 @@ edges
359375
| tst2.js:49:7:49:53 | unsafe | tst2.js:51:12:51:17 | unsafe |
360376
| tst2.js:49:16:49:53 | seriali ... true}) | tst2.js:49:7:49:53 | unsafe |
361377
| tst2.js:49:36:49:36 | p | tst2.js:49:16:49:53 | seriali ... true}) |
378+
| tst2.js:57:7:57:24 | p | tst2.js:60:11:60:11 | p |
379+
| tst2.js:57:7:57:24 | p | tst2.js:63:12:63:12 | p |
380+
| tst2.js:57:7:57:24 | p | tst2.js:63:12:63:12 | p |
381+
| tst2.js:57:9:57:9 | p | tst2.js:57:7:57:24 | p |
382+
| tst2.js:57:9:57:9 | p | tst2.js:57:7:57:24 | p |
383+
| tst2.js:60:11:60:11 | p | tst2.js:64:12:64:18 | other.p |
384+
| tst2.js:60:11:60:11 | p | tst2.js:64:12:64:18 | other.p |
385+
| tst2.js:69:7:69:24 | p | tst2.js:72:11:72:11 | p |
386+
| tst2.js:69:7:69:24 | p | tst2.js:75:12:75:12 | p |
387+
| tst2.js:69:7:69:24 | p | tst2.js:75:12:75:12 | p |
388+
| tst2.js:69:9:69:9 | p | tst2.js:69:7:69:24 | p |
389+
| tst2.js:69:9:69:9 | p | tst2.js:69:7:69:24 | p |
390+
| tst2.js:72:11:72:11 | p | tst2.js:76:12:76:18 | other.p |
391+
| tst2.js:72:11:72:11 | p | tst2.js:76:12:76:18 | other.p |
362392
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p |
363393
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p |
364394
| tst3.js:5:9:5:9 | p | tst3.js:5:7:5:24 | p |
@@ -412,5 +442,9 @@ edges
412442
| tst2.js:36:12:36:12 | p | tst2.js:30:9:30:9 | p | tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
413443
| tst2.js:37:12:37:18 | other.p | tst2.js:30:9:30:9 | p | tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
414444
| tst2.js:51:12:51:17 | unsafe | tst2.js:43:9:43:9 | p | tst2.js:51:12:51:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:43:9:43:9 | p | user-provided value |
445+
| tst2.js:63:12:63:12 | p | tst2.js:57:9:57:9 | p | tst2.js:63:12:63:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:57:9:57:9 | p | user-provided value |
446+
| tst2.js:64:12:64:18 | other.p | tst2.js:57:9:57:9 | p | tst2.js:64:12:64:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:57:9:57:9 | p | user-provided value |
447+
| tst2.js:75:12:75:12 | p | tst2.js:69:9:69:9 | p | tst2.js:75:12:75:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
448+
| tst2.js:76:12:76:18 | other.p | tst2.js:69:9:69:9 | p | tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
415449
| tst3.js:6:12:6:12 | p | tst3.js:5:9:5:9 | p | tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |
416450
| tst3.js:12:12:12:15 | code | tst3.js:11:32:11:39 | reg.body | tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,9 @@
4040
| tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
4141
| tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
4242
| tst2.js:51:12:51:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:43:9:43:9 | p | user-provided value |
43+
| tst2.js:63:12:63:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:57:9:57:9 | p | user-provided value |
44+
| tst2.js:64:12:64:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:57:9:57:9 | p | user-provided value |
45+
| tst2.js:75:12:75:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
46+
| tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
4347
| tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |
4448
| tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,29 @@ app.get('/baz', function(req, res) {
4949
var unsafe = serializeJavaScript(p, {unsafe: true});
5050

5151
res.send(unsafe); // NOT OK
52+
});
53+
54+
const fclone = require('fclone');
55+
56+
app.get('/baz', function(req, res) {
57+
let { p } = req.params;
58+
59+
var obj = {};
60+
obj.p = p;
61+
var other = fclone(obj);
62+
63+
res.send(p); // NOT OK
64+
res.send(other.p); // NOT OK
65+
});
66+
67+
const jc = require('json-cycle');
68+
app.get('/baz', function(req, res) {
69+
let { p } = req.params;
70+
71+
var obj = {};
72+
obj.p = p;
73+
var other = jc.retrocycle(jc.decycle(obj));
74+
75+
res.send(p); // NOT OK
76+
res.send(other.p); // NOT OK
5277
});

‎javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,17 @@ nodes
6565
| logInjectionBad.js:58:17:58:59 | stripAn ... rname)) |
6666
| logInjectionBad.js:58:27:58:58 | chalk.u ... ername) |
6767
| logInjectionBad.js:58:50:58:57 | username |
68+
| logInjectionBad.js:63:9:63:36 | q |
69+
| logInjectionBad.js:63:13:63:36 | url.par ... , true) |
70+
| logInjectionBad.js:63:23:63:29 | req.url |
71+
| logInjectionBad.js:63:23:63:29 | req.url |
72+
| logInjectionBad.js:64:9:64:35 | username |
73+
| logInjectionBad.js:64:20:64:20 | q |
74+
| logInjectionBad.js:64:20:64:26 | q.query |
75+
| logInjectionBad.js:64:20:64:35 | q.query.username |
76+
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
77+
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
78+
| logInjectionBad.js:66:35:66:42 | username |
6879
edges
6980
| logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
7081
| logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
@@ -130,6 +141,16 @@ edges
130141
| logInjectionBad.js:58:27:58:58 | chalk.u ... ername) | logInjectionBad.js:58:17:58:59 | stripAn ... rname)) |
131142
| logInjectionBad.js:58:27:58:58 | chalk.u ... ername) | logInjectionBad.js:58:17:58:59 | stripAn ... rname)) |
132143
| logInjectionBad.js:58:50:58:57 | username | logInjectionBad.js:58:27:58:58 | chalk.u ... ername) |
144+
| logInjectionBad.js:63:9:63:36 | q | logInjectionBad.js:64:20:64:20 | q |
145+
| logInjectionBad.js:63:13:63:36 | url.par ... , true) | logInjectionBad.js:63:9:63:36 | q |
146+
| logInjectionBad.js:63:23:63:29 | req.url | logInjectionBad.js:63:13:63:36 | url.par ... , true) |
147+
| logInjectionBad.js:63:23:63:29 | req.url | logInjectionBad.js:63:13:63:36 | url.par ... , true) |
148+
| logInjectionBad.js:64:9:64:35 | username | logInjectionBad.js:66:35:66:42 | username |
149+
| logInjectionBad.js:64:20:64:20 | q | logInjectionBad.js:64:20:64:26 | q.query |
150+
| logInjectionBad.js:64:20:64:26 | q.query | logInjectionBad.js:64:20:64:35 | q.query.username |
151+
| logInjectionBad.js:64:20:64:35 | q.query.username | logInjectionBad.js:64:9:64:35 | username |
152+
| logInjectionBad.js:66:35:66:42 | username | logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
153+
| logInjectionBad.js:66:35:66:42 | username | logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
133154
#select
134155
| logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
135156
| logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
@@ -146,3 +167,4 @@ edges
146167
| logInjectionBad.js:56:17:56:55 | kleur.b ... ername) | logInjectionBad.js:46:23:46:29 | req.url | logInjectionBad.js:56:17:56:55 | kleur.b ... ername) | $@ flows to log entry. | logInjectionBad.js:46:23:46:29 | req.url | User-provided value |
147168
| logInjectionBad.js:57:17:57:48 | chalk.u ... ername) | logInjectionBad.js:46:23:46:29 | req.url | logInjectionBad.js:57:17:57:48 | chalk.u ... ername) | $@ flows to log entry. | logInjectionBad.js:46:23:46:29 | req.url | User-provided value |
148169
| logInjectionBad.js:58:17:58:59 | stripAn ... rname)) | logInjectionBad.js:46:23:46:29 | req.url | logInjectionBad.js:58:17:58:59 | stripAn ... rname)) | $@ flows to log entry. | logInjectionBad.js:46:23:46:29 | req.url | User-provided value |
170+
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) | logInjectionBad.js:63:23:63:29 | req.url | logInjectionBad.js:66:17:66:43 | prettyj ... ername) | $@ flows to log entry. | logInjectionBad.js:63:23:63:29 | req.url | User-provided value |

0 commit comments

Comments
 (0)