Skip to content

Commit 91674f6

Browse files
erik-krogherik-krogh
committed
refactoring to remove duplicated code and simplify the ExceptionXss query
1 parent 853c866 commit 91674f6

File tree

5 files changed

+25
-67
lines changed

5 files changed

+25
-67
lines changed

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,21 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {
245245
ctx.(ConditionalExpr).inNullSensitiveContext()
246246
)
247247
}
248+
249+
/**
250+
* Gets the data-flow node where exceptional data-flow will flow if this expression
251+
* causes an exception to be thrown.
252+
*/
253+
DataFlow::Node getThrowsToNode() {
254+
if exists(this.getEnclosingStmt().getEnclosingTryStmt())
255+
then
256+
result = DataFlow::parameterNode(this
257+
.getEnclosingStmt()
258+
.getEnclosingTryStmt()
259+
.getACatchClause()
260+
.getAParameter())
261+
else result = any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn()
262+
}
248263
}
249264

250265
/**

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1125,6 +1125,9 @@ class MidPathNode extends PathNode, MkMidNode {
11251125
// Skip to the top of big left-leaning string concatenation trees.
11261126
nd = any(AddExpr add).flow() and
11271127
nd = any(AddExpr add).getAnOperand().flow()
1128+
or
1129+
// Skip the exceptional return on functions, as this highlights the entire function.
1130+
nd = any(DataFlow::FunctionNode f).getExceptionalReturn()
11281131
}
11291132
}
11301133

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

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) {
6666
or
6767
DataFlow::exceptionalInvocationReturnNode(pred, expr)
6868
|
69-
// Propagate out of enclosing function.
70-
not exists(expr.getEnclosingStmt().getEnclosingTryStmt()) 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 = expr.getEnclosingStmt().getEnclosingTryStmt() and
80-
DataFlow::parameterNode(succ, try.getCatchClause().getAParameter())
81-
)
69+
succ = expr.getThrowsToNode()
8270
)
8371
}
8472

‎javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll‎

Lines changed: 3 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,26 +13,6 @@ module ExceptionXss {
1313
import Xss::ReflectedXss as ReflectedXSS
1414
import Xss::StoredXss as StoredXss
1515
import Xss as XSS
16-
17-
DataFlow::ExceptionalInvocationReturnNode getCallerExceptionalReturn(Function func) {
18-
exists(DataFlow::InvokeNode call |
19-
not call.isImprecise() and
20-
func = call.getACallee(0) and
21-
result = call.getExceptionalReturn()
22-
)
23-
}
24-
25-
DataFlow::Node getExceptionalSuccessor(DataFlow::Node pred) {
26-
if exists(pred.asExpr().getEnclosingStmt().getEnclosingTryStmt())
27-
then
28-
result = DataFlow::parameterNode(pred
29-
.asExpr()
30-
.getEnclosingStmt()
31-
.getEnclosingTryStmt()
32-
.getACatchClause()
33-
.getAParameter())
34-
else result = getCallerExceptionalReturn(pred.getContainer())
35-
}
3616

3717
/**
3818
* Holds if `node` cannot cause an exception containing sensitive information to be thrown.
@@ -92,13 +72,9 @@ module ExceptionXss {
9272
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl,
9373
DataFlow::FlowLabel outlbl
9474
) {
95-
inlbl instanceof NotYetThrown and
96-
(outlbl.isTaint() or outlbl instanceof NotYetThrown) and
97-
succ = getExceptionalSuccessor(pred) and
98-
(
99-
canThrowSensitiveInformation(pred) or
100-
pred = any(DataFlow::InvokeNode c).getExceptionalReturn()
101-
)
75+
inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
76+
succ = pred.asExpr().getThrowsToNode() and
77+
canThrowSensitiveInformation(pred)
10278
or
10379
// All the usual taint-flow steps apply on data-flow before it has been thrown in an exception.
10480
this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown

‎javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected‎

Lines changed: 3 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ nodes
22
| exception-xss.js:2:9:2:31 | foo |
33
| exception-xss.js:2:15:2:31 | document.location |
44
| exception-xss.js:2:15:2:31 | document.location |
5-
| exception-xss.js:4:20:4:20 | x |
6-
| exception-xss.js:5:14:5:14 | x |
75
| exception-xss.js:9:11:9:13 | foo |
86
| exception-xss.js:10:10:10:10 | e |
97
| exception-xss.js:11:18:11:18 | e |
@@ -28,21 +26,12 @@ nodes
2826
| exception-xss.js:34:10:34:10 | e |
2927
| exception-xss.js:35:18:35:18 | e |
3028
| exception-xss.js:35:18:35:18 | e |
31-
| exception-xss.js:38:16:38:16 | x |
32-
| exception-xss.js:39:3:39:10 | exceptional return of deep2(x) |
33-
| exception-xss.js:39:9:39:9 | x |
34-
| exception-xss.js:41:17:41:17 | x |
35-
| exception-xss.js:42:3:42:10 | exceptional return of inner(x) |
36-
| exception-xss.js:42:9:42:9 | x |
3729
| exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) |
3830
| exception-xss.js:46:8:46:18 | "bar" + foo |
3931
| exception-xss.js:46:16:46:18 | foo |
4032
| exception-xss.js:47:10:47:10 | e |
4133
| exception-xss.js:48:18:48:18 | e |
4234
| exception-xss.js:48:18:48:18 | e |
43-
| exception-xss.js:74:28:74:28 | x |
44-
| exception-xss.js:75:4:75:11 | exceptional return of inner(x) |
45-
| exception-xss.js:75:10:75:10 | x |
4635
| exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) |
4736
| exception-xss.js:81:16:81:18 | foo |
4837
| exception-xss.js:82:10:82:10 | e |
@@ -76,15 +65,11 @@ edges
7665
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:95:12:95:14 | foo |
7766
| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
7867
| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
79-
| exception-xss.js:4:20:4:20 | x | exception-xss.js:5:14:5:14 | x |
80-
| exception-xss.js:5:14:5:14 | x | exception-xss.js:15:3:15:12 | exceptional return of inner(foo) |
81-
| exception-xss.js:5:14:5:14 | x | exception-xss.js:42:3:42:10 | exceptional return of inner(x) |
82-
| exception-xss.js:5:14:5:14 | x | exception-xss.js:75:4:75:11 | exceptional return of inner(x) |
8368
| exception-xss.js:9:11:9:13 | foo | exception-xss.js:10:10:10:10 | e |
8469
| exception-xss.js:10:10:10:10 | e | exception-xss.js:11:18:11:18 | e |
8570
| exception-xss.js:10:10:10:10 | e | exception-xss.js:11:18:11:18 | e |
8671
| exception-xss.js:15:3:15:12 | exceptional return of inner(foo) | exception-xss.js:16:10:16:10 | e |
87-
| exception-xss.js:15:9:15:11 | foo | exception-xss.js:4:20:4:20 | x |
72+
| exception-xss.js:15:9:15:11 | foo | exception-xss.js:15:3:15:12 | exceptional return of inner(foo) |
8873
| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e |
8974
| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e |
9075
| exception-xss.js:21:11:21:13 | foo | exception-xss.js:21:11:21:21 | foo + "bar" |
@@ -99,22 +84,13 @@ edges
9984
| exception-xss.js:33:19:33:21 | foo | exception-xss.js:33:11:33:22 | ["bar", foo] |
10085
| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e |
10186
| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e |
102-
| exception-xss.js:38:16:38:16 | x | exception-xss.js:39:9:39:9 | x |
103-
| exception-xss.js:39:3:39:10 | exceptional return of deep2(x) | exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) |
104-
| exception-xss.js:39:9:39:9 | x | exception-xss.js:41:17:41:17 | x |
105-
| exception-xss.js:41:17:41:17 | x | exception-xss.js:42:9:42:9 | x |
106-
| exception-xss.js:42:3:42:10 | exceptional return of inner(x) | exception-xss.js:39:3:39:10 | exceptional return of deep2(x) |
107-
| exception-xss.js:42:9:42:9 | x | exception-xss.js:4:20:4:20 | x |
10887
| exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) | exception-xss.js:47:10:47:10 | e |
109-
| exception-xss.js:46:8:46:18 | "bar" + foo | exception-xss.js:38:16:38:16 | x |
88+
| exception-xss.js:46:8:46:18 | "bar" + foo | exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) |
11089
| exception-xss.js:46:16:46:18 | foo | exception-xss.js:46:8:46:18 | "bar" + foo |
11190
| exception-xss.js:47:10:47:10 | e | exception-xss.js:48:18:48:18 | e |
11291
| exception-xss.js:47:10:47:10 | e | exception-xss.js:48:18:48:18 | e |
113-
| exception-xss.js:74:28:74:28 | x | exception-xss.js:75:10:75:10 | x |
114-
| exception-xss.js:75:4:75:11 | exceptional return of inner(x) | exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) |
115-
| exception-xss.js:75:10:75:10 | x | exception-xss.js:4:20:4:20 | x |
11692
| exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) | exception-xss.js:82:10:82:10 | e |
117-
| exception-xss.js:81:16:81:18 | foo | exception-xss.js:74:28:74:28 | x |
93+
| exception-xss.js:81:16:81:18 | foo | exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) |
11894
| exception-xss.js:82:10:82:10 | e | exception-xss.js:83:18:83:18 | e |
11995
| exception-xss.js:82:10:82:10 | e | exception-xss.js:83:18:83:18 | e |
12096
| exception-xss.js:89:11:89:13 | foo | exception-xss.js:89:11:89:26 | foo.match(/foo/) |

0 commit comments

Comments
 (0)