Merge pull request #1835 from xiemaisi/js/dom-fixes

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.23/analysis-javascript.md

@@ -0,0 +1,17 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+## New queries
+
+| **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**          | **Change**                                                                |
+|--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. |
+
+## Changes to QL libraries

javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll

@@ -71,17 +71,26 @@ class DomMethodCallExpr extends MethodCallExpr {
       or
       name = "writeln"
       or
-      name = "insertAdjacentHTML" and argPos = 0
+      name = "insertAdjacentHTML" and argPos = 1
       or
-      name = "insertAdjacentElement" and argPos = 0
+      name = "insertAdjacentElement" and argPos = 1
       or
       name = "insertBefore" and argPos = 0
       or
       name = "createElement" and argPos = 0
       or
       name = "appendChild" and argPos = 0
       or
-      name = "setAttribute" and argPos = 0
+      (
+        name = "setAttribute" and argPos = 1
+        or
+        name = "setAttributeNS" and argPos = 2
+      ) and
+      // restrict to potentially dangerous attributes
+      exists(string attr |
+        attr = "action" or attr = "formaction" or attr = "href" or attr = "src" |
+        getArgument(argPos-1).getStringValue().toLowerCase() = attr
+      )
     )
   }
 }

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

@@ -49,6 +49,15 @@ nodes
 | translate.js:7:42:7:47 | target |
 | translate.js:7:42:7:60 | target.substring(1) |
 | translate.js:9:27:9:50 | searchP ... 'term') |
+| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
+| tst3.js:2:23:2:74 | decodeU ... str(1)) |
+| tst3.js:2:42:2:56 | window.location |
+| tst3.js:2:42:2:63 | window. ... .search |
+| tst3.js:2:42:2:73 | window. ... bstr(1) |
+| tst3.js:4:25:4:28 | data |
+| tst3.js:4:25:4:32 | data.src |
+| tst3.js:5:26:5:29 | data |
+| tst3.js:5:26:5:31 | data.p |
 | tst.js:2:7:2:39 | target |
 | tst.js:2:16:2:32 | document.location |
 | tst.js:2:16:2:39 | documen ... .search |
@@ -215,6 +224,14 @@ edges
 | translate.js:6:16:6:39 | documen ... .search | translate.js:6:7:6:39 | target |
 | translate.js:7:42:7:47 | target | translate.js:7:42:7:60 | target.substring(1) |
 | translate.js:7:42:7:60 | target.substring(1) | translate.js:9:27:9:50 | searchP ... 'term') |
+| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:4:25:4:28 | data |
+| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:5:26:5:29 | data |
+| tst3.js:2:23:2:74 | decodeU ... str(1)) | tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
+| tst3.js:2:42:2:56 | window.location | tst3.js:2:42:2:63 | window. ... .search |
+| tst3.js:2:42:2:63 | window. ... .search | tst3.js:2:42:2:73 | window. ... bstr(1) |
+| tst3.js:2:42:2:73 | window. ... bstr(1) | tst3.js:2:23:2:74 | decodeU ... str(1)) |
+| tst3.js:4:25:4:28 | data | tst3.js:4:25:4:32 | data.src |
+| tst3.js:5:26:5:29 | data | tst3.js:5:26:5:31 | data.p |
 | tst.js:2:7:2:39 | target | tst.js:5:18:5:23 | target |
 | tst.js:2:7:2:39 | target | tst.js:12:28:12:33 | target |
 | tst.js:2:7:2:39 | target | tst.js:23:42:23:47 | target |
@@ -342,6 +359,8 @@ edges
 | string-manipulations.js:9:16:9:58 | String. ... n.href) | string-manipulations.js:9:36:9:52 | document.location | string-manipulations.js:9:16:9:58 | String. ... n.href) | Cross-site scripting vulnerability due to $@. | string-manipulations.js:9:36:9:52 | document.location | user-provided value |
 | string-manipulations.js:10:16:10:45 | String( ... n.href) | string-manipulations.js:10:23:10:39 | document.location | string-manipulations.js:10:16:10:45 | String( ... n.href) | Cross-site scripting vulnerability due to $@. | string-manipulations.js:10:23:10:39 | document.location | user-provided value |
 | translate.js:9:27:9:50 | searchP ... 'term') | translate.js:6:16:6:32 | document.location | translate.js:9:27:9:50 | searchP ... 'term') | Cross-site scripting vulnerability due to $@. | translate.js:6:16:6:32 | document.location | user-provided value |
+| tst3.js:4:25:4:32 | data.src | tst3.js:2:42:2:56 | window.location | tst3.js:4:25:4:32 | data.src | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:56 | window.location | user-provided value |
+| tst3.js:5:26:5:31 | data.p | tst3.js:2:42:2:56 | window.location | tst3.js:5:26:5:31 | data.p | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:56 | window.location | user-provided value |
 | tst.js:5:18:5:23 | target | tst.js:2:16:2:32 | document.location | tst.js:5:18:5:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:2:16:2:32 | document.location | user-provided value |
 | tst.js:8:18:8:126 | "<OPTIO ... PTION>" | tst.js:8:37:8:53 | document.location | tst.js:8:18:8:126 | "<OPTIO ... PTION>" | Cross-site scripting vulnerability due to $@. | tst.js:8:37:8:53 | document.location | user-provided value |
 | tst.js:12:5:12:42 | '<div s ...  'px">' | tst.js:2:16:2:32 | document.location | tst.js:12:5:12:42 | '<div s ...  'px">' | Cross-site scripting vulnerability due to $@. | tst.js:2:16:2:32 | document.location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/tst3.js

@@ -0,0 +1,9 @@
+var foo = document.getElementById("foo");
+var data = JSON.parse(decodeURIComponent(window.location.search.substr(1)));
+
+foo.setAttribute("src", data.src); // NOT OK
+foo.setAttribute("HREF", data.p);  // NOT OK
+foo.setAttribute("width", data.w); // OK
+
+for (var p in data)
+  foo.setAttribute(p, data[p]); // not flagged since attribute name is unknown