Skip to content

Commit f22df76

Browse files
asgerfasgerf
authored
Merge pull request #8533 from asgerf/mad-receiver-token
JS/Ruby: Represent non-positional arguments with Argument/Parameter tokens
2 parents af1d949 + 0b30ecf commit f22df76

File tree

15 files changed

+702
-481
lines changed

15 files changed

+702
-481
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,10 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathToken token) {
120120
// API graphs do not use store/load steps for arrays
121121
token.getName() = ["ArrayElement", "Element"] and
122122
result = node.getUnknownMember()
123+
or
124+
token.getName() = "Parameter" and
125+
token.getAnArgument() = "this" and
126+
result = node.getReceiver()
123127
}
124128

125129
/**
@@ -129,6 +133,10 @@ bindingset[token]
129133
API::Node getExtraSuccessorFromInvoke(API::InvokeNode node, AccessPathToken token) {
130134
token.getName() = "Instance" and
131135
result = node.getInstance()
136+
or
137+
token.getName() = "Argument" and
138+
token.getAnArgument() = "this" and
139+
result.getARhs() = node.(DataFlow::CallNode).getReceiver()
132140
}
133141

134142
/**

‎javascript/ql/test/library-tests/frameworks/data/test.expected

Lines changed: 66 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -14,69 +14,73 @@ taintFlow
1414
| test.js:24:43:24:50 | source() | test.js:24:8:24:51 | testlib ... urce()) |
1515
| test.js:31:29:31:36 | source() | test.js:32:10:32:10 | y |
1616
| test.js:37:29:37:36 | source() | test.js:38:10:38:10 | y |
17-
| test.js:46:18:46:25 | source() | test.js:46:18:46:25 | source() |
18-
| test.js:47:22:47:29 | source() | test.js:47:22:47:29 | source() |
19-
| test.js:49:24:49:31 | source() | test.js:49:24:49:31 | source() |
20-
| test.js:53:27:53:34 | source() | test.js:53:27:53:34 | source() |
21-
| test.js:58:31:58:38 | source() | test.js:58:31:58:38 | source() |
22-
| test.js:62:34:62:41 | source() | test.js:62:34:62:41 | source() |
23-
| test.js:67:31:67:38 | source() | test.js:67:31:67:38 | source() |
24-
| test.js:68:34:68:41 | source() | test.js:68:34:68:41 | source() |
25-
| test.js:72:36:72:43 | source() | test.js:72:36:72:43 | source() |
26-
| test.js:73:39:73:46 | source() | test.js:73:39:73:46 | source() |
27-
| test.js:75:28:75:35 | source() | test.js:75:28:75:35 | source() |
28-
| test.js:76:31:76:38 | source() | test.js:76:31:76:38 | source() |
29-
| test.js:77:34:77:41 | source() | test.js:77:34:77:41 | source() |
30-
| test.js:81:28:81:35 | source() | test.js:81:28:81:35 | source() |
31-
| test.js:87:17:87:24 | source() | test.js:87:17:87:24 | source() |
32-
| test.js:88:17:88:24 | source() | test.js:88:17:88:24 | source() |
33-
| test.js:89:17:89:24 | source() | test.js:89:17:89:24 | source() |
17+
| test.js:43:29:43:36 | source() | test.js:44:10:44:10 | y |
18+
| test.js:47:33:47:40 | source() | test.js:49:10:49:13 | this |
19+
| test.js:54:18:54:25 | source() | test.js:54:18:54:25 | source() |
20+
| test.js:55:22:55:29 | source() | test.js:55:22:55:29 | source() |
21+
| test.js:57:24:57:31 | source() | test.js:57:24:57:31 | source() |
22+
| test.js:61:27:61:34 | source() | test.js:61:27:61:34 | source() |
23+
| test.js:66:31:66:38 | source() | test.js:66:31:66:38 | source() |
24+
| test.js:70:34:70:41 | source() | test.js:70:34:70:41 | source() |
25+
| test.js:75:31:75:38 | source() | test.js:75:31:75:38 | source() |
26+
| test.js:76:34:76:41 | source() | test.js:76:34:76:41 | source() |
27+
| test.js:80:36:80:43 | source() | test.js:80:36:80:43 | source() |
28+
| test.js:81:39:81:46 | source() | test.js:81:39:81:46 | source() |
29+
| test.js:83:28:83:35 | source() | test.js:83:28:83:35 | source() |
30+
| test.js:84:31:84:38 | source() | test.js:84:31:84:38 | source() |
31+
| test.js:85:34:85:41 | source() | test.js:85:34:85:41 | source() |
32+
| test.js:89:28:89:35 | source() | test.js:89:28:89:35 | source() |
33+
| test.js:95:17:95:24 | source() | test.js:95:17:95:24 | source() |
34+
| test.js:96:17:96:24 | source() | test.js:96:17:96:24 | source() |
35+
| test.js:97:17:97:24 | source() | test.js:97:17:97:24 | source() |
36+
| test.js:102:16:102:34 | testlib.getSource() | test.js:103:8:103:13 | source |
37+
| test.js:102:16:102:34 | testlib.getSource() | test.js:104:8:104:24 | source.continue() |
3438
isSink
35-
| test.js:46:18:46:25 | source() | test-sink |
36-
| test.js:47:22:47:29 | source() | test-sink |
37-
| test.js:49:24:49:31 | source() | test-sink |
38-
| test.js:53:27:53:34 | source() | test-sink |
39-
| test.js:55:38:55:38 | 4 | test-sink |
40-
| test.js:56:38:56:38 | 4 | test-sink |
41-
| test.js:57:38:57:38 | 4 | test-sink |
42-
| test.js:58:31:58:38 | source() | test-sink |
43-
| test.js:60:41:60:41 | 3 | test-sink |
44-
| test.js:61:41:61:41 | 3 | test-sink |
45-
| test.js:62:34:62:41 | source() | test-sink |
46-
| test.js:63:34:63:34 | 3 | test-sink |
47-
| test.js:65:38:65:38 | 3 | test-sink |
48-
| test.js:65:41:65:41 | 4 | test-sink |
49-
| test.js:66:38:66:38 | 3 | test-sink |
50-
| test.js:66:41:66:41 | 4 | test-sink |
51-
| test.js:67:31:67:38 | source() | test-sink |
52-
| test.js:67:41:67:41 | 4 | test-sink |
53-
| test.js:68:31:68:31 | 3 | test-sink |
54-
| test.js:68:34:68:41 | source() | test-sink |
55-
| test.js:70:43:70:43 | 3 | test-sink |
56-
| test.js:70:46:70:46 | 4 | test-sink |
57-
| test.js:71:43:71:43 | 3 | test-sink |
58-
| test.js:71:46:71:46 | 4 | test-sink |
59-
| test.js:72:36:72:43 | source() | test-sink |
60-
| test.js:72:46:72:46 | 4 | test-sink |
61-
| test.js:73:36:73:36 | 3 | test-sink |
62-
| test.js:73:39:73:46 | source() | test-sink |
63-
| test.js:75:28:75:35 | source() | test-sink |
64-
| test.js:75:38:75:38 | 2 | test-sink |
65-
| test.js:75:41:75:41 | 3 | test-sink |
66-
| test.js:76:28:76:28 | 1 | test-sink |
67-
| test.js:76:31:76:38 | source() | test-sink |
68-
| test.js:76:41:76:41 | 3 | test-sink |
69-
| test.js:77:28:77:28 | 1 | test-sink |
70-
| test.js:77:31:77:31 | 2 | test-sink |
71-
| test.js:77:34:77:41 | source() | test-sink |
72-
| test.js:78:28:78:28 | 1 | test-sink |
73-
| test.js:78:31:78:31 | 2 | test-sink |
74-
| test.js:78:34:78:34 | 3 | test-sink |
75-
| test.js:81:28:81:35 | source() | test-sink |
76-
| test.js:82:28:82:28 | 1 | test-sink |
77-
| test.js:87:17:87:24 | source() | test-sink |
78-
| test.js:88:17:88:24 | source() | test-sink |
79-
| test.js:89:17:89:24 | source() | test-sink |
39+
| test.js:54:18:54:25 | source() | test-sink |
40+
| test.js:55:22:55:29 | source() | test-sink |
41+
| test.js:57:24:57:31 | source() | test-sink |
42+
| test.js:61:27:61:34 | source() | test-sink |
43+
| test.js:63:38:63:38 | 4 | test-sink |
44+
| test.js:64:38:64:38 | 4 | test-sink |
45+
| test.js:65:38:65:38 | 4 | test-sink |
46+
| test.js:66:31:66:38 | source() | test-sink |
47+
| test.js:68:41:68:41 | 3 | test-sink |
48+
| test.js:69:41:69:41 | 3 | test-sink |
49+
| test.js:70:34:70:41 | source() | test-sink |
50+
| test.js:71:34:71:34 | 3 | test-sink |
51+
| test.js:73:38:73:38 | 3 | test-sink |
52+
| test.js:73:41:73:41 | 4 | test-sink |
53+
| test.js:74:38:74:38 | 3 | test-sink |
54+
| test.js:74:41:74:41 | 4 | test-sink |
55+
| test.js:75:31:75:38 | source() | test-sink |
56+
| test.js:75:41:75:41 | 4 | test-sink |
57+
| test.js:76:31:76:31 | 3 | test-sink |
58+
| test.js:76:34:76:41 | source() | test-sink |
59+
| test.js:78:43:78:43 | 3 | test-sink |
60+
| test.js:78:46:78:46 | 4 | test-sink |
61+
| test.js:79:43:79:43 | 3 | test-sink |
62+
| test.js:79:46:79:46 | 4 | test-sink |
63+
| test.js:80:36:80:43 | source() | test-sink |
64+
| test.js:80:46:80:46 | 4 | test-sink |
65+
| test.js:81:36:81:36 | 3 | test-sink |
66+
| test.js:81:39:81:46 | source() | test-sink |
67+
| test.js:83:28:83:35 | source() | test-sink |
68+
| test.js:83:38:83:38 | 2 | test-sink |
69+
| test.js:83:41:83:41 | 3 | test-sink |
70+
| test.js:84:28:84:28 | 1 | test-sink |
71+
| test.js:84:31:84:38 | source() | test-sink |
72+
| test.js:84:41:84:41 | 3 | test-sink |
73+
| test.js:85:28:85:28 | 1 | test-sink |
74+
| test.js:85:31:85:31 | 2 | test-sink |
75+
| test.js:85:34:85:41 | source() | test-sink |
76+
| test.js:86:28:86:28 | 1 | test-sink |
77+
| test.js:86:31:86:31 | 2 | test-sink |
78+
| test.js:86:34:86:34 | 3 | test-sink |
79+
| test.js:89:28:89:35 | source() | test-sink |
80+
| test.js:90:28:90:28 | 1 | test-sink |
81+
| test.js:95:17:95:24 | source() | test-sink |
82+
| test.js:96:17:96:24 | source() | test-sink |
83+
| test.js:97:17:97:24 | source() | test-sink |
8084
syntaxErrors
8185
| Member[foo |
8286
| Member[foo] .Member[bar] |

‎javascript/ql/test/library-tests/frameworks/data/test.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,14 @@ function testPreserveTaint() {
4040
testlib.taintIntoCallback(source(), undefined, undefined, y => {
4141
sink(y); // OK - only callback 1-2 receive taint
4242
});
43+
testlib.taintIntoCallback(source(), function(y) {
44+
sink(y); // NOT OK
45+
sink(this); // OK - receiver is not tainted
46+
});
47+
testlib.taintIntoCallbackThis(source(), function(y) {
48+
sink(y); // OK - only receiver is tainted
49+
sink(this); // NOT OK
50+
});
4351
}
4452

4553
function testSinks() {
@@ -89,3 +97,10 @@ function testSinks() {
8997
testlib.sink3(source()); // NOT OK
9098
testlib.sink4(source()); // OK
9199
}
100+
101+
function testFlowThroughReceiver() {
102+
let source = testlib.getSource();
103+
sink(source); // NOT OK
104+
sink(source.continue()); // NOT OK
105+
sink(source.blah()); // OK
106+
}

‎javascript/ql/test/library-tests/frameworks/data/test.ql

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@ class Steps extends ModelInput::SummaryModelCsv {
99
[
1010
"testlib;;Member[preserveTaint];Argument[0];ReturnValue;taint",
1111
"testlib;;Member[taintIntoCallback];Argument[0];Argument[1..2].Parameter[0];taint",
12+
"testlib;;Member[taintIntoCallbackThis];Argument[0];Argument[1..2].Parameter[this];taint",
1213
"testlib;;Member[preserveArgZeroAndTwo];Argument[0,2];ReturnValue;taint",
1314
"testlib;;Member[preserveAllButFirstArgument];Argument[1..];ReturnValue;taint",
14-
"testlib;;Member[preserveAllIfCall].Call;Argument[0..];ReturnValue;taint"
15+
"testlib;;Member[preserveAllIfCall].Call;Argument[0..];ReturnValue;taint",
16+
"testlib;;Member[getSource].ReturnValue.Member[continue];Argument[this];ReturnValue;taint",
1517
]
1618
}
1719
}
@@ -35,11 +37,17 @@ class Sinks extends ModelInput::SinkModelCsv {
3537
}
3638
}
3739

40+
class Sources extends ModelInput::SourceModelCsv {
41+
override predicate row(string row) { row = "testlib;;Member[getSource].ReturnValue;test-source" }
42+
}
43+
3844
class BasicTaintTracking extends TaintTracking::Configuration {
3945
BasicTaintTracking() { this = "BasicTaintTracking" }
4046

4147
override predicate isSource(DataFlow::Node source) {
4248
source.(DataFlow::CallNode).getCalleeName() = "source"
49+
or
50+
source = ModelOutput::getASourceNode("test-source").getAnImmediateUse()
4351
}
4452

4553
override predicate isSink(DataFlow::Node sink) {

‎ruby/ql/lib/codeql/ruby/ApiGraphs.qll

Lines changed: 34 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -473,36 +473,6 @@ module API {
473473
/** Gets a data flow node that flows to the RHS of a def-node. */
474474
private DataFlow::LocalSourceNode defCand() { result = defCand(TypeBackTracker::end()) }
475475

