From 4c06eb8bfec5b1fbac8f04557cf7c64b1d0da0ca Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 3 Jul 2020 14:26:10 +0100 Subject: [PATCH 1/3] JS: Add test showing FPs --- .../ClientSideUrlRedirect.expected | 65 +++++++++++++++++++ .../ClientSideUrlRedirect/sanitizer.js | 39 +++++++++++ 2 files changed, 104 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected index ba2a4236c78c..542731ea9e48 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected @@ -1,4 +1,31 @@ nodes +| sanitizer.js:2:9:2:25 | url | +| sanitizer.js:2:15:2:25 | window.name | +| sanitizer.js:2:15:2:25 | window.name | +| sanitizer.js:4:27:4:29 | url | +| sanitizer.js:4:27:4:29 | url | +| sanitizer.js:7:27:7:29 | url | +| sanitizer.js:7:27:7:29 | url | +| sanitizer.js:10:27:10:29 | url | +| sanitizer.js:10:27:10:29 | url | +| sanitizer.js:13:27:13:29 | url | +| sanitizer.js:13:27:13:29 | url | +| sanitizer.js:16:27:16:29 | url | +| sanitizer.js:16:27:16:29 | url | +| sanitizer.js:19:27:19:29 | url | +| sanitizer.js:19:27:19:29 | url | +| sanitizer.js:22:27:22:29 | url | +| sanitizer.js:22:27:22:29 | url | +| sanitizer.js:25:27:25:29 | url | +| sanitizer.js:25:27:25:29 | url | +| sanitizer.js:28:27:28:29 | url | +| sanitizer.js:28:27:28:29 | url | +| sanitizer.js:31:27:31:29 | url | +| sanitizer.js:31:27:31:29 | url | +| sanitizer.js:34:27:34:29 | url | +| sanitizer.js:34:27:34:29 | url | +| sanitizer.js:37:27:37:29 | url | +| sanitizer.js:37:27:37:29 | url | | tst2.js:2:7:2:33 | href | | tst2.js:2:7:2:33 | href | | tst2.js:2:14:2:28 | window.location | @@ -80,6 +107,32 @@ nodes | tst.js:6:34:6:50 | document.location | | tst.js:6:34:6:55 | documen ... on.href | edges +| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:22:27:22:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:22:27:22:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:25:27:25:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:25:27:25:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url | +| sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url | +| sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url | +| sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url | | tst2.js:2:7:2:33 | href | tst2.js:4:21:4:24 | href | | tst2.js:2:7:2:33 | href | tst2.js:4:21:4:24 | href | | tst2.js:2:14:2:28 | window.location | tst2.js:2:14:2:33 | window.location.href | @@ -155,6 +208,18 @@ edges | tst.js:6:34:6:50 | document.location | tst.js:6:34:6:55 | documen ... on.href | | tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) | #select +| sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:7:27:7:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:7:27:7:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:10:27:10:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:10:27:10:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:13:27:13:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:13:27:13:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:22:27:22:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:22:27:22:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:25:27:25:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:25:27:25:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:28:27:28:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:28:27:28:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:31:27:31:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:31:27:31:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:34:27:34:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:34:27:34:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | +| sanitizer.js:37:27:37:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:37:27:37:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | 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 | | 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 | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js new file mode 100644 index 000000000000..247f668f9b85 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js @@ -0,0 +1,39 @@ +function f() { + let url = window.name; + if (url.startsWith('https://example.com')) { + window.location = url; // NOT OK - can be example.com.evil.com + } + if (url.startsWith('https://example.com/')) { + window.location = url; // OK - but flagged anyway + } + if (url.startsWith('https://example.com//')) { + window.location = url; // OK - but flagged anyway + } + if (url.startsWith('https://example.com/foo')) { + window.location = url; // OK - but flagged anyway + } + if (url.startsWith('https://')) { + window.location = url; // NOT OK - does not restrict hostname + } + if (url.startsWith('https:/')) { + window.location = url; // NOT OK - does not restrict hostname + } + if (url.startsWith('https:')) { + window.location = url; // NOT OK - does not restrict hostname + } + if (url.startsWith('/')) { + window.location = url; // NOT OK - can be //evil.com + } + if (url.startsWith('//')) { + window.location = url; // NOT OK - can be //evil.com + } + if (url.startsWith('//example.com')) { + window.location = url; // NOT OK - can be //example.com.evil.com + } + if (url.startsWith('//example.com/')) { + window.location = url; // OK - but flagged anyway + } + if (url.endsWith('https://example.com/')) { + window.location = url; // NOT OK - could be evil.com?x=https://example.com/ + } +} From b5104ae42d545bc75927fcb0453a1c85420cd89f Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 3 Jul 2020 14:34:59 +0100 Subject: [PATCH 2/3] JS: Add StartsWith sanitizer --- .../dataflow/ClientSideUrlRedirect.qll | 4 ++++ .../ClientSideUrlRedirectCustomizations.qll | 2 +- .../dataflow/ServerSideUrlRedirect.qll | 3 ++- .../ServerSideUrlRedirectCustomizations.qll | 2 +- .../security/dataflow/UrlConcatenation.qll | 14 +++++++++++++ .../ClientSideUrlRedirect.expected | 20 ------------------- .../ClientSideUrlRedirect/sanitizer.js | 12 +++++++---- 7 files changed, 30 insertions(+), 27 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll index 5102b53f0ce3..f7493f44131d 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll @@ -50,5 +50,9 @@ module ClientSideUrlRedirect { g instanceof DocumentUrl and succ.(DataFlow::PropRead).accesses(pred, "href") } + + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { + guard instanceof HostnameSanitizerGuard + } } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll index 8c2211b086f7..bb213e227e24 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll @@ -6,7 +6,7 @@ import javascript import semmle.javascript.security.dataflow.RemoteFlowSources -import UrlConcatenation +private import UrlConcatenation module ClientSideUrlRedirect { private import Xss::DomBasedXss as DomBasedXss diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll index ac69497eada5..f2d6cfb1652b 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll @@ -34,7 +34,8 @@ module ServerSideUrlRedirect { } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { - guard instanceof LocalUrlSanitizingGuard + guard instanceof LocalUrlSanitizingGuard or + guard instanceof HostnameSanitizerGuard } } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll index 8806e1811e13..3b6e7db73225 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll @@ -6,7 +6,7 @@ import javascript import RemoteFlowSources -import UrlConcatenation +private import UrlConcatenation module ServerSideUrlRedirect { /** diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll index 8d6a30148229..e32826b79be6 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll @@ -96,3 +96,17 @@ predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sin hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1])) ) } + +/** + * A check that sanitizes the hostname of a URL. + */ +class HostnameSanitizerGuard extends TaintTracking::SanitizerGuardNode, StringOps::StartsWith { + HostnameSanitizerGuard() { + hasHostnameSanitizingSubstring(getSubstring()) + } + + override predicate sanitizes(boolean outcome, Expr e) { + outcome = getPolarity() and + e = getBaseString().asExpr() + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected index 542731ea9e48..02cf0c015d5c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected @@ -4,12 +4,6 @@ nodes | sanitizer.js:2:15:2:25 | window.name | | sanitizer.js:4:27:4:29 | url | | sanitizer.js:4:27:4:29 | url | -| sanitizer.js:7:27:7:29 | url | -| sanitizer.js:7:27:7:29 | url | -| sanitizer.js:10:27:10:29 | url | -| sanitizer.js:10:27:10:29 | url | -| sanitizer.js:13:27:13:29 | url | -| sanitizer.js:13:27:13:29 | url | | sanitizer.js:16:27:16:29 | url | | sanitizer.js:16:27:16:29 | url | | sanitizer.js:19:27:19:29 | url | @@ -22,8 +16,6 @@ nodes | sanitizer.js:28:27:28:29 | url | | sanitizer.js:31:27:31:29 | url | | sanitizer.js:31:27:31:29 | url | -| sanitizer.js:34:27:34:29 | url | -| sanitizer.js:34:27:34:29 | url | | sanitizer.js:37:27:37:29 | url | | sanitizer.js:37:27:37:29 | url | | tst2.js:2:7:2:33 | href | @@ -109,12 +101,6 @@ nodes edges | sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url | @@ -127,8 +113,6 @@ edges | sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url | -| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url | | sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url | | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url | @@ -209,16 +193,12 @@ edges | tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) | #select | sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | -| sanitizer.js:7:27:7:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:7:27:7:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | -| sanitizer.js:10:27:10:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:10:27:10:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | -| sanitizer.js:13:27:13:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:13:27:13:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:22:27:22:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:22:27:22:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:25:27:25:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:25:27:25:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:28:27:28:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:28:27:28:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:31:27:31:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:31:27:31:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | -| sanitizer.js:34:27:34:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:34:27:34:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | sanitizer.js:37:27:37:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:37:27:37:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value | | 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 | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js index 247f668f9b85..f8d5b805f4b2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js @@ -4,13 +4,13 @@ function f() { window.location = url; // NOT OK - can be example.com.evil.com } if (url.startsWith('https://example.com/')) { - window.location = url; // OK - but flagged anyway + window.location = url; // OK } if (url.startsWith('https://example.com//')) { - window.location = url; // OK - but flagged anyway + window.location = url; // OK } if (url.startsWith('https://example.com/foo')) { - window.location = url; // OK - but flagged anyway + window.location = url; // OK } if (url.startsWith('https://')) { window.location = url; // NOT OK - does not restrict hostname @@ -31,9 +31,13 @@ function f() { window.location = url; // NOT OK - can be //example.com.evil.com } if (url.startsWith('//example.com/')) { - window.location = url; // OK - but flagged anyway + window.location = url; // OK } if (url.endsWith('https://example.com/')) { window.location = url; // NOT OK - could be evil.com?x=https://example.com/ } + let basedir = whatever() ? 'foo' : 'bar'; + if (url.startsWith('https://example.com/' + basedir)) { + window.location = url; // OK - the whole prefix is not known, but enough to restrict hostname + } } From 9a944625d167f7c985ef557ea819726ff1310c5b Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 6 Jul 2020 15:17:15 +0200 Subject: [PATCH 3/3] autoformat --- .../semmle/javascript/security/dataflow/UrlConcatenation.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll index e32826b79be6..b6ac17828526 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll @@ -101,9 +101,7 @@ predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sin * A check that sanitizes the hostname of a URL. */ class HostnameSanitizerGuard extends TaintTracking::SanitizerGuardNode, StringOps::StartsWith { - HostnameSanitizerGuard() { - hasHostnameSanitizingSubstring(getSubstring()) - } + HostnameSanitizerGuard() { hasHostnameSanitizingSubstring(getSubstring()) } override predicate sanitizes(boolean outcome, Expr e) { outcome = getPolarity() and