Skip to content

JS: Add AdditionalTypeTrackingStep #1788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
semmle-qlci merged 1 commit into from
Aug 24, 2019
Merged

JS: Add AdditionalTypeTrackingStep #1788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ module StepSummary {
basicLoadStep(pred, succ, prop) and
summary = LoadStep(prop)
)
or
any(AdditionalTypeTrackingStep st).step(pred, succ) and

@ghost ghost Aug 22, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is a library function that writes a property that can be read later.
Shouldn't the step type be configurable to support that case?

Suggested change
any(AdditionalTypeTrackingStep st).step(pred, succ) and
any(AdditionalTypeTrackingStep st).step(pred, succ, summary) and

@asger-semmle asger-semmle Aug 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all of our additional step mechanisms have this limitation, and we should totally fix that. I've often wanted to do something about this, but so far I haven't been able to find a robust way to do it right.

Concretely, the API should not restrict future implementation changes too much, and should be applied consistently across all the additional step mechanisms. Adding a step summary as an extra parameter might work here, but if we try the same in data-flow configurations, we will quickly run out of possible ways to overload this predicate, as we already have overloads using flow-labels as extra parameters.

@ghost ghost Aug 22, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Lets start with this version then, perhaps with a docstring warning about the default choice of flowstep (which all the other additional step mechanism also need).

@xiemaisi xiemaisi Aug 23, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TaintFunction API in the C++ library is a fairly nice way of supporting a special, but common, case of this. We're also using it for Go, and I think we should explore having something like it for JavaScript. (Though we'd probably have to attach the models to function calls, not the functions themselves, since we don't have those for most framework functions.)

summary = LevelStep()
}
}

Expand Down Expand Up @@ -338,3 +341,20 @@ module TypeBackTracker {
*/
TypeBackTracker end() { result.end() }
}

/**
* A data flow edge that should be followed by type tracking.
*
* Unlike `AdditionalFlowStep`, this type of edge does not affect
* the local data flow graph, and is not used by data-flow configurations.
*
* Note: For performance reasons, all subclasses of this class should be part
* of the standard library. For query-specific steps, consider including the
* custom steps in the type-tracking predicate itself.
*/
abstract class AdditionalTypeTrackingStep extends DataFlow::Node {
/**
* Holds if type-tracking should step from `pred` to `succ`.
*/
abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
}
48 changes: 26 additions & 22 deletions javascript/ql/test/library-tests/TypeTracking/ClassStyle.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
test_ApiObject
| tst.js:3:11:3:21 | new myapi() |
| tst.js:15:10:15:21 | api.chain1() |
| tst.js:15:10:15:30 | api.cha ... hain2() |
| tst.js:4:11:4:21 | new myapi() |
| tst.js:16:10:16:21 | api.chain1() |
| tst.js:16:10:16:30 | api.cha ... hain2() |
test_Connection
| tst.js:6:15:6:18 | conn |
| tst.js:10:5:10:19 | this.connection |
| tst.js:15:10:15:49 | api.cha ... ction() |
| tst.js:18:7:18:21 | getConnection() |
| tst.js:30:9:30:23 | getConnection() |
| tst.js:39:7:39:21 | getConnection() |
| tst.js:47:7:47:21 | getConnection() |
| tst.js:7:15:7:18 | conn |
| tst.js:11:5:11:19 | this.connection |
| tst.js:16:10:16:49 | api.cha ... ction() |
| tst.js:19:7:19:21 | getConnection() |
| tst.js:31:9:31:23 | getConnection() |
| tst.js:40:7:40:21 | getConnection() |
| tst.js:48:7:48:21 | getConnection() |
| tst.js:54:37:54:51 | getConnection() |
| tst.js:57:14:57:48 | config. ... ction') |
test_DataCallback
| tst.js:9:11:9:12 | cb |
| tst.js:20:1:22:1 | functio ... ata);\\n} |
| tst.js:29:26:29:27 | cb |
| tst.js:32:17:32:26 | data => {} |
| tst.js:37:10:37:19 | data => {} |
| tst.js:39:32:39:45 | getDataCurry() |
| tst.js:44:19:44:20 | cb |
| tst.js:47:32:47:60 | identit ... llback) |
| tst.js:10:11:10:12 | cb |
| tst.js:21:1:23:1 | functio ... ata);\\n} |
| tst.js:30:26:30:27 | cb |
| tst.js:33:17:33:26 | data => {} |
| tst.js:38:10:38:19 | data => {} |
| tst.js:40:32:40:45 | getDataCurry() |
| tst.js:45:19:45:20 | cb |
| tst.js:48:32:48:60 | identit ... llback) |
| tst.js:58:16:58:22 | x => {} |
test_DataValue
| tst.js:20:18:20:21 | data |
| tst.js:24:19:24:22 | data |
| tst.js:32:17:32:20 | data |
| tst.js:37:10:37:13 | data |
| tst.js:21:18:21:21 | data |
| tst.js:25:19:25:22 | data |
| tst.js:33:17:33:20 | data |
| tst.js:38:10:38:13 | data |
| tst.js:58:16:58:16 | x |
1 change: 1 addition & 0 deletions javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import javascript
import CustomStep

