Skip to content

Commit 8256f2e

Browse files
author
Esben Sparre Andreasen
authored
Merge pull request #1308 from asger-semmle/exceptional-flow
JS: Add flow through exceptions
2 parents 762c977 + 9c1208e commit 8256f2e

File tree

11 files changed

+435
-8
lines changed

11 files changed

+435
-8
lines changed

‎change-notes/1.21/analysis-javascript.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
* The security queries now track data flow through Base64 decoders such as the Node.js `Buffer` class, the DOM function `atob`, and a number of npm packages intcluding [`abab`](https://www.npmjs.com/package/abab), [`atob`](https://www.npmjs.com/package/atob), [`btoa`](https://www.npmjs.com/package/btoa), [`base-64`](https://www.npmjs.com/package/base-64), [`js-base64`](https://www.npmjs.com/package/js-base64), [`Base64.js`](https://www.npmjs.com/package/Base64) and [`base64-js`](https://www.npmjs.com/package/base64-js).
1515

16+
* The security queries now track data flow through exceptions.
17+
1618
* The security queries now treat comparisons with symbolic constants as sanitizers, resulting in fewer false positives.
1719

1820
* TypeScript 3.4 features are now supported.

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,3 +224,27 @@ private class PromiseTaintStep extends TaintTracking::AdditionalTaintStep {
224224
pred = source and succ = this
225225
}
226226
}
227+
228+
/**
229+
* A flow step propagating the exception thrown from a callback to a method whose name coincides
230+
* a built-in Array iteration method, such as `forEach` or `map`.
231+
*/
232+
private class IteratorExceptionStep extends DataFlow::MethodCallNode, DataFlow::AdditionalFlowStep {
233+
IteratorExceptionStep() {
234+
exists(string name | name = getMethodName() |
235+
name = "forEach" or
236+
name = "each" or
237+
name = "map" or
238+
name = "filter" or
239+
name = "some" or
240+
name = "every" or
241+
name = "fold" or
242+
name = "reduce"
243+
)
244+
}
245+
246+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
247+
pred = getAnArgument().(DataFlow::FunctionNode).getExceptionalReturn() and
248+
succ = this.getExceptionalReturn()
249+
}
250+
}

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,8 @@ abstract class Configuration extends string {
172172
isBarrierGuard(guard) and
173173
guard.blocks(node)
174174
)
175+
or
176+
none() // relax type inference to account for overriding
175177
}
176178

177179
/**
@@ -599,14 +601,23 @@ private predicate appendStep(
599601
* configuration `cfg`, possibly through callees.
600602
*/
601603
private predicate flowThroughCall(
602-
DataFlow::Node input, DataFlow::Node invk, DataFlow::Configuration cfg, PathSummary summary
604+
DataFlow::Node input, DataFlow::Node output, DataFlow::Configuration cfg, PathSummary summary
603605
) {
604606
exists(Function f, DataFlow::ValueNode ret |
605607
ret.asExpr() = f.getAReturnedExpr() and
606-
calls(invk, f) and // Do not consider partial calls
608+
calls(output, f) and // Do not consider partial calls
609+
reachableFromInput(f, output, input, ret, cfg, summary) and
610+
not cfg.isBarrier(ret, output) and
611+
not cfg.isLabeledBarrier(output, summary.getEndLabel())
612+
)
613+
or
614+
exists(Function f, DataFlow::Node invk, DataFlow::Node ret |
615+
DataFlow::exceptionalFunctionReturnNode(ret, f) and
616+
DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
617+
calls(invk, f) and
607618
reachableFromInput(f, invk, input, ret, cfg, summary) and
608-
not cfg.isBarrier(ret, invk) and
609-
not cfg.isLabeledBarrier(invk, summary.getEndLabel())
619+
not cfg.isBarrier(ret, output) and
620+
not cfg.isLabeledBarrier(output, summary.getEndLabel())
610621
)
611622
}
612623

