Merge pull request #706 from asger-semmle/jquery-location-sink

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.20/analysis-javascript.md

@@ -24,7 +24,7 @@
 
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
-| Client-side cross-site scripting           | More results                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection. |
+| Client-side cross-site scripting           | More true-positive results, fewer false-positive results.                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection, and no longer flags certain safe uses of jQuery. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |

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

@@ -91,7 +91,8 @@ module DomBasedXss {
           isPrefixOfJQueryHtmlString(astNode, prefix) and
           strval = prefix.asExpr().getStringValue() and
           not strval.regexpMatch("\\s*<.*")
-        )
+        ) and
+        not isDocumentURL(astNode)
       )
       or
       // call to an Angular method that interprets its argument as HTML

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

@@ -214,6 +214,10 @@ nodes
 | tst.js:256:7:256:17 | window.name |
 | tst.js:257:7:257:10 | name |
 | tst.js:261:11:261:21 | window.name |
+| tst.js:272:9:272:32 | loc3 |
+| tst.js:272:16:272:32 | document.location |
+| tst.js:275:7:275:10 | loc3 |
+| tst.js:277:22:277:29 | location |
 | winjs.js:2:7:2:53 | tainted |
 | winjs.js:2:17:2:33 | document.location |
 | winjs.js:2:17:2:40 | documen ... .search |
@@ -396,6 +400,8 @@ edges
 | tst.js:238:23:238:29 | tainted | tst.js:228:32:228:49 | prevProps.tainted4 |
 | tst.js:244:39:244:55 | props.propTainted | tst.js:248:60:248:82 | this.st ... Tainted |
 | tst.js:252:23:252:29 | tainted | tst.js:244:39:244:55 | props.propTainted |
+| tst.js:272:9:272:32 | loc3 | tst.js:275:7:275:10 | loc3 |
+| tst.js:272:16:272:32 | document.location | tst.js:272:9:272:32 | loc3 |
 | winjs.js:2:7:2:53 | tainted | winjs.js:3:43:3:49 | tainted |
 | winjs.js:2:7:2:53 | tainted | winjs.js:4:43:4:49 | tainted |
 | winjs.js:2:17:2:33 | document.location | winjs.js:2:17:2:40 | documen ... .search |

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

@@ -171,6 +171,10 @@ nodes
 | tst.js:256:7:256:17 | window.name |
 | tst.js:257:7:257:10 | name |
 | tst.js:261:11:261:21 | window.name |
+| tst.js:272:9:272:32 | loc3 |
+| tst.js:272:16:272:32 | document.location |
+| tst.js:275:7:275:10 | loc3 |
+| tst.js:277:22:277:29 | location |
 | winjs.js:2:7:2:53 | tainted |
 | winjs.js:2:17:2:33 | document.location |
 | winjs.js:2:17:2:40 | documen ... .search |
@@ -307,6 +311,8 @@ edges
 | tst.js:238:23:238:29 | tainted | tst.js:228:32:228:49 | prevProps.tainted4 |
 | tst.js:244:39:244:55 | props.propTainted | tst.js:248:60:248:82 | this.st ... Tainted |
 | tst.js:252:23:252:29 | tainted | tst.js:244:39:244:55 | props.propTainted |
+| tst.js:272:9:272:32 | loc3 | tst.js:275:7:275:10 | loc3 |
+| tst.js:272:16:272:32 | document.location | tst.js:272:9:272:32 | loc3 |
 | winjs.js:2:7:2:53 | tainted | winjs.js:3:43:3:49 | tainted |
 | winjs.js:2:7:2:53 | tainted | winjs.js:4:43:4:49 | tainted |
 | winjs.js:2:17:2:33 | document.location | winjs.js:2:17:2:40 | documen ... .search |
@@ -378,5 +384,7 @@ edges
 | tst.js:256:7:256:17 | window.name | tst.js:256:7:256:17 | window.name | tst.js:256:7:256:17 | window.name | Cross-site scripting vulnerability due to $@. | tst.js:256:7:256:17 | window.name | user-provided value |
 | tst.js:257:7:257:10 | name | tst.js:257:7:257:10 | name | tst.js:257:7:257:10 | name | Cross-site scripting vulnerability due to $@. | tst.js:257:7:257:10 | name | user-provided value |
 | tst.js:261:11:261:21 | window.name | tst.js:261:11:261:21 | window.name | tst.js:261:11:261:21 | window.name | Cross-site scripting vulnerability due to $@. | tst.js:261:11:261:21 | window.name | user-provided value |
+| tst.js:275:7:275:10 | loc3 | tst.js:272:16:272:32 | document.location | tst.js:275:7:275:10 | loc3 | Cross-site scripting vulnerability due to $@. | tst.js:272:16:272:32 | document.location | user-provided value |
+| tst.js:277:22:277:29 | location | tst.js:277:22:277:29 | location | tst.js:277:22:277:29 | location | Cross-site scripting vulnerability due to $@. | tst.js:277:22:277:29 | location | user-provided value |
 | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value |
 | winjs.js:4:43:4:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:4:43:4:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value |

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

@@ -262,3 +262,17 @@ function windowNameAssigned() {
         $(name); // OK
     }
 }
+
+function jqueryLocation() {
+    $(location); // OK
+    $(window.location); // OK
+    $(document.location); // OK
+    var loc1 = location;
+    var loc2 = window.location;
+    var loc3 = document.location;
+    $(loc1); // OK
+    $(loc2); // OK
+    $(loc3); // OK - but still flagged
+
+    $("body").append(location); // NOT OK
+}