Merge pull request #2420 from erik-krogh/safeStringSink

Approved by asgerf

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -4,6 +4,7 @@
 
 * Support for the following frameworks and libraries has been improved:
   - [react](https://www.npmjs.com/package/react)
+  - [Handlebars](https://www.npmjs.com/package/handlebars)
 
 ## New queries
 

javascript/ql/src/javascript.qll

@@ -76,6 +76,7 @@ import semmle.javascript.frameworks.Electron
 import semmle.javascript.frameworks.Files
 import semmle.javascript.frameworks.Firebase
 import semmle.javascript.frameworks.jQuery
+import semmle.javascript.frameworks.Handlebars
 import semmle.javascript.frameworks.LodashUnderscore
 import semmle.javascript.frameworks.Logging
 import semmle.javascript.frameworks.HttpFrameworks

javascript/ql/src/semmle/javascript/frameworks/Handlebars.qll

@@ -0,0 +1,29 @@
+/**
+ * Provides classes for working with Handlebars code.
+ */
+
+import javascript
+
+module Handlebars {
+  /**
+   * A reference to the Handlebars library. 
+   */
+  class Handlebars extends DataFlow::SourceNode {
+    Handlebars() {
+      this.accessesGlobal("handlebars")
+      or
+      this.accessesGlobal("Handlebars")
+      or
+      this = DataFlow::moduleImport("handlebars")
+      or
+      this.hasUnderlyingType("Handlebars")
+    }
+  }
+
+  /**
+   * A new instantiation of a Handlebars.SafeString.
+   */
+  class SafeString extends DataFlow::NewNode {
+    SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
+  }
+}

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

@@ -95,6 +95,8 @@ module DomBasedXss {
         mcn.getMethodName() = m and
         this = mcn.getArgument(1)
       )
+      or
+      this = any(Handlebars::SafeString s).getAnArgument()
     }
   }
 

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

@@ -315,6 +315,9 @@ nodes
 | tst.js:285:59:285:65 | tainted |
 | tst.js:285:59:285:65 | tainted |
 | tst.js:285:59:285:65 | tainted |
+| tst.js:297:35:297:42 | location |
+| tst.js:297:35:297:42 | location |
+| tst.js:297:35:297:42 | location |
 | v-html.vue:2:8:2:23 | v-html=tainted |
 | v-html.vue:2:8:2:23 | v-html=tainted |
 | v-html.vue:6:42:6:58 | document.location |
@@ -604,6 +607,7 @@ edges
 | tst.js:282:19:282:29 | window.name | tst.js:282:9:282:29 | tainted |
 | tst.js:282:19:282:29 | window.name | tst.js:282:9:282:29 | tainted |
 | tst.js:285:59:285:65 | tainted | tst.js:285:59:285:65 | tainted |
+| tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location |
 | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted |
 | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted |
 | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted |
@@ -693,6 +697,7 @@ edges
 | tst.js:285:59:285:65 | tainted | tst.js:282:9:282:29 | tainted | tst.js:285:59:285:65 | tainted | Cross-site scripting vulnerability due to $@. | tst.js:282:9:282:29 | tainted | user-provided value |
 | tst.js:285:59:285:65 | tainted | tst.js:282:19:282:29 | window.name | tst.js:285:59:285:65 | tainted | Cross-site scripting vulnerability due to $@. | tst.js:282:19:282:29 | window.name | user-provided value |
 | tst.js:285:59:285:65 | tainted | tst.js:285:59:285:65 | tainted | tst.js:285:59:285:65 | tainted | Cross-site scripting vulnerability due to $@. | tst.js:285:59:285:65 | tainted | user-provided value |
+| tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location | Cross-site scripting vulnerability due to $@. | tst.js:297:35:297:42 | location | user-provided value |
 | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.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

@@ -292,3 +292,7 @@ function flowThroughPropertyNames() {
     for (var p in obj)
       $(p); // OK
 }
+
+function handlebarsSafeString() {
+	return new Handlebars.SafeString(location); // NOT OK!	
+}