Skip to content

Commit 1ad471c

Browse files
asgerfasgerf
committed
JS: Track through spread/rest params in API graphs
1 parent ff99d5c commit 1ad471c

File tree

6 files changed

+72
-6
lines changed

6 files changed

+72
-6
lines changed

‎javascript/ql/lib/semmle/javascript/ApiGraphs.qll

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,11 @@ module API {
318318
Node getParameter(int i) {
319319
Stages::ApiStage::ref() and
320320
result = this.getASuccessor(Label::parameter(i))
321+
or
322+
exists(int spreadIndex, string arrayProp |
323+
result = this.getASuccessor(Label::spreadArgument(spreadIndex)).getMember(arrayProp) and
324+
i = spreadIndex + arrayProp.toInt()
325+
)
321326
}
322327

323328
/**
@@ -860,6 +865,15 @@ module API {
860865
.getStaticMember(name, DataFlow::MemberKind::getter())
861866
.getAReturn()
862867
)
868+
or
869+
exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest |
870+
fun.getRestParameter() = rest and
871+
rest.flow() = pred and
872+
invoke.getACallee() = fun and
873+
invoke.getArgument(argIndex) = rhs and
874+
argIndex >= rest.getIndex() and
875+
lbl = Label::member((argIndex - rest.getIndex()).toString())
876+
)
863877
)
864878
or
865879
exists(DataFlow::ClassNode cls, string name |
@@ -888,6 +902,11 @@ module API {
888902
i = -1 and lbl = Label::receiver()
889903
)
890904
or
905+
exists(int i |
906+
spreadArgumentPassing(base, i, rhs) and
907+
lbl = Label::spreadArgument(i)
908+
)
909+
or
891910
exists(DataFlow::SourceNode src, DataFlow::PropWrite pw |
892911
use(base, src) and pw = trackUseNode(src).getAPropertyWrite() and rhs = pw.getRhs()
893912
|
@@ -931,6 +950,21 @@ module API {
931950
)
932951
}
933952

953+
pragma[nomagic]
954+
private int firstSpreadIndex(InvokeExpr expr) {
955+
result = min(int i | expr.getArgument(i) instanceof SpreadElement)
956+
}
957+
958+
private predicate spreadArgumentPassing(TApiNode base, int i, DataFlow::Node spreadArray) {
959+
exists(DataFlow::Node use, DataFlow::SourceNode pred, int bound, InvokeExpr invoke |
960+
use(base, use) and
961+
pred = trackUseNode(use, _, bound, "") and
962+
invoke = pred.getAnInvocation().asExpr() and
963+
i = firstSpreadIndex(invoke) and
964+
spreadArray = invoke.getArgument(i - bound).(SpreadElement).getOperand().flow()
965+
)
966+
}
967+
934968
/**
935969
* Holds if `rhs` is the right-hand side of a definition of node `nd`.
936970
*/
@@ -1579,6 +1613,9 @@ module API {
15791613
/** Gets the edge label for the receiver. */
15801614
LabelReceiver receiver() { any() }
15811615

1616+
/** Gets the edge label for a spread argument passed at index `i`. */
1617+
LabelSpreadArgument spreadArgument(int i) { result.getIndex() = i }
1618+
15821619
/** Gets the `return` edge label. */
15831620
LabelReturn return() { any() }
15841621

@@ -1628,6 +1665,7 @@ module API {
16281665
} or
16291666
MkLabelReceiver() or
16301667
MkLabelReturn() or
1668+
MkLabelSpreadArgument(int index) { index = [0 .. 10] } or
16311669
MkLabelDecoratedClass() or
16321670
MkLabelDecoratedMember() or
16331671
MkLabelDecoratedParameter() or
@@ -1743,6 +1781,18 @@ module API {
17431781
override string toString() { result = "getReceiver()" }
17441782
}
17451783

1784+
/** A label representing an array passed as a spread argument at a given index. */
1785+
class LabelSpreadArgument extends ApiLabel, MkLabelSpreadArgument {
1786+
private int index;
1787+
1788+
LabelSpreadArgument() { this = MkLabelSpreadArgument(index) }
1789+
1790+
/** The argument index at which the spread argument appears. */
1791+
int getIndex() { result = index }
1792+
1793+
override string toString() { result = "getSpreadArgument(" + index + ")" }
1794+
}
1795+
17461796
/** A label for a function that is a wrapper around another function. */
17471797
class LabelForwardingFunction extends ApiLabel, MkLabelForwardingFunction {
17481798
override string toString() { result = "getForwardingFunction()" }

‎javascript/ql/lib/semmle/javascript/dataflow/Sources.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,12 @@ private module Cached {
254254
cached
255255
predicate invocation(DataFlow::SourceNode func, DataFlow::InvokeNode invoke) {
256256
hasLocalSource(invoke.getCalleeNode(), func)
257+
or
258+
exists(ClassDefinition cls, SuperCall call |
259+
hasLocalSource(cls.getSuperClass().flow(), func) and
260+
call.getBinder() = cls.getConstructor().getBody() and
261+
invoke = call.flow()
262+
)
257263
}
258264

259265
/**

‎javascript/ql/test/ApiGraphs/spread/VerifyAssertions.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +0,0 @@
1-
| tst.js:15:14:15:101 | /* def= ... r(0) */ | use moduleImport("something").getMember("exports").getMember("m2") has no outgoing edge labelled getParameter(0); it does have outgoing edges labelled getReceiver(), getReturn(). |
2-
| tst.js:16:14:16:101 | /* def= ... r(1) */ | use moduleImport("something").getMember("exports").getMember("m2") has no outgoing edge labelled getParameter(1); it does have outgoing edges labelled getReceiver(), getReturn(). |

‎javascript/ql/test/ApiGraphs/spread/tst.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ lib.m1({
1212

1313
function getArgs() {
1414
return [
15-
'x', /* def=moduleImport("something").getMember("exports").getMember("m2").getParameter(0) */
16-
'y', /* def=moduleImport("something").getMember("exports").getMember("m2").getParameter(1) */
15+
'x', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
16+
'y', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
1717
]
1818
}
1919

‎javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#select
22
| apollo.serverSide.ts:8:39:8:64 | get(fil ... => {}) | apollo.serverSide.ts:7:36:7:44 | { files } | apollo.serverSide.ts:8:43:8:50 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:8:43:8:50 | file.url | URL | apollo.serverSide.ts:7:36:7:44 | { files } | user-provided value |
3+
| apollo.serverSide.ts:18:37:18:62 | get(fil ... => {}) | apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:18:41:18:48 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:18:41:18:48 | file.url | URL | apollo.serverSide.ts:17:34:17:42 | { files } | user-provided value |
34
| axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | The $@ of this request depends on a $@. | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | endpoint | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | user-provided value |
45
| serverSide.js:18:5:18:20 | request(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:18:13:18:19 | tainted | The $@ of this request depends on a $@. | serverSide.js:18:13:18:19 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
56
| serverSide.js:20:5:20:24 | request.get(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:20:17:20:23 | tainted | The $@ of this request depends on a $@. | serverSide.js:20:17:20:23 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
@@ -31,6 +32,11 @@ edges
3132
| apollo.serverSide.ts:8:13:8:17 | files | apollo.serverSide.ts:8:28:8:31 | file | provenance | |
3233
| apollo.serverSide.ts:8:28:8:31 | file | apollo.serverSide.ts:8:43:8:46 | file | provenance | |
3334
| apollo.serverSide.ts:8:43:8:46 | file | apollo.serverSide.ts:8:43:8:50 | file.url | provenance | |
35+
| apollo.serverSide.ts:17:34:17:42 | files | apollo.serverSide.ts:18:11:18:15 | files | provenance | |
36+
| apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:17:34:17:42 | files | provenance | |
37+
| apollo.serverSide.ts:18:11:18:15 | files | apollo.serverSide.ts:18:26:18:29 | file | provenance | |
38+
| apollo.serverSide.ts:18:26:18:29 | file | apollo.serverSide.ts:18:41:18:44 | file | provenance | |
39+
| apollo.serverSide.ts:18:41:18:44 | file | apollo.serverSide.ts:18:41:18:48 | file.url | provenance | |
3440
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | axiosInterceptors.serverSide.js:19:11:19:28 | url | provenance | |
3541
| axiosInterceptors.serverSide.js:19:11:19:28 | url | axiosInterceptors.serverSide.js:20:23:20:25 | url | provenance | |
3642
| axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | provenance | |
@@ -91,6 +97,12 @@ nodes
9197
| apollo.serverSide.ts:8:28:8:31 | file | semmle.label | file |
9298
| apollo.serverSide.ts:8:43:8:46 | file | semmle.label | file |
9399
| apollo.serverSide.ts:8:43:8:50 | file.url | semmle.label | file.url |
100+
| apollo.serverSide.ts:17:34:17:42 | files | semmle.label | files |
101+
| apollo.serverSide.ts:17:34:17:42 | { files } | semmle.label | { files } |
102+
| apollo.serverSide.ts:18:11:18:15 | files | semmle.label | files |
103+
| apollo.serverSide.ts:18:26:18:29 | file | semmle.label | file |
104+
| apollo.serverSide.ts:18:41:18:44 | file | semmle.label | file |
105+
| apollo.serverSide.ts:18:41:18:48 | file.url | semmle.label | file.url |
94106
| axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | semmle.label | userProvidedUrl |
95107
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | semmle.label | { url } |
96108
| axiosInterceptors.serverSide.js:19:11:19:28 | url | semmle.label | url |

‎javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ function createApolloServer(typeDefs) {
1414

1515
const resolvers2 = {
1616
Mutation: {
17-
downloadFiles: async (_, { files }) => { // $ MISSING: Source[js/request-forgery]
18-
files.forEach((file) => { get(file.url, (res) => {}); }); // $ MISSING: Alert[js/request-forgery] Sink[js/request-forgery]
17+
downloadFiles: async (_, { files }) => { // $ Source[js/request-forgery]
18+
files.forEach((file) => { get(file.url, (res) => {}); }); // $ Alert[js/request-forgery] Sink[js/request-forgery]
1919
return true;
2020
},
2121
},

0 commit comments

Comments
 (0)