Skip to content

Commit 455dbcc

Browse files
author
Max Schaefer
committed
JavaScript: Fix definitions of SourcePathNode and SinkPathNode.
Their charpreds previously only ensured that they were on a path from a source to a sink, not that they actually were the source and sink, respectively. See two commits further for a test case.
1 parent c674f54 commit 455dbcc

File tree

3 files changed

+15
-6
lines changed

3 files changed

+15
-6
lines changed

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -819,7 +819,7 @@ private predicate reachableFromSource(
819819
isSource(nd, cfg, lbl) and
820820
not cfg.isBarrier(nd) and
821821
not cfg.isLabeledBarrier(nd, lbl) and
822-
summary = MkPathSummary(false, false, lbl, lbl)
822+
summary = PathSummary::level(lbl)
823823
)
824824
or
825825
exists(DataFlow::Node pred, PathSummary oldSummary, PathSummary newSummary |
@@ -952,14 +952,19 @@ class PathNode extends TPathNode {
952952
* A path node corresponding to a flow source.
953953
*/
954954
class SourcePathNode extends PathNode {
955-
SourcePathNode() { isSource(nd, cfg, _) }
955+
SourcePathNode() {
956+
exists(FlowLabel lbl |
957+
summary = PathSummary::level(lbl) and
958+
isSource(nd, cfg, lbl)
959+
)
960+
}
956961
}
957962

958963
/**
959964
* A path node corresponding to a flow sink.
960965
*/
961966
class SinkPathNode extends PathNode {
962-
SinkPathNode() { isSink(nd, cfg, _) }
967+
SinkPathNode() { isSink(nd, cfg, summary.getEndLabel()) }
963968
}
964969

965970
/**

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,13 @@ module PathSummary {
406406
/**
407407
* Gets a summary describing a path without any calls or returns.
408408
*/
409-
PathSummary level() { exists(FlowLabel lbl | result = MkPathSummary(false, false, lbl, lbl)) }
409+
PathSummary level() { result = level(_) }
410+
411+
/**
412+
* Gets a summary describing a path without any calls or returns, transforming `lbl` into
413+
* itself.
414+
*/
415+
PathSummary level(FlowLabel lbl) { result = MkPathSummary(false, false, lbl, lbl) }
410416

411417
/**
412418
* Gets a summary describing a path with one or more calls, but no returns.

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,12 @@ edges
7878
| tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) |
7979
#select
8080
| tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
81-
| tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
8281
| tst6.js:4:21:4:28 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:4:21:4:28 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
8382
| tst6.js:6:17:6:24 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:6:17:6:24 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
8483
| tst6.js:8:21:8:56 | $locati ... + "foo" | tst6.js:8:21:8:48 | $locati ... irect') | tst6.js:8:21:8:56 | $locati ... + "foo" | Untrusted URL redirection due to $@. | tst6.js:8:21:8:48 | $locati ... irect') | user-provided value |
8584
| tst7.js:2:12:2:35 | documen ... .search | tst7.js:2:12:2:28 | document.location | tst7.js:2:12:2:35 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:2:12:2:28 | document.location | user-provided value |
8685
| tst7.js:5:27:5:50 | documen ... .search | tst7.js:5:27:5:43 | document.location | tst7.js:5:27:5:50 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:5:27:5:43 | document.location | user-provided value |
8786
| tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
88-
| tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
8987
| tst10.js:5:17:5:46 | '/' + d ... .search | tst10.js:5:23:5:39 | document.location | tst10.js:5:17:5:46 | '/' + d ... .search | Untrusted URL redirection due to $@. | tst10.js:5:23:5:39 | document.location | user-provided value |
9088
| tst10.js:8:17:8:47 | '//' + ... .search | tst10.js:8:24:8:40 | document.location | tst10.js:8:17:8:47 | '//' + ... .search | Untrusted URL redirection due to $@. | tst10.js:8:24:8:40 | document.location | user-provided value |
9189
| tst10.js:11:17:11:50 | '//foo' ... .search | tst10.js:11:27:11:43 | document.location | tst10.js:11:17:11:50 | '//foo' ... .search | Untrusted URL redirection due to $@. | tst10.js:11:27:11:43 | document.location | user-provided value |

0 commit comments

Comments
 (0)