‎javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ module DataFlow {
4141
TDestructuredModuleImportNode(ImportDeclaration decl) {
4242
exists(decl.getASpecifier().getImportedName())
4343
} or
44-
THtmlAttributeNode(HTML::Attribute attr)
44+
THtmlAttributeNode(HTML::Attribute attr) or
45+
TExceptionalFunctionReturnNode(Function f) or
46+
TExceptionalInvocationReturnNode(InvokeExpr e)
4547

4648
/**
4749
* A node in the data flow graph.
@@ -773,6 +775,50 @@ module DataFlow {
773775
HTML::Attribute getAttribute() { result = attr }
774776
}
775777

778+
/**
779+
* A data flow node representing the exceptions thrown by a function.
780+
*/
781+
class ExceptionalFunctionReturnNode extends DataFlow::Node, TExceptionalFunctionReturnNode {
782+
Function function;
783+
784+
ExceptionalFunctionReturnNode() { this = TExceptionalFunctionReturnNode(function) }
785+
786+
override string toString() { result = "exceptional return of " + function.describe() }
787+
788+
override predicate hasLocationInfo(
789+
string filepath, int startline, int startcolumn, int endline, int endcolumn
790+
) {
791+
function.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
792+
}
793+
794+
/**
795+
* Gets the function corresponding to this exceptional return node.
796+
*/
797+
Function getFunction() { result = function }
798+
}
799+
800+
/**
801+
* A data flow node representing the exceptions thrown by the callee of an invocation.
802+
*/
803+
class ExceptionalInvocationReturnNode extends DataFlow::Node, TExceptionalInvocationReturnNode {
804+
InvokeExpr invoke;
805+
806+
ExceptionalInvocationReturnNode() { this = TExceptionalInvocationReturnNode(invoke) }
807+
808+
override string toString() { result = "exceptional return of " + invoke.toString() }
809+
810+
override predicate hasLocationInfo(
811+
string filepath, int startline, int startcolumn, int endline, int endcolumn
812+
) {
813+
invoke.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
814+
}
815+
816+
/**
817+
* Gets the invocation corresponding to this exceptional return node.
818+
*/
819+
DataFlow::InvokeNode getInvocation() { result = invoke.flow() }
820+
}
821+
776822
/**
777823
* Provides classes representing various kinds of calls.
778824
*
@@ -977,6 +1023,20 @@ module DataFlow {
9771023
result = TDestructuredModuleImportNode(imprt)
9781024
}
9791025

1026+
/**
1027+
* INTERNAL: Use `ExceptionalInvocationReturnNode` instead.
1028+
*/
1029+
predicate exceptionalInvocationReturnNode(DataFlow::Node nd, InvokeExpr invocation) {
1030+
nd = TExceptionalInvocationReturnNode(invocation)
1031+
}
1032+
1033+
/**
1034+
* INTERNAL: Use `ExceptionalFunctionReturnNode` instead.
1035+
*/
1036+
predicate exceptionalFunctionReturnNode(DataFlow::Node nd, Function function) {
1037+
nd = TExceptionalFunctionReturnNode(function)
1038+
}
1039+
9801040
/**
9811041
* A classification of flows that are not modeled, or only modeled incompletely, by
9821042
* `DataFlowNode`:

‎javascript/ql/src/semmle/javascript/dataflow/Nodes.qll‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,13 @@ class InvokeNode extends DataFlow::SourceNode {
141141
* likely to be imprecise or incomplete.
142142
*/
143143
predicate isUncertain() { isImprecise() or isIncomplete() }
144+
145+
/**
146+
* Gets the data flow node representing an exception thrown from this invocation.
147+
*/
148+
DataFlow::ExceptionalInvocationReturnNode getExceptionalReturn() {
149+
DataFlow::exceptionalInvocationReturnNode(result, asExpr())
150+
}
144151
}
145152

146153
/** Auxiliary module used to cache a few related predicates together. */
@@ -356,6 +363,13 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
356363
* To get the data flow node for `this` in an arrow function, consider using `getThisBinder().getReceiver()`.
357364
*/
358365
ThisNode getReceiver() { result.getBinder() = this }
366+
367+
/**
368+
* Gets the data flow node representing an exception thrown from this function.
369+
*/
370+
DataFlow::ExceptionalFunctionReturnNode getExceptionalReturn() {
371+
DataFlow::exceptionalFunctionReturnNode(result, astNode)
372+
}
359373
}
360374

361375
/** A data flow node corresponding to an object literal expression. */

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,30 @@ module TaintTracking {
579579
}
580580
}
581581

