Skip to content

Commit 951e309

Browse files
codeql-cicodeql-ci
authored
Merge pull request #4231 from erik-krogh/CVE767
Approved by asgerf
2 parents 2de94ab + 88bbc2f commit 951e309

File tree

14 files changed

+199
-37
lines changed

14 files changed

+199
-37
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,19 @@
33
## General improvements
44

55
* Support for the following frameworks and libraries has been improved:
6+
- [bluebird](https://www.npmjs.com/package/bluebird)
67
- [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
78
- [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
89
- [javascript-stringify](https://www.npmjs.com/package/javascript-stringify)
910
- [js-stringify](https://www.npmjs.com/package/js-stringify)
1011
- [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)
1112
- [json-stringify-safe](https://www.npmjs.com/package/json-stringify-safe)
1213
- [json3](https://www.npmjs.com/package/json3)
14+
- [lodash](https://www.npmjs.com/package/lodash)
1315
- [object-inspect](https://www.npmjs.com/package/object-inspect)
1416
- [pretty-format](https://www.npmjs.com/package/pretty-format)
1517
- [stringify-object](https://www.npmjs.com/package/stringify-object)
18+
- [underscore](https://www.npmjs.com/package/underscore)
1619

1720
* Analyzing files with the ".cjs" extension is now supported.
1821

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

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,11 @@ module ArrayTaintTracking {
2424
predicate arrayFunctionTaintStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode call) {
2525
// `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are
2626
// `elt` and `ary`; similar for `forEach`
27-
exists(string name, Function f, int i |
28-
(name = "map" or name = "forEach") and
29-
(i = 0 or i = 2) and
27+
exists(Function f |
3028
call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and
31-
call.(DataFlow::MethodCallNode).getMethodName() = name and
29+
call.(DataFlow::MethodCallNode).getMethodName() = ["map", "forEach"] and
3230
pred = call.getReceiver() and
33-
succ = DataFlow::parameterNode(f.getParameter(i))
31+
succ = DataFlow::parameterNode(f.getParameter([0, 2]))
3432
)
3533
or
3634
// `array.map` with tainted return value in callback
@@ -47,41 +45,22 @@ module ArrayTaintTracking {
4745
succ = call
4846
or
4947
// `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
50-
exists(string name |
51-
name = "push" or
52-
name = "unshift"
53-
|
54-
pred = call.getAnArgument() and
55-
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
56-
)
48+
pred = call.getAnArgument() and
49+
succ.(DataFlow::SourceNode).getAMethodCall(["push", "unshift"]) = call
5750
or
5851
// `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`.
59-
exists(string name |
60-
name = "push" or
61-
name = "unshift"
62-
|
63-
pred = call.getASpreadArgument() and
64-
// Make sure we handle reflective calls
65-
succ = call.getReceiver().getALocalSource() and
66-
call.getCalleeName() = name
67-
)
52+
pred = call.getASpreadArgument() and
53+
// Make sure we handle reflective calls
54+
succ = call.getReceiver().getALocalSource() and
55+
call.getCalleeName() = ["push", "unshift"]
6856
or
6957
// `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
70-
exists(string name | name = "splice" |
71-
pred = call.getArgument(2) and
72-
succ.(DataFlow::SourceNode).getAMethodCall(name) = call
73-
)
58+
pred = call.getArgument(2) and
59+
succ.(DataFlow::SourceNode).getAMethodCall("splice") = call
7460
or
7561
// `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
76-
exists(string name |
77-
name = "pop" or
78-
name = "shift" or
79-
name = "slice" or
80-
name = "splice"
81-
|
82-
call.(DataFlow::MethodCallNode).calls(pred, name) and
83-
succ = call
84-
)
62+
call.(DataFlow::MethodCallNode).calls(pred, ["pop", "shift", "slice", "splice"]) and
63+
succ = call
8564
or
8665
// `e = Array.from(x)`: if `x` is tainted, then so is `e`.
8766
call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,18 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
440440
pred.getEnclosingExpr() = await.getOperand() and
441441
succ.getEnclosingExpr() = await
442442
)
443+
or
444+
exists(DataFlow::CallNode mapSeries |
445+
mapSeries = DataFlow::moduleMember("bluebird", "mapSeries").getACall()
446+
|
447+
// from `xs` to `x` in `require("bluebird").mapSeries(xs, (x) => {...})`.
448+
pred = mapSeries.getArgument(0) and
449+
succ = mapSeries.getABoundCallbackParameter(1, 0)
450+
or
451+
// from `y` to `require("bluebird").mapSeries(x, x => y)`.
452+
pred = mapSeries.getCallback(1).getAReturn() and
453+
succ = mapSeries
454+
)
443455
}
444456

445457
/**

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,53 @@ module ClientRequest {
389389
}
390390
}
391391

392+
/**
393+
* Gets an instantiation `socket` of `require("net").Socket` type tracked using `t`.
394+
*/
395+
private DataFlow::SourceNode netSocketInstantiation(
396+
DataFlow::TypeTracker t, DataFlow::NewNode socket
397+
) {
398+
t.start() and
399+
socket = DataFlow::moduleMember("net", "Socket").getAnInstantiation() and
400+
result = socket
401+
or
402+
exists(DataFlow::TypeTracker t2 | result = netSocketInstantiation(t2, socket).track(t2, t))
403+
}
404+
405+
/**
406+
* A model of a request made using `(new require("net").Socket()).connect(args);`.
407+
*/
408+
class NetSocketRequest extends ClientRequest::Range {
409+
DataFlow::NewNode socket;
410+
411+
NetSocketRequest() {
412+
this = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMethodCall("connect")
413+
}
414+
415+
override DataFlow::Node getUrl() {
416+
result = getArgument([0, 1]) // there are multiple overrides of `connect`, and the URL can be in the first or second argument.
417+
}
418+
419+
override DataFlow::Node getHost() { result = getOptionArgument(0, "host") }
420+
421+
override DataFlow::Node getAResponseDataNode(string responseType, boolean promise) {
422+
responseType = "text" and
423+
promise = false and
424+
exists(DataFlow::CallNode call |
425+
call = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMemberCall("on") and
426+
call.getArgument(0).mayHaveStringValue("data") and
427+
result = call.getABoundCallbackParameter(1, 0)
428+
)
429+
}
430+
431+
override DataFlow::Node getADataNode() {
432+
exists(DataFlow::CallNode call |
433+
call = netSocketInstantiation(DataFlow::TypeTracker::end(), socket).getAMemberCall("write") and
434+
result = call.getArgument(0)
435+
)
436+
}
437+
}
438+
392439
/**
393440
* A model of a URL request made using the `superagent` library.
394441
*/

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,52 @@ module LodashUnderscore {
394394
succ = this.getExceptionalReturn()
395395
}
396396
}
397+
398+
/**
399+
* Holds if there is a taint-step involving a (non-function) underscore method from `pred` to `succ`.
400+
*/
401+
private predicate underscoreTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
402+
exists(string name, DataFlow::CallNode call |
403+
call = any(Member member | member.getName() = name).getACall()
404+
|
405+
name =
406+
["find", "filter", "findWhere", "where", "reject", "pluck", "max", "min", "sortBy",
407+
"shuffle", "sample", "toArray", "partition", "compact", "first", "initial", "last",
408+
"rest", "flatten", "without", "difference", "uniq", "unique", "unzip", "transpose",
409+
"object", "chunk", "values", "mapObject", "pick", "omit", "defaults", "clone", "tap",
410+
"identity"] and
411+
pred = call.getArgument(0) and
412+
succ = call
413+
or
414+
name = ["union", "zip"] and
415+
pred = call.getAnArgument() and
416+
succ = call
417+
or
418+
name =
419+
["each", "map", "every", "some", "max", "min", "sortBy", "partition", "mapObject", "tap"] and
420+
pred = call.getArgument(0) and
421+
succ = call.getABoundCallbackParameter(1, 0)
422+
or
423+
name = ["reduce", "reduceRight"] and
424+
pred = call.getArgument(0) and
425+
succ = call.getABoundCallbackParameter(1, 1)
426+
or
427+
name = ["map", "reduce", "reduceRight"] and
428+
pred = call.getCallback(1).getAReturn() and
429+
succ = call
430+
)
431+
}
432+
433+
/**
434+
* A model for taint-steps involving (non-function) underscore methods.
435+
*/
436+
private class UnderscoreTaintStep extends TaintTracking::AdditionalTaintStep {
437+
UnderscoreTaintStep() { underscoreTaintStep(this, _) }
438+
439+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
440+
underscoreTaintStep(pred, succ) and pred = this
441+
}
442+
}
397443
}
398444

399445
/**

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,8 +1068,10 @@ module NodeJSLib {
10681068
/**
10691069
* An instance of net.createServer(), which creates a new TCP/IPC server.
10701070
*/
1071-
private class NodeJSNetServer extends DataFlow::SourceNode {
1072-
NodeJSNetServer() { this = DataFlow::moduleMember("net", "createServer").getAnInvocation() }
1071+
class NodeJSNetServer extends DataFlow::InvokeNode {
1072+
NodeJSNetServer() {
1073+
this = DataFlow::moduleMember(["net", "tls"], "createServer").getAnInvocation()
1074+
}
10731075

10741076
private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
10751077
t.start() and result = this
@@ -1096,6 +1098,8 @@ module NodeJSLib {
10961098
|
10971099
this = call.getCallback(1).getParameter(0)
10981100
)
1101+
or
1102+
this = server.getCallback([0, 1]).getParameter(0)
10991103
}
11001104

11011105
DataFlow::SourceNode ref() { result = EventEmitter::trackEventEmitter(this) }

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,3 +86,9 @@
8686
| tst.js:2:17:2:22 | "src1" | tst.js:61:16:61:18 | o.r |
8787
| tst.js:2:17:2:22 | "src1" | tst.js:68:16:68:22 | inner() |
8888
| tst.js:2:17:2:22 | "src1" | tst.js:80:16:80:22 | outer() |
89+
| underscore.js:2:17:2:22 | "src1" | underscore.js:3:15:3:28 | _.max(source1) |
90+
| underscore.js:5:17:5:22 | "src2" | underscore.js:6:15:6:34 | _.union([], source2) |
91+
| underscore.js:5:17:5:22 | "src2" | underscore.js:7:15:7:32 | _.zip(source2, []) |
92+
| underscore.js:9:17:9:22 | "src3" | underscore.js:11:17:11:17 | x |
93+
| underscore.js:14:17:14:22 | "src4" | underscore.js:16:17:16:17 | e |
94+
| underscore.js:19:17:19:22 | "src5" | underscore.js:20:15:20:44 | _.map([ ... ource5) |

‎javascript/ql/test/library-tests/InterProceduralFlow/underscore.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
(function() {
2+
var source1 = "src1";
3+
var sink1 = _.max(source1); // NOT OK
4+
5+
var source2 = "src2";
6+
var sink2 = _.union([], source2); // NOT OK
7+
var sink3 = _.zip(source2, []); // NOT OK
8+
9+
var source3 = "src3";
10+
_.map(source3, (x) => {
11+
let sink4 = x; // NOT OK
12+
});
13+
14+
var source4 = "src4";
15+
_.reduce(source4, (acc, e) => {
16+
let sink5 = e; // NOT OK
17+
});
18+
19+
var source5 = "src5";
20+
var sink6 = _.map([1,2,3], (x) => source5); // NOT OK
21+
})();

‎javascript/ql/test/library-tests/Promises/flow.js‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,4 +154,15 @@
154154
} catch (e) {
155155
sink(e); // NOT OK
156156
}
157-
})();
157+
})();
158+
159+
(function () {
160+
var source = "source";
161+
162+
var bluebird = require("bluebird");
163+
164+
bluebird.mapSeries(source, x => sink(x)); // NOT OK (for taint-tracking configs)
165+
166+
const foo = bluebird.mapSeries(source, x => x);
167+
sink(foo); // NOT OK (for taint-tracking configs)
168+
})

‎javascript/ql/test/library-tests/Promises/tests.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,8 @@ flow
237237
exclusiveTaintFlow
238238
| flow2.js:2:15:2:22 | "source" | flow2.js:20:7:20:14 | tainted3 |
239239
| flow.js:136:15:136:22 | "source" | flow.js:141:7:141:13 | async() |
240+
| flow.js:160:15:160:22 | "source" | flow.js:164:39:164:39 | x |
241+
| flow.js:160:15:160:22 | "source" | flow.js:167:7:167:9 | foo |
240242
| interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |
241243
typetrack
242244
| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | copy $PromiseResolveField$ |

0 commit comments

Comments
 (0)