476-
private Label::ApiLabel getLabelFromArgumentPosition(DataFlowDispatch::ArgumentPosition pos) {
477-
exists(int n |
478-
pos.isPositional(n) and
479-
result = Label::parameter(n)
480-
)
481-
or
482-
exists(string name |
483-
pos.isKeyword(name) and
484-
result = Label::keywordParameter(name)
485-
)
486-
or
487-
pos.isBlock() and
488-
result = Label::blockParameter()
489-
}
490-
491-
private Label::ApiLabel getLabelFromParameterPosition(DataFlowDispatch::ParameterPosition pos) {
492-
exists(int n |
493-
pos.isPositional(n) and
494-
result = Label::parameter(n)
495-
)
496-
or
497-
exists(string name |
498-
pos.isKeyword(name) and
499-
result = Label::keywordParameter(name)
500-
)
501-
or
502-
pos.isBlock() and
503-
result = Label::blockParameter()
504-
}
505-
506476
/**
507477
* Holds if there should be a `lbl`-edge from the given call to an argument.
508478
*/
@@ -512,7 +482,7 @@ module API {
512482
) {
513483
exists(DataFlowDispatch::ArgumentPosition argPos |
514484
argument.sourceArgumentOf(call.asExpr(), argPos) and
515-
lbl = getLabelFromArgumentPosition(argPos)
485+
lbl = Label::getLabelFromArgumentPosition(argPos)
516486
)
517487
}
518488

