Skip to content

Commit 82d61e4

Browse files
asgerfasgerf
committed
Merge branch 'js/shared-dataflow-branch' into js/shared-dataflow-merge-main
2 parents d52bc97 + c2e9dca commit 82d61e4

25 files changed

+404
-84
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ private class ArrayIterationCallbackAsPartialInvoke extends DataFlow::PartialInv
6969
* A flow step propagating the exception thrown from a callback to a method whose name coincides
7070
* a built-in Array iteration method, such as `forEach` or `map`.
7171
*/
72-
private class IteratorExceptionStep extends DataFlow::SharedFlowStep {
72+
private class IteratorExceptionStep extends DataFlow::LegacyFlowStep {
7373
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
7474
exists(DataFlow::MethodCallNode call |
7575
call.getMethodName() = ["forEach", "each", "map", "filter", "some", "every", "fold", "reduce"] and

‎javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowImplConsistency.qll

+2
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
2626
or
2727
n instanceof FlowSummaryDynamicParameterArrayNode
2828
or
29+
n instanceof FlowSummaryDefaultExceptionalReturn
30+
or
2931
n instanceof GenericSynthesizedNode
3032
or
3133
n = DataFlow::globalAccessPathRootPseudoNode()

‎javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll

+3
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,9 @@ private module Cached {
100100
// So it doesn't cause negative recursion but it might look a bit surprising.
101101
FlowSummaryPrivate::Steps::summaryStoreStep(sn, MkAwaited(), _)
102102
} or
103+
TFlowSummaryDefaultExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
104+
not DataFlowPrivate::mentionsExceptionalReturn(callable)
105+
} or
103106
TSynthCaptureNode(VariableCapture::VariableCaptureOutput::SynthesizedCaptureNode node) or
104107
TGenericSynthesizedNode(AstNode node, string tag, DataFlowPrivate::DataFlowCallable container) {
105108
any(AdditionalFlowInternal flow).needsSynthesizedNode(node, tag, container)

‎javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

+72-2
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,33 @@ class FlowSummaryIntermediateAwaitStoreNode extends DataFlow::Node,
112112
}
113113
}
114114

115+
predicate mentionsExceptionalReturn(FlowSummaryImpl::Public::SummarizedCallable callable) {
116+
exists(FlowSummaryImpl::Private::SummaryNode node | node.getSummarizedCallable() = callable |
117+
FlowSummaryImpl::Private::summaryReturnNode(node, MkExceptionalReturnKind())
118+
or
119+
FlowSummaryImpl::Private::summaryOutNode(_, node, MkExceptionalReturnKind())
120+
)
121+
}
122+
123+
/**
124+
* Exceptional return node in a summarized callable whose summary does not mention `ReturnValue[exception]`.
125+
*
126+
* By default, every call inside such a callable will forward their exceptional return to the caller's
127+
* exceptional return, i.e. exceptions are not caught.
128+
*/
129+
class FlowSummaryDefaultExceptionalReturn extends DataFlow::Node,
130+
TFlowSummaryDefaultExceptionalReturn
131+
{
132+
private FlowSummaryImpl::Public::SummarizedCallable callable;
133+
134+
FlowSummaryDefaultExceptionalReturn() { this = TFlowSummaryDefaultExceptionalReturn(callable) }
135+
136+
FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = callable }
137+
138+
cached
139+
override string toString() { result = "[default exceptional return] " + callable }
140+
}
141+
115142
class CaptureNode extends DataFlow::Node, TSynthCaptureNode {
116143
/** Gets the underlying node from the variable-capture library. */
117144
VariableCaptureOutput::SynthesizedCaptureNode getNode() {
@@ -296,6 +323,9 @@ private predicate returnNodeImpl(DataFlow::Node node, ReturnKind kind) {
296323
)
297324
or
298325
FlowSummaryImpl::Private::summaryReturnNode(node.(FlowSummaryNode).getSummaryNode(), kind)
326+
or
327+
node instanceof FlowSummaryDefaultExceptionalReturn and
328+
kind = MkExceptionalReturnKind()
299329
}
300330

301331
private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
@@ -311,6 +341,10 @@ private DataFlow::Node getAnOutNodeImpl(DataFlowCall call, ReturnKind kind) {
311341
or
312342
FlowSummaryImpl::Private::summaryOutNode(call.(SummaryCall).getReceiver(),
313343
result.(FlowSummaryNode).getSummaryNode(), kind)
344+
or
345+
kind = MkExceptionalReturnKind() and
346+
result.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable() =
347+
call.(SummaryCall).getSummarizedCallable()
314348
}
315349

316350
class ReturnNode extends DataFlow::Node {
@@ -404,6 +438,19 @@ abstract class LibraryCallable extends string {
404438
DataFlow::InvokeNode getACallSimple() { none() }
405439
}
406440

441+
/** Internal subclass of `LibraryCallable`, whose member predicates should not be visible on `SummarizedCallable`. */
442+
abstract class LibraryCallableInternal extends LibraryCallable {
443+
bindingset[this]
444+
LibraryCallableInternal() { any() }
445+
446+
/**
447+
* Gets a call to this library callable.
448+
*
449+
* Same as `getACall()` but is evaluated later and may depend negatively on `getACall()`.
450+
*/
451+
DataFlow::InvokeNode getACallStage2() { none() }
452+
}
453+
407454
private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosition pos) {
408455
exists(Parameter parameter |
409456
parameter = c.asSourceCallable().(Function).getParameter(pos.asPositional()) and
@@ -505,6 +552,8 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) {
505552
or
506553
result.asLibraryCallable() = node.(FlowSummaryIntermediateAwaitStoreNode).getSummarizedCallable()
507554
or
555+
result.asLibraryCallable() = node.(FlowSummaryDefaultExceptionalReturn).getSummarizedCallable()
556+
or
508557
node = TGenericSynthesizedNode(_, _, result)
509558
}
510559

@@ -865,6 +914,8 @@ class SummaryCall extends DataFlowCall, MkSummaryCall {
865914

866915
/** Gets the receiver node. */
867916
FlowSummaryImpl::Private::SummaryNode getReceiver() { result = receiver }
917+
918+
FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = enclosingCallable }
868919
}
869920

870921
/**
@@ -976,7 +1027,11 @@ DataFlowCallable viableCallable(DataFlowCall node) {
9761027
or
9771028
exists(LibraryCallable callable |
9781029
result = MkLibraryCallable(callable) and
979-
node.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
1030+
node.asOrdinaryCall() =
1031+
[
1032+
callable.getACall(), callable.getACallSimple(),
1033+
callable.(LibraryCallableInternal).getACallStage2()
1034+
]
9801035
)
9811036
or
9821037
result.asSourceCallableNotExterns() = node.asImpliedLambdaCall()
@@ -1217,14 +1272,29 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
12171272

12181273
predicate localMustFlowStep(Node node1, Node node2) { node1 = node2.getImmediatePredecessor() }
12191274

1275+
/**
1276+
* Holds if `node1 -> node2` should be removed as a jump step.
1277+
*
1278+
* Currently this is done as a workaround for the local steps generated from IIFEs.
1279+
*/
1280+
private predicate excludedJumpStep(Node node1, Node node2) {
1281+
exists(ImmediatelyInvokedFunctionExpr iife |
1282+
iife.argumentPassing(node2.asExpr(), node1.asExpr())
1283+
or
1284+
node1 = iife.getAReturnedExpr().flow() and
1285+
node2 = iife.getInvocation().flow()
1286+
)
1287+
}
1288+
12201289
/**
12211290
* Holds if data can flow from `node1` to `node2` through a non-local step
12221291
* that does not follow a call edge. For example, a step through a global
12231292
* variable.
12241293
*/
12251294
predicate jumpStep(Node node1, Node node2) {
12261295
valuePreservingStep(node1, node2) and
1227-
node1.getContainer() != node2.getContainer()
1296+
node1.getContainer() != node2.getContainer() and
1297+
not excludedJumpStep(node1, node2)
12281298
or
12291299
FlowSummaryPrivate::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
12301300
node2.(FlowSummaryNode).getSummaryNode())

‎javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll

+6-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import sharedlib.DataFlowImplCommon
99
private import sharedlib.FlowSummaryImpl::Private as Private
1010
private import sharedlib.FlowSummaryImpl::Public
1111
private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
12+
private import semmle.javascript.internal.flow_summaries.ExceptionFlow
1213

1314
/**
1415
* A class of callables that are candidates for flow summary modeling.
@@ -131,7 +132,11 @@ ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() and Stag
131132
private module FlowSummaryStepInput implements Private::StepsInputSig {
132133
DataFlowCall getACall(SummarizedCallable sc) {
133134
exists(LibraryCallable callable | callable = sc |
134-
result.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
135+
result.asOrdinaryCall() =
136+
[
137+
callable.getACall(), callable.getACallSimple(),
138+
callable.(LibraryCallableInternal).getACallStage2()
139+
]
135140
)
136141
}
137142
}

‎javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll

+5-27
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
88
private js::Function getLambdaFromVariable(js::LocalVariable variable) {
99
result.getVariable() = variable
1010
or
11-
result = variable.getAnAssignedExpr()
11+
result = variable.getAnAssignedExpr().getUnderlyingValue()
1212
or
1313
exists(js::ClassDeclStmt cls |
1414
result = cls.getConstructor().getBody() and
@@ -23,35 +23,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
2323
or
2424
isTopLevelLike(container.(js::ImmediatelyInvokedFunctionExpr).getEnclosingContainer())
2525
or
26-
// Functions declared in a top-level with no parameters and can't generate flow-through, except through 'this'
27-
// which we rule out with a few syntactic checks. In this case we treat its captured variables as singletons.
28-
// NOTE: This was done to prevent a blow-up in fiddlesalad where a function called 'Runtime' captures 7381 variables but is only called once.
29-
exists(js::Function fun |
30-
container = fun and
31-
fun.getNumParameter() = 0 and
32-
isTopLevelLike(fun.getEnclosingContainer()) and
33-
not mayHaveFlowThroughThisArgument(fun)
34-
)
35-
or
36-
// Container declaring >100 captured variables tend to be singletons and are too expensive anyway
26+
// Containers declaring >100 captured variables tend to be singletons and are too expensive anyway
3727
strictcount(js::LocalVariable v | v.isCaptured() and v.getDeclaringContainer() = container) >
3828
100
3929
}
4030

41-
private predicate hasLocalConstructorCall(js::Function fun) {
42-
fun = getLambdaFromVariable(any(js::NewExpr e).getCallee().(js::VarAccess).getVariable())
43-
}
44-
45-
private predicate mayHaveFlowThroughThisArgument(js::Function fun) {
46-
any(js::ThisExpr e).getBinder() = fun and
47-
not hasLocalConstructorCall(fun) and // 'this' argument is assumed to be a fresh object
48-
(
49-
exists(fun.getAReturnedExpr())
50-
or
51-
exists(js::YieldExpr e | e.getContainer() = fun)
52-
)
53-
}
54-
5531
class CapturedVariable extends LocalVariableOrThis {
5632
CapturedVariable() {
5733
DataFlowImplCommon::forceCachingInSameStage() and
@@ -172,9 +148,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
172148
predicate hasAliasedAccess(Expr e) {
173149
e = this
174150
or
151+
e.(js::Expr).getUnderlyingValue() = this
152+
or
175153
exists(js::LocalVariable variable |
176154
this = getLambdaFromVariable(variable) and
177-
e = variable.getAnAccess()
155+
e.(js::Expr).getUnderlyingValue() = variable.getAnAccess()
178156
)
179157
}
180158
}

‎javascript/ql/lib/semmle/javascript/filters/ClassifyFiles.qll

+2
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ predicate isTestFile(File f) {
6161
)
6262
or
6363
f.getAbsolutePath().regexpMatch(".*/__(mocks|tests)__/.*")
64+
or
65+
f.getBaseName().matches("%.test.%")
6466
}
6567