582+
/**
583+
* A taint step through an exception constructor, such as `x` to `new Error(x)`.
584+
*/
585+
class ErrorConstructorTaintStep extends AdditionalTaintStep, DataFlow::InvokeNode {
586+
ErrorConstructorTaintStep() {
587+
exists(string name |
588+
this = DataFlow::globalVarRef(name).getAnInvocation()
589+
|
590+
name = "Error" or
591+
name = "EvalError" or
592+
name = "RangeError" or
593+
name = "ReferenceError" or
594+
name = "SyntaxError" or
595+
name = "TypeError" or
596+
name = "URIError"
597+
)
598+
}
599+
600+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
601+
pred = getArgument(0) and
602+
succ = this
603+
}
604+
}
605+
582606
/**
583607
* A conditional checking a tainted string against a regular expression, which is
584608
* considered to be a sanitizer for all configurations.

‎javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ private module NodeTracking {
5555
pred = succ.getAPredecessor()
5656
or
5757
any(DataFlow::AdditionalFlowStep afs).step(pred, succ)
58+
or
59+
localExceptionStep(pred, succ)
5860
}
5961

6062
/**
@@ -153,9 +155,16 @@ private module NodeTracking {
153155
* which is either an argument or a definition captured by the function, flows,
154156
* possibly through callees.
155157
*/
156-
private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk) {
158+
private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node output) {
157159
exists(Function f, DataFlow::ValueNode ret |
158160
ret.asExpr() = f.getAReturnedExpr() and
161+
reachableFromInput(f, output, input, ret, _)
162+
)
163+
or
164+
exists(Function f, DataFlow::Node invk, DataFlow::Node ret |
165+
DataFlow::exceptionalFunctionReturnNode(ret, f) and
166+
DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
167+
calls(invk, f) and
159168
reachableFromInput(f, invk, input, ret, _)
160169
)
161170
}

‎javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,35 @@ predicate localFlowStep(
5151
)
5252
or
5353
configuration.isAdditionalFlowStep(pred, succ, predlbl, succlbl)
54+
or
55+
localExceptionStep(pred, succ) and
56+
predlbl = succlbl
57+
}
58+
59+
/**
60+
* Holds if an exception thrown from `pred` can propagate locally to `succ`.
61+
*/
62+
predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
63+
exists(Expr expr |
64+
expr = any(ThrowStmt throw).getExpr() and
65+
pred = expr.flow()
66+
or
67+
DataFlow::exceptionalInvocationReturnNode(pred, expr)
68+
|
69+
// Propagate out of enclosing function.
70+
not exists(getEnclosingTryStmt(expr.getEnclosingStmt())) and
71+
exists(Function f |
72+
f = expr.getEnclosingFunction() and
73+
DataFlow::exceptionalFunctionReturnNode(succ, f)
74+
)
75+
or
76+
// Propagate to enclosing try/catch.
77+
// To avoid false flow, we only propagate to an unguarded catch clause.
78+
exists(TryStmt try |
79+
try = getEnclosingTryStmt(expr.getEnclosingStmt()) and
80+
DataFlow::parameterNode(succ, try.getCatchClause().getAParameter())
81+
)
82+
)
5483
}
5584

5685
/**
@@ -121,8 +150,23 @@ private module CachedSteps {
121150
predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
122151

123152
/**
124-
* Holds if there is a flow step from `pred` to `succ` through returning
125-
* from a function call or the receiver flowing out of a constructor call.
153+
* Gets the `try` statement containing `stmt` without crossing function boundaries
154+
* or other `try ` statements.
155+
*/
156+
cached
157+
TryStmt getEnclosingTryStmt(Stmt stmt) {
158+
result.getBody() = stmt
159+
or
160+
not stmt instanceof Function and
161+
not stmt = any(TryStmt try).getBody() and
162+
result = getEnclosingTryStmt(stmt.getParentStmt())
163+
}
164+
165+
/**
166+
* Holds if there is a flow step from `pred` to `succ` through:
167+
* - returning a value from a function call, or
168+
* - throwing an exception out of a function call, or
169+
* - the receiver flowing out of a constructor call.
126170
*/
127171
cached
128172
predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
@@ -132,6 +176,12 @@ private module CachedSteps {
132176
succ instanceof DataFlow::NewNode and
133177
DataFlow::thisNode(pred, f)
134178
)
179+
or
180+
exists(InvokeExpr invoke, Function fun |
181+
DataFlow::exceptionalFunctionReturnNode(pred, fun) and
182+
DataFlow::exceptionalInvocationReturnNode(succ, invoke) and
183+
calls(invoke.flow(), fun)
184+
)
135185
}
136186