@@ -525,7 +495,7 @@ module API {
525495
) {
526496
exists(DataFlowDispatch::ParameterPosition paramPos |
527497
paramNode.isSourceParameterOf(callable.asExpr().getExpr(), paramPos) and
528-
lbl = getLabelFromParameterPosition(paramPos)
498+
lbl = Label::getLabelFromParameterPosition(paramPos)
529499
)
530500
}
531501

@@ -803,5 +773,37 @@ module API {
803773

804774
/** Gets the label for the edge from the root node to a custom entry point of the given name. */
805775
LabelEntryPoint entryPoint(API::EntryPoint name) { result.getName() = name }
776+
777+
/** Gets the API graph label corresponding to the given argument position. */
778+
Label::ApiLabel getLabelFromArgumentPosition(DataFlowDispatch::ArgumentPosition pos) {
779+
exists(int n |
780+
pos.isPositional(n) and
781+
result = Label::parameter(n)
782+
)
783+
or
784+
exists(string name |
785+
pos.isKeyword(name) and
786+
result = Label::keywordParameter(name)
787+
)
788+
or
789+
pos.isBlock() and
790+
result = Label::blockParameter()
791+
}
792+
793+
/** Gets the API graph label corresponding to the given parameter position. */
794+
Label::ApiLabel getLabelFromParameterPosition(DataFlowDispatch::ParameterPosition pos) {
795+
exists(int n |
796+
pos.isPositional(n) and
797+
result = Label::parameter(n)
798+
)
799+
or
800+
exists(string name |
801+
pos.isKeyword(name) and
802+
result = Label::keywordParameter(name)
803+
)
804+
or
805+
pos.isBlock() and
806+
result = Label::blockParameter()
807+
}
806808
}
807809
}

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,8 @@ private module Cached {
257257
name = any(KeywordParameter kp).getName()
258258
or
259259
exists(any(Call c).getKeywordArgument(name))
260+
or
261+
FlowSummaryImplSpecific::ParsePositions::isParsedKeywordParameterPosition(_, name)
260262
}
261263

262264
cached
@@ -270,7 +272,11 @@ private module Cached {
270272
or
271273
FlowSummaryImplSpecific::ParsePositions::isParsedArgumentPosition(_, pos)
272274
} or
273-
TKeywordParameterPosition(string name) { name = any(KeywordParameter kp).getName() }
275+
TKeywordParameterPosition(string name) {
276+
name = any(KeywordParameter kp).getName()
277+
or
278+
FlowSummaryImplSpecific::ParsePositions::isParsedKeywordArgumentPosition(_, name)
279+
}
274280
}
275281

276282
import Cached

0 commit comments

Comments
 (0)