6668
/**

‎javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
private import AmbiguousCoreMethods
22
private import Arrays
33
private import AsyncAwait
4+
private import ExceptionFlow
45
private import ForOfLoops
56
private import Generators
67
private import Iterators

‎javascript/ql/lib/semmle/javascript/internal/flow_summaries/ExceptionFlow.qll

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* Contains a summary for propagating exceptions out of callbacks
3+
*/
4+
5+
private import javascript
6+
private import FlowSummaryUtil
7+
private import semmle.javascript.dataflow.internal.AdditionalFlowInternal
8+
private import semmle.javascript.dataflow.internal.DataFlowPrivate
9+
private import semmle.javascript.dataflow.FlowSummary
10+
private import semmle.javascript.internal.flow_summaries.Promises
11+
12+
private predicate isCallback(DataFlow::SourceNode node) {
13+
node instanceof DataFlow::FunctionNode
14+
or
15+
node instanceof DataFlow::PartialInvokeNode
16+
or
17+
exists(DataFlow::SourceNode prev |
18+
isCallback(prev) and
19+
DataFlow::argumentPassingStep(_, prev.getALocalUse(), _, node)
20+
)
21+
}
22+
23+
/**
24+
* Summary that propagates exceptions out of callbacks back to the caller.
25+
*
26+
* This summary only applies to calls that have no other call targets.
27+
* See also `FlowSummaryDefaultExceptionalReturn`, which handles calls that have a summary target,
28+
* but where the summary does not mention `ReturnValue[exception]`.
29+
*/
30+
private class ExceptionFlowSummary extends SummarizedCallable, LibraryCallableInternal {
31+
ExceptionFlowSummary() { this = "Exception propagator" }
32+
33+
override DataFlow::CallNode getACallStage2() {
34+
not exists(result.getACallee()) and
35+
not exists(SummarizedCallable c | result = [c.getACall(), c.getACallSimple()]) and
36+
// Avoid a few common cases where the exception should not propagate back
37+
not result.getCalleeName() = ["addEventListener", EventEmitter::on()] and
38+
not result = promiseConstructorRef().getAnInvocation() and
39+
// Restrict to cases where a callback is known to flow in, as lambda flow in DataFlowImplCommon blows up otherwise
40+
isCallback(result.getAnArgument().getALocalSource())
41+
}
42+
43+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
44+
preservesValue = true and
45+
input = "Argument[0..].ReturnValue[exception]" and
46+
output = "ReturnValue[exception]"
47+
}
48+
}

