Skip to content

JS: fix context sensitivity bug in store-load matching #8478

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.

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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 17 additions & 11 deletions javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
Original file line number Diff line number Diff line change
Expand Up @@ -1365,27 +1365,31 @@ private predicate loadStep(

/**
* Holds if there is flow to `base.startProp`, and `base.startProp` flows to `nd.endProp` under `cfg/summary`.
*
* If `onlyRelevantInCall` is true, the `base` object will not be propagated out of return edges, because
* the flow that originally reached `base.startProp` used a call edge.
*/
pragma[nomagic]
private predicate reachableFromStoreBase(
string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
DataFlow::Configuration cfg, PathSummary summary
DataFlow::Configuration cfg, PathSummary summary, boolean onlyRelevantInCall
) {
exists(PathSummary s1, PathSummary s2, DataFlow::Node rhs |
reachableFromSource(rhs, cfg, s1)
reachableFromSource(rhs, cfg, s1) and
onlyRelevantInCall = s1.hasCall()
or
reachableFromStoreBase(_, _, _, rhs, cfg, s1)
reachableFromStoreBase(_, _, _, rhs, cfg, s1, onlyRelevantInCall)
|
storeStep(rhs, nd, startProp, cfg, s2) and
endProp = startProp and
base = nd and
summary =
MkPathSummary(false, s1.hasCall().booleanOr(s2.hasCall()), DataFlow::FlowLabel::data(),
DataFlow::FlowLabel::data())
MkPathSummary(false, s2.hasCall(), DataFlow::FlowLabel::data(), DataFlow::FlowLabel::data())
)
or
exists(PathSummary newSummary, PathSummary oldSummary |
reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary) and
reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary,
onlyRelevantInCall) and
summary = oldSummary.appendValuePreserving(newSummary)
)
}
Expand All @@ -1399,14 +1403,16 @@ private predicate reachableFromStoreBase(
pragma[noinline]
private predicate reachableFromStoreBaseStep(
string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary
DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary,
boolean onlyRelevantInCall
) {
exists(DataFlow::Node mid |
reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary) and
flowStep(mid, cfg, nd, newSummary)
reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
flowStep(mid, cfg, nd, newSummary) and
onlyRelevantInCall.booleanAnd(newSummary.hasReturn()) = false
or
exists(string midProp |
reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary) and
reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
isAdditionalLoadStoreStep(mid, nd, midProp, endProp, cfg) and
newSummary = PathSummary::level()
)
Expand Down Expand Up @@ -1446,7 +1452,7 @@ private predicate storeToLoad(
PathSummary s1, PathSummary s2
|
storeStep(pred, storeBase, storeProp, cfg, s1) and
reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2) and
reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2, _) and
oldSummary = s1.appendValuePreserving(s2) and
loadStep(loadBase, succ, loadProp, cfg, newSummary)
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: minorAnalysis
---
* Fixed an issue that would sometimes prevent the data-flow analysis from finding flow
paths through a function that stores its result on an object.
This may lead to more results for the security queries.
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ typeInferenceMismatch
| string-replace.js:3:13:3:20 | source() | string-replace.js:21:6:21:41 | safe(). ... taint) |
| string-replace.js:3:13:3:20 | source() | string-replace.js:22:6:22:48 | safe(). ... taint) |
| string-replace.js:3:13:3:20 | source() | string-replace.js:24:6:24:45 | taint.r ... + '!') |
| summarize-store-load-in-call.js:9:15:9:22 | source() | summarize-store-load-in-call.js:9:10:9:23 | blah(source()) |
| thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
| thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
| tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as dummy from 'dummy';

function blah(obj) {
obj.prop = obj.prop + "x";
return obj.prop;
}

function test() {
sink(blah(source())); // NOT OK

blah(); // ensure more than one call site exists
}