Merge pull request #1892 from asger-semmle/event-handler-sink

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.23/analysis-javascript.md

@@ -18,6 +18,7 @@
 | **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. |
+| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. |
 | Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. |
 
 ## Changes to QL libraries

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

@@ -101,4 +101,18 @@ module CodeInjection {
       )
     }
   }
+
+  /**
+   * An event handler attribute as a code injection sink.
+   */
+  class EventHandlerAttributeSink extends Sink {
+    EventHandlerAttributeSink() {
+      exists(DOM::AttributeDefinition def |
+        def.getName().regexpMatch("(?i)on.+") and
+        this = def.getValueNode() and
+        // JSX event handlers are functions, not strings
+        not def instanceof JSXAttribute
+      )
+    }
+  }
 }

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

@@ -55,6 +55,9 @@ nodes
 | tst.js:23:11:23:27 | document.location |
 | tst.js:23:11:23:32 | documen ... on.hash |
 | tst.js:23:11:23:45 | documen ... ring(1) |
+| tst.js:26:26:26:33 | location |
+| tst.js:26:26:26:40 | location.search |
+| tst.js:26:26:26:53 | locatio ... ring(1) |
 edges
 | angularjs.js:10:22:10:29 | location | angularjs.js:10:22:10:36 | location.search |
 | angularjs.js:13:23:13:30 | location | angularjs.js:13:23:13:37 | location.search |
@@ -93,6 +96,8 @@ edges
 | tst.js:23:11:23:27 | document.location | tst.js:23:11:23:32 | documen ... on.hash |
 | tst.js:23:11:23:32 | documen ... on.hash | tst.js:23:11:23:45 | documen ... ring(1) |
 | tst.js:23:11:23:45 | documen ... ring(1) | tst.js:23:6:23:46 | atob(do ... ing(1)) |
+| tst.js:26:26:26:33 | location | tst.js:26:26:26:40 | location.search |
+| tst.js:26:26:26:40 | location.search | tst.js:26:26:26:53 | locatio ... ring(1) |
 #select
 | angularjs.js:10:22:10:36 | location.search | angularjs.js:10:22:10:29 | location | angularjs.js:10:22:10:36 | location.search | $@ flows to here and is interpreted as code. | angularjs.js:10:22:10:29 | location | User-provided value |
 | angularjs.js:13:23:13:37 | location.search | angularjs.js:13:23:13:30 | location | angularjs.js:13:23:13:37 | location.search | $@ flows to here and is interpreted as code. | angularjs.js:13:23:13:30 | location | User-provided value |
@@ -120,3 +125,4 @@ edges
 | tst.js:17:21:17:42 | documen ... on.hash | tst.js:17:21:17:37 | document.location | tst.js:17:21:17:42 | documen ... on.hash | $@ flows to here and is interpreted as code. | tst.js:17:21:17:37 | document.location | User-provided value |
 | tst.js:20:30:20:51 | documen ... on.hash | tst.js:20:30:20:46 | document.location | tst.js:20:30:20:51 | documen ... on.hash | $@ flows to here and is interpreted as code. | tst.js:20:30:20:46 | document.location | User-provided value |
 | tst.js:23:6:23:46 | atob(do ... ing(1)) | tst.js:23:11:23:27 | document.location | tst.js:23:6:23:46 | atob(do ... ing(1)) | $@ flows to here and is interpreted as code. | tst.js:23:11:23:27 | document.location | User-provided value |
+| tst.js:26:26:26:53 | locatio ... ring(1) | tst.js:26:26:26:33 | location | tst.js:26:26:26:53 | locatio ... ring(1) | $@ flows to here and is interpreted as code. | tst.js:26:26:26:33 | location | User-provided value |

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

@@ -21,3 +21,6 @@ WebAssembly.compileStreaming(document.location.hash);
 
 // NOT OK
 eval(atob(document.location.hash.substring(1)));
+
+// NOT OK
+$('<a>').attr("onclick", location.search.substring(1));