string chainableMethod() {
result = "chain1" or
Expand Down
19 changes: 19 additions & 0 deletions javascript/ql/test/library-tests/TypeTracking/CustomStep.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import javascript
private import DataFlow

predicate configStep(Node pred, Node succ) {
exists(CallNode setter, CallNode getter |
getter = moduleMember("@test/myconfig", "getConfigValue").getACall() and
setter = moduleMember("@test/myconfig", "setConfigValue").getACall() and
getter.getArgument(0).getStringValue() = setter.getArgument(0).getStringValue() and
pred = setter.getArgument(1) and
succ = getter
)
}

class CustomStep extends AdditionalTypeTrackingStep, Node {
override predicate step(Node pred, Node succ) {
pred = this and
configStep(pred, succ)
}
}
50 changes: 27 additions & 23 deletions javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,31 @@
apiObject
| tst.js:3:11:3:21 | new myapi() |
| tst.js:15:10:15:21 | api.chain1() |
| tst.js:15:10:15:30 | api.cha ... hain2() |
| tst.js:4:11:4:21 | new myapi() |
| tst.js:16:10:16:21 | api.chain1() |
| tst.js:16:10:16:30 | api.cha ... hain2() |
connection
| type tracker with call steps | tst.js:6:15:6:18 | conn |
| type tracker with call steps | tst.js:10:5:10:19 | this.connection |
| type tracker with call steps with property connection | tst.js:6:14:6:13 | this |
| type tracker without call steps | tst.js:15:10:15:49 | api.cha ... ction() |
| type tracker without call steps | tst.js:18:7:18:21 | getConnection() |
| type tracker without call steps | tst.js:30:9:30:23 | getConnection() |
| type tracker without call steps | tst.js:39:7:39:21 | getConnection() |
| type tracker without call steps | tst.js:47:7:47:21 | getConnection() |
| type tracker with call steps | tst.js:7:15:7:18 | conn |
| type tracker with call steps | tst.js:11:5:11:19 | this.connection |
| type tracker with call steps with property connection | tst.js:7:14:7:13 | this |
| type tracker without call steps | tst.js:16:10:16:49 | api.cha ... ction() |
| type tracker without call steps | tst.js:19:7:19:21 | getConnection() |
| type tracker without call steps | tst.js:31:9:31:23 | getConnection() |
| type tracker without call steps | tst.js:40:7:40:21 | getConnection() |
| type tracker without call steps | tst.js:48:7:48:21 | getConnection() |
| type tracker without call steps | tst.js:54:37:54:51 | getConnection() |
| type tracker without call steps | tst.js:57:14:57:48 | config. ... ction') |
dataCallback
| tst.js:9:11:9:12 | cb |
| tst.js:20:1:22:1 | functio ... ata);\\n} |
| tst.js:29:26:29:27 | cb |
| tst.js:32:17:32:26 | data => {} |
| tst.js:37:10:37:19 | data => {} |
| tst.js:39:32:39:45 | getDataCurry() |
| tst.js:44:19:44:20 | cb |
| tst.js:47:32:47:60 | identit ... llback) |
| tst.js:10:11:10:12 | cb |
| tst.js:21:1:23:1 | functio ... ata);\\n} |
| tst.js:30:26:30:27 | cb |
| tst.js:33:17:33:26 | data => {} |
| tst.js:38:10:38:19 | data => {} |
| tst.js:40:32:40:45 | getDataCurry() |
| tst.js:45:19:45:20 | cb |
| tst.js:48:32:48:60 | identit ... llback) |
| tst.js:58:16:58:22 | x => {} |
dataValue
| tst.js:20:18:20:21 | data |
| tst.js:24:19:24:22 | data |
| tst.js:32:17:32:20 | data |
| tst.js:37:10:37:13 | data |
| tst.js:21:18:21:21 | data |
| tst.js:25:19:25:22 | data |
| tst.js:33:17:33:20 | data |
| tst.js:38:10:38:13 | data |
| tst.js:58:16:58:16 | x |
1 change: 1 addition & 0 deletions javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import javascript
import CustomStep

string chainableMethod() {
result = "chain1" or
Expand Down
8 changes: 8 additions & 0 deletions javascript/ql/test/library-tests/TypeTracking/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import myapi from "@test/myapi";
import config from "@test/myconfig";

let api = new myapi();

Expand Down Expand Up @@ -49,3 +50,10 @@ identity(fakeGetDataCallback);

function realGetDataCallback(data) {} // not found due to missing summarization
function fakeGetDataCallback(notData) {} // should not be found

config.setConfigValue('connection', getConnection());

function getFromConfigFramework() {
let conn = config.getConfigValue('connection');
conn.getData(x => {});
}