137187
/**

‎javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,47 @@ module LodashUnderscore {
360360
name = "eachRight" or
361361
name = "first"
362362
}
363+
364+
/**
365+
* A data flow step propagating an exception thrown from a callback to a Lodash/Underscore function.
366+
*/
367+
private class ExceptionStep extends DataFlow::CallNode, DataFlow::AdditionalFlowStep {
368+
ExceptionStep() {
369+
exists(string name |
370+
this = member(name).getACall()
371+
|
372+
// Members ending with By, With, or While indicate that they are a variant of
373+
// another function that takes a callback.
374+
name.matches("%By") or
375+
name.matches("%With") or
376+
name.matches("%While") or
377+
378+
// Other members that don't fit the above pattern.
379+
name = "each" or
380+
name = "eachRight" or
381+
name = "every" or
382+
name = "filter" or
383+
name = "find" or
384+
name = "findLast" or
385+
name = "flatMap" or
386+
name = "flatMapDeep" or
387+
name = "flatMapDepth" or
388+
name = "forEach" or
389+
name = "forEachRight" or
390+
name = "partition" or
391+
name = "reduce" or
392+
name = "reduceRight" or
393+
name = "replace" or
394+
name = "some" or
395+
name = "transform"
396+
)
397+
}
398+
399+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
400+
pred = getAnArgument().(DataFlow::FunctionNode).getExceptionalReturn() and
401+
succ = this.getExceptionalReturn()
402+
}
403+
}
363404
}
364405

365406
/**

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,25 @@
2222
| constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:30:8:30:19 | d_safe.taint |
2323
| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:17:8:17:14 | c.param |
2424
| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:25:8:25:14 | d.param |
25+
| exceptions.js:3:15:3:22 | source() | exceptions.js:5:10:5:10 | e |
26+
| exceptions.js:21:17:21:24 | source() | exceptions.js:23:10:23:10 | e |
27+
| exceptions.js:21:17:21:24 | source() | exceptions.js:24:10:24:21 | e.toString() |
28+
| exceptions.js:21:17:21:24 | source() | exceptions.js:25:10:25:18 | e.message |
29+
| exceptions.js:21:17:21:24 | source() | exceptions.js:26:10:26:19 | e.fileName |
30+
| exceptions.js:48:16:48:23 | source() | exceptions.js:50:10:50:10 | e |
31+
| exceptions.js:59:24:59:31 | source() | exceptions.js:61:12:61:12 | e |
32+
| exceptions.js:88:6:88:13 | source() | exceptions.js:11:10:11:10 | e |
33+
| exceptions.js:88:6:88:13 | source() | exceptions.js:32:10:32:10 | e |
34+
| exceptions.js:88:6:88:13 | source() | exceptions.js:33:10:33:21 | e.toString() |
35+
| exceptions.js:88:6:88:13 | source() | exceptions.js:34:10:34:18 | e.message |
36+
| exceptions.js:88:6:88:13 | source() | exceptions.js:35:10:35:19 | e.fileName |
37+
| exceptions.js:93:11:93:18 | source() | exceptions.js:95:10:95:10 | e |
38+
| exceptions.js:100:13:100:20 | source() | exceptions.js:102:12:102:12 | e |
39+
| exceptions.js:115:21:115:28 | source() | exceptions.js:121:10:121:10 | e |
40+
| exceptions.js:144:9:144:16 | source() | exceptions.js:129:10:129:10 | e |
41+
| exceptions.js:144:9:144:16 | source() | exceptions.js:132:8:132:27 | returnThrownSource() |
42+
| exceptions.js:150:13:150:20 | source() | exceptions.js:153:10:153:10 | e |
43+
| exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e |
2544
| indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
2645
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x |
2746
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y |

0 commit comments

Comments
 (0)