diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll index bb213e227e24..358de5e8900d 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll @@ -145,4 +145,17 @@ module ClientSideUrlRedirect { ) } } + + /** + * A write of an attribute which may execute JavaScript code or + * exfiltrate data to an attacker controlled site. + */ + class AttributeWriteUrlSink extends ScriptUrlSink, DataFlow::ValueNode { + AttributeWriteUrlSink() { + exists(DomPropWriteNode pw | + pw.interpretsValueAsJavaScriptUrl() and + this = DataFlow::valueNode(pw.getRhs()) + ) + } + } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll index d02e85609aa1..f9882537b4e4 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll @@ -90,7 +90,8 @@ class DomMethodCallExpr extends MethodCallExpr { attr = "formaction" or attr = "href" or attr = "src" or - attr = "xlink:href" + attr = "xlink:href" or + attr = "data" | getArgument(argPos - 1).getStringValue().toLowerCase() = attr ) @@ -116,6 +117,17 @@ class DomPropWriteNode extends Assignment { lhs.getPropertyName() = "innerHTML" or lhs.getPropertyName() = "outerHTML" } + + /** + * Holds if the assigned value is interpreted as JavaScript via javascript: protocol. + */ + predicate interpretsValueAsJavaScriptUrl() { + lhs.getPropertyName() = "action" or + lhs.getPropertyName() = "formaction" or + lhs.getPropertyName() = "href" or + lhs.getPropertyName() = "src" or + lhs.getPropertyName() = "data" + } } /** 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 02cf0c015d5c..506dcf1ee0ad 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 @@ -86,6 +86,33 @@ nodes | tst12.js:4:15:4:45 | urlPart ... s.value | | tst12.js:5:23:5:25 | loc | | tst12.js:5:23:5:25 | loc | +| tst13.js:2:9:2:52 | payload | +| tst13.js:2:19:2:35 | document.location | +| tst13.js:2:19:2:35 | document.location | +| tst13.js:2:19:2:42 | documen ... .search | +| tst13.js:2:19:2:52 | documen ... bstr(1) | +| tst13.js:4:15:4:21 | payload | +| tst13.js:4:15:4:21 | payload | +| tst13.js:8:21:8:27 | payload | +| tst13.js:8:21:8:27 | payload | +| tst13.js:12:14:12:20 | payload | +| tst13.js:12:14:12:20 | payload | +| tst13.js:16:17:16:23 | payload | +| tst13.js:16:17:16:23 | payload | +| tst13.js:20:14:20:20 | payload | +| tst13.js:20:14:20:20 | payload | +| tst13.js:24:14:24:20 | payload | +| tst13.js:24:14:24:20 | payload | +| tst13.js:28:21:28:27 | payload | +| tst13.js:28:21:28:27 | payload | +| tst13.js:32:17:32:23 | payload | +| tst13.js:32:17:32:23 | payload | +| tst13.js:36:21:36:27 | payload | +| tst13.js:36:21:36:27 | payload | +| tst13.js:40:15:40:21 | payload | +| tst13.js:40:15:40:21 | payload | +| tst13.js:44:14:44:20 | payload | +| tst13.js:44:14:44:20 | payload | | tst.js:2:19:2:69 | /.*redi ... n.href) | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | @@ -181,6 +208,32 @@ edges | tst12.js:4:15:4:25 | urlParts[0] | tst12.js:4:15:4:45 | urlPart ... s.value | | tst12.js:4:15:4:45 | urlPart ... s.value | tst12.js:4:9:4:45 | loc | | tst12.js:5:23:5:25 | loc | tst12.js:3:20:3:34 | window.location | +| tst13.js:2:9:2:52 | payload | tst13.js:4:15:4:21 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:4:15:4:21 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:8:21:8:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:8:21:8:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:12:14:12:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:12:14:12:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:16:17:16:23 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:16:17:16:23 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:20:14:20:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:20:14:20:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:24:14:24:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:24:14:24:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:28:21:28:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:28:21:28:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:32:17:32:23 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:32:17:32:23 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:36:21:36:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:36:21:36:27 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:40:15:40:21 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:40:15:40:21 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:44:14:44:20 | payload | +| tst13.js:2:9:2:52 | payload | tst13.js:44:14:44:20 | payload | +| tst13.js:2:19:2:35 | document.location | tst13.js:2:19:2:42 | documen ... .search | +| tst13.js:2:19:2:35 | document.location | tst13.js:2:19:2:42 | documen ... .search | +| tst13.js:2:19:2:42 | documen ... .search | tst13.js:2:19:2:52 | documen ... bstr(1) | +| tst13.js:2:19:2:52 | documen ... bstr(1) | tst13.js:2:9:2:52 | payload | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href | @@ -212,5 +265,16 @@ edges | 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 | | tst10.js:14:17:14:56 | 'https: ... .search | tst10.js:14:33:14:49 | document.location | tst10.js:14:17:14:56 | 'https: ... .search | Untrusted URL redirection due to $@. | tst10.js:14:33:14:49 | document.location | user-provided value | | tst12.js:5:23:5:25 | loc | tst12.js:3:20:3:34 | window.location | tst12.js:5:23:5:25 | loc | Untrusted URL redirection due to $@. | tst12.js:3:20:3:34 | window.location | user-provided value | +| tst13.js:4:15:4:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:4:15:4:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:8:21:8:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:8:21:8:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:12:14:12:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:12:14:12:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:16:17:16:23 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:16:17:16:23 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:20:14:20:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:20:14:20:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:24:14:24:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:24:14:24:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:28:21:28:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:28:21:28:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:32:17:32:23 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:32:17:32:23 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:36:21:36:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:36:21:36:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:40:15:40:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:40:15:40:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:44:14:44:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:44:14:44:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value | | tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:50 | document.location | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:6:34:6:50 | document.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js new file mode 100644 index 000000000000..72c9c0b6e727 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js @@ -0,0 +1,46 @@ +function foo() { + var payload = document.location.search.substr(1); + var el = document.createElement("a"); + el.href = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("button"); + el.formaction = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("embed"); + el.src = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("form"); + el.action = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("frame"); + el.src = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("iframe"); + el.src = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("input"); + el.formaction = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("isindex"); + el.action = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("isindex"); + el.formaction = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("object"); + el.data = payload; + document.body.appendChild(el); // NOT OK + + var el = document.createElement("script"); + el.src = payload; + document.body.appendChild(el); // NOT OK +} diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst14.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst14.js new file mode 100644 index 000000000000..f1fb24019483 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst14.js @@ -0,0 +1,10 @@ +function foo() { + var url = document.location.search.substr(1); + var el = document.createElement("script"); + el.src = "https://github.com/" + url; + document.body.appendChild(el); // OK + + if (url.startsWith('https://github.com/')) { + window.location = url; // OK + } +}