Merge pull request github#12424 from asgerf/js/html-sanitizer-for-sql

asgerf · web-flow · commit 5461f94c6ce5 · 2023-03-13T11:36:19.000+01:00
JS: Add html sanitizers as a taint step in a few queries
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectQuery.qll
@@ -48,6 +48,12 @@ class Configuration extends TaintTracking::Configuration {
     f instanceof DocumentUrl and
     g instanceof DocumentUrl and
     succ.(DataFlow::PropRead).accesses(pred, "href")
+    or
+    exists(HtmlSanitizerCall call |
+      pred = call.getInput() and
+      succ = call and
+      f = g
+    )
   }
 
   override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll
@@ -72,6 +72,11 @@ module RequestForgery {
       succ = url and
       pred = url.getArgument(0)
     )
+    or
+    exists(HtmlSanitizerCall call |
+      pred = call.getInput() and
+      succ = call
+    )
   }
 
   private class SinkFromModel extends Sink {
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ServerSideUrlRedirectQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ServerSideUrlRedirectQuery.qll
@@ -35,6 +35,13 @@ class Configuration extends TaintTracking::Configuration {
     guard instanceof LocalUrlSanitizingGuard or
     guard instanceof HostnameSanitizerGuard
   }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(HtmlSanitizerCall call |
+      pred = call.getInput() and
+      succ = call
+    )
+  }
 }
 
 /**
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/SqlInjectionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/SqlInjectionQuery.qll
@@ -30,5 +30,10 @@ class Configuration extends TaintTracking::Configuration {
       pred = filter.getInput() and
       succ = filter.getOutput()
     )
+    or
+    exists(HtmlSanitizerCall call |
+      pred = call.getInput() and
+      succ = call
+    )
   }
 }
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -841,6 +841,12 @@ module TaintedPath {
       dst = call and
       srclabel = dstlabel
     )
+    or
+    exists(HtmlSanitizerCall call |
+      src = call.getInput() and
+      dst = call and
+      srclabel = dstlabel
+    )
   }
 
   /**
diff --git a/javascript/ql/src/change-notes/2023-03-07-html-sanitizer-for-sql.md b/javascript/ql/src/change-notes/2023-03-07-html-sanitizer-for-sql.md
@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* The following queries now recognize HTML sanitizers as propagating taint: `js/sql-injection`,
+  `js/path-injection`, `js/server-side-unvalidated-url-redirection`, `js/client-side-unvalidated-url-redirection`,
+  and `js/request-forgery`.
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected
@@ -1,3 +1,4 @@
+| html-sanitizer.js:15:5:17:5 | connect ... K\\n    ) |
 | json-schema-validator.js:27:13:27:27 | doc.find(query) |
 | json-schema-validator.js:30:13:30:27 | doc.find(query) |
 | json-schema-validator.js:33:13:33:27 | doc.find(query) |
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected
@@ -50,6 +50,14 @@ nodes
 | graphql.js:120:38:120:48 | `foo ${id}` |
 | graphql.js:120:38:120:48 | `foo ${id}` |
 | graphql.js:120:45:120:46 | id |
+| html-sanitizer.js:13:39:13:44 | param1 |
+| html-sanitizer.js:13:39:13:44 | param1 |
+| html-sanitizer.js:14:5:14:24 | param1 |
+| html-sanitizer.js:14:14:14:24 | xss(param1) |
+| html-sanitizer.js:14:18:14:23 | param1 |
+| html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 |
+| html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 |
+| html-sanitizer.js:16:54:16:59 | param1 |
 | json-schema-validator.js:25:15:25:48 | query |
 | json-schema-validator.js:25:23:25:48 | JSON.pa ... y.data) |
 | json-schema-validator.js:25:34:25:47 | req.query.data |
@@ -466,6 +474,13 @@ edges
 | graphql.js:119:16:119:28 | req.params.id | graphql.js:119:11:119:28 | id |
 | graphql.js:120:45:120:46 | id | graphql.js:120:38:120:48 | `foo ${id}` |
 | graphql.js:120:45:120:46 | id | graphql.js:120:38:120:48 | `foo ${id}` |
+| html-sanitizer.js:13:39:13:44 | param1 | html-sanitizer.js:14:18:14:23 | param1 |
+| html-sanitizer.js:13:39:13:44 | param1 | html-sanitizer.js:14:18:14:23 | param1 |
+| html-sanitizer.js:14:5:14:24 | param1 | html-sanitizer.js:16:54:16:59 | param1 |
+| html-sanitizer.js:14:14:14:24 | xss(param1) | html-sanitizer.js:14:5:14:24 | param1 |
+| html-sanitizer.js:14:18:14:23 | param1 | html-sanitizer.js:14:14:14:24 | xss(param1) |
+| html-sanitizer.js:16:54:16:59 | param1 | html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 |
+| html-sanitizer.js:16:54:16:59 | param1 | html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 |
 | json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:33:22:33:26 | query |
 | json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:33:22:33:26 | query |
 | json-schema-validator.js:25:15:25:48 | query | json-schema-validator.js:35:18:35:22 | query |
@@ -924,6 +939,7 @@ edges
 | graphql.js:75:46:75:64 | "{ foo" + id + " }" | graphql.js:74:14:74:25 | req.query.id | graphql.js:75:46:75:64 | "{ foo" + id + " }" | This query depends on a $@. | graphql.js:74:14:74:25 | req.query.id | user-provided value |
 | graphql.js:84:14:90:8 | `{\\n     ...      }` | graphql.js:74:14:74:25 | req.query.id | graphql.js:84:14:90:8 | `{\\n     ...      }` | This query depends on a $@. | graphql.js:74:14:74:25 | req.query.id | user-provided value |
 | graphql.js:120:38:120:48 | `foo ${id}` | graphql.js:119:16:119:28 | req.params.id | graphql.js:120:38:120:48 | `foo ${id}` | This query depends on a $@. | graphql.js:119:16:119:28 | req.params.id | user-provided value |
+| html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 | html-sanitizer.js:13:39:13:44 | param1 | html-sanitizer.js:16:9:16:59 | `SELECT ...  param1 | This query depends on a $@. | html-sanitizer.js:13:39:13:44 | param1 | user-provided value |
 | json-schema-validator.js:33:22:33:26 | query | json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:33:22:33:26 | query | This query depends on a $@. | json-schema-validator.js:25:34:25:47 | req.query.data | user-provided value |
 | json-schema-validator.js:35:18:35:22 | query | json-schema-validator.js:25:34:25:47 | req.query.data | json-schema-validator.js:35:18:35:22 | query | This query depends on a $@. | json-schema-validator.js:25:34:25:47 | req.query.data | user-provided value |
 | json-schema-validator.js:55:22:55:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:55:22:55:26 | query | This query depends on a $@. | json-schema-validator.js:50:34:50:47 | req.query.data | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/html-sanitizer.js b/javascript/ql/test/query-tests/Security/CWE-089/untyped/html-sanitizer.js
@@ -0,0 +1,18 @@
+const route = require('koa-route');
+const Koa = require('koa');
+const mysql = require('mysql2');
+const app = new Koa();
+const xss = require('xss');
+
+const connection = mysql.createConnection({
+    host: 'localhost',
+    user: 'root',
+    database: 'test'
+});
+
+app.use(route.get('/test1', (context, param1) => {
+    param1 = xss(param1)
+    connection.query(
+        `SELECT * FROM \`table\` WHERE \`name\` =` + param1, // NOT OK
+    );
+}));

Original file line number	Diff line number	Diff line change
`@@ -72,6 +72,11 @@ module RequestForgery {`
`72`	`72`	`succ = url and`
`73`	`73`	`pred = url.getArgument(0)`
`74`	`74`	`)`
	`75`	`+ or`
	`76`	`+ exists(HtmlSanitizerCall call \|`
	`77`	`+ pred = call.getInput() and`
	`78`	`+ succ = call`
	`79`	`+ )`
`75`	`80`	`}`
`76`	`81`
`77`	`82`	`private class SinkFromModel extends Sink {`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,13 @@ class Configuration extends TaintTracking::Configuration {`
`35`	`35`	`guard instanceof LocalUrlSanitizingGuard or`
`36`	`36`	`guard instanceof HostnameSanitizerGuard`
`37`	`37`	`}`
	`38`	`+`
	`39`	`+ override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {`
	`40`	`+ exists(HtmlSanitizerCall call \|`
	`41`	`+ pred = call.getInput() and`
	`42`	`+ succ = call`
	`43`	`+ )`
	`44`	`+ }`
`38`	`45`	`}`
`39`	`46`
`40`	`47`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -30,5 +30,10 @@ class Configuration extends TaintTracking::Configuration {`
`30`	`30`	`pred = filter.getInput() and`
`31`	`31`	`succ = filter.getOutput()`
`32`	`32`	`)`
	`33`	`+ or`
	`34`	`+ exists(HtmlSanitizerCall call \|`
	`35`	`+ pred = call.getInput() and`
	`36`	`+ succ = call`
	`37`	`+ )`
`33`	`38`	`}`
`34`	`39`	`}`
Original file line number	Diff line number	Diff line change
`@@ -841,6 +841,12 @@ module TaintedPath {`
`841`	`841`	`dst = call and`
`842`	`842`	`srclabel = dstlabel`
`843`	`843`	`)`
	`844`	`+ or`
	`845`	`+ exists(HtmlSanitizerCall call \|`
	`846`	`+ src = call.getInput() and`
	`847`	`+ dst = call and`
	`848`	`+ srclabel = dstlabel`
	`849`	`+ )`
`844`	`850`	`}`
`845`	`851`
`846`	`852`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+\| html-sanitizer.js:15:5:17:5 \| connect ... K\\n ) \|`
`1`	`2`	`\| json-schema-validator.js:27:13:27:27 \| doc.find(query) \|`
`2`	`3`	`\| json-schema-validator.js:30:13:30:27 \| doc.find(query) \|`
`3`	`4`	`\| json-schema-validator.js:33:13:33:27 \| doc.find(query) \|`