‎javascript/ql/lib/semmle/javascript/internal/flow_summaries/Promises.qll

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ private import javascript
66
private import semmle.javascript.dataflow.FlowSummary
77
private import FlowSummaryUtil
88

9-
private DataFlow::SourceNode promiseConstructorRef() {
9+
DataFlow::SourceNode promiseConstructorRef() {
1010
result = Promises::promiseConstructorRef()
1111
or
1212
result = DataFlow::moduleImport("bluebird")

‎javascript/ql/lib/semmle/javascript/security/dataflow/InsecureRandomnessQuery.qll

+6-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import javascript
1111
private import semmle.javascript.security.SensitiveActions
1212
import InsecureRandomnessCustomizations::InsecureRandomness
1313
private import InsecureRandomnessCustomizations::InsecureRandomness as InsecureRandomness
14+
private import semmle.javascript.filters.ClassifyFiles as ClassifyFiles
1415

1516
/**
1617
* A taint tracking configuration for random values that are not cryptographically secure.
@@ -20,7 +21,11 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig {
2021

2122
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2223

23-
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
24+
predicate isBarrier(DataFlow::Node node) {
25+
node instanceof Sanitizer
26+
or
27+
ClassifyFiles::isTestFile(node.getFile())
28+
}
2429

2530
predicate isBarrierOut(DataFlow::Node node) {
2631
// stop propagation at the sinks to avoid double reporting

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ legacyDataFlowDifference
1717
| callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
1818
| capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
1919
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
20+
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
2021
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
2122
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
2223
| constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
@@ -109,7 +110,6 @@ flow
109110
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
110111
| capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
111112
| capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
112-
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
113113
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
114114
| capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
115115
| capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ legacyDataFlowDifference
1111
| callbacks.js:44:17:44:24 | source() | callbacks.js:38:35:38:35 | x | only flow with NEW data flow library |
1212
| capture-flow.js:89:13:89:20 | source() | capture-flow.js:89:6:89:21 | test3c(source()) | only flow with NEW data flow library |
1313
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:102:6:102:20 | test5("safe")() | only flow with OLD data flow library |
14+
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint | only flow with OLD data flow library |
1415
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:40:8:40:14 | e.taint | only flow with NEW data flow library |
1516
| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library |
1617
| constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library |
@@ -84,7 +85,6 @@ flow
8485
| capture-flow.js:101:12:101:19 | source() | capture-flow.js:101:6:101:22 | test5(source())() |
8586
| capture-flow.js:110:12:110:19 | source() | capture-flow.js:106:14:106:14 | x |
8687
| capture-flow.js:118:37:118:44 | source() | capture-flow.js:114:14:114:14 | x |
87-
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:123:14:123:26 | orderingTaint |
8888
| capture-flow.js:126:25:126:32 | source() | capture-flow.js:129:14:129:26 | orderingTaint |
8989
| capture-flow.js:177:26:177:33 | source() | capture-flow.js:173:14:173:14 | x |
9090
| capture-flow.js:187:34:187:41 | source() | capture-flow.js:183:14:183:14 | x |

‎javascript/ql/test/library-tests/TaintTracking/capture-flow.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ global.doEscape(testEscapeViaReturn(source()));
120120
function ordering() {
121121
var orderingTaint;
122122
global.addEventListener('click', () => {
123-
sink(orderingTaint); // NOT OK
123+
sink(orderingTaint); // NOT OK [INCONSISTENCY]
124124
});
125125
global.addEventListener('load', () => {
126126
orderingTaint = source();

0 commit comments

Comments
 (0)