Merge pull request #3578 from erik-krogh/HtmlGuard

Approved by asgerf

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -438,7 +438,10 @@ private predicate barrierGuardBlocksNode(BarrierGuardNode guard, DataFlow::Node
   barrierGuardIsRelevant(guard) and
   exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
     nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
-    guard.getEnclosingExpr() = cond.getTest() and
+    (
+      guard.getEnclosingExpr() = cond.getTest() or
+      guard = cond.getTest().flow().getImmediatePredecessor+()
+    ) and
     outcome = cond.getOutcome() and
     barrierGuardBlocksAccessPath(guard, outcome, p, label) and
     cond.dominates(bb)

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

@@ -73,6 +73,66 @@ module Shared {
       e = this.getBaseString().getEnclosingExpr() and outcome = this.getPolarity().booleanNot()
     }
   }
+
+  /**
+   * A sanitizer guard that checks for the existence of HTML chars in a string.
+   * E.g. `/["'&<>]/.exec(str)`.
+   */
+  class ContainsHTMLGuard extends SanitizerGuard, DataFlow::MethodCallNode {
+    DataFlow::RegExpCreationNode regExp;
+
+    ContainsHTMLGuard() {
+      this.getMethodName() = ["test", "exec"] and
+      this.getReceiver().getALocalSource() = regExp and
+      regExp.getRoot() instanceof RegExpCharacterClass and
+      forall(string s | s = ["\"", "&", "<", ">"] | regExp.getRoot().getAMatchedString() = s)
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = false and e = this.getArgument(0).asExpr()
+    }
+  }
+
+  /**
+   * Holds if `str` is used in a switch-case that has cases matching HTML escaping.
+   */
+  private predicate isUsedInHTMLEscapingSwitch(Expr str) {
+    exists(SwitchStmt switch |
+      // "\"".charCodeAt(0) == 34, "&".charCodeAt(0) == 38, "<".charCodeAt(0) == 60
+      forall(int c | c = [34, 38, 60] | c = switch.getACase().getExpr().getIntValue()) and
+      exists(DataFlow::MethodCallNode mcn | mcn.getMethodName() = "charCodeAt" |
+        mcn.flowsToExpr(switch.getExpr()) and
+        str = mcn.getReceiver().asExpr()
+      )
+      or
+      forall(string c | c = ["\"", "&", "<"] | c = switch.getACase().getExpr().getStringValue()) and
+      (
+        exists(DataFlow::MethodCallNode mcn | mcn.getMethodName() = "charAt" |
+          mcn.flowsToExpr(switch.getExpr()) and
+          str = mcn.getReceiver().asExpr()
+        )
+        or
+        exists(DataFlow::PropRead read | exists(read.getPropertyNameExpr()) |
+          read.flowsToExpr(switch.getExpr()) and
+          str = read.getBase().asExpr()
+        )
+      )
+    )
+  }
+
+  /**
+   * Gets an Ssa variable that is used in a sanitizing switch statement.
+   * The `pragma[noinline]` is to avoid materializing a cartesian product.
+   */
+  pragma[noinline]
+  private SsaVariable getAPathEscapedInSwitch() { isUsedInHTMLEscapingSwitch(result.getAUse()) }
+
+  /**
+   * An expression that is sanitized by a switch-case.
+   */
+  class IsEscapedInSwitchSanitizer extends Sanitizer {
+    IsEscapedInSwitchSanitizer() { this.asExpr() = getAPathEscapedInSwitch().getAUse() }
+  }
 }
 
 /** Provides classes and predicates for the DOM-based XSS query. */
@@ -328,6 +388,8 @@ module DomBasedXss {
 
   private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
 
+  private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
+
   private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
 
   /**
@@ -359,6 +421,8 @@ module DomBasedXss {
       )
     )
   }
+
+  private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
 }
 
 /** Provides classes and predicates for the reflected XSS query. */
@@ -462,7 +526,11 @@ module ReflectedXss {
 
   private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
 
+  private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
+
   private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
+
+  private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
 }
 
 /** Provides classes and predicates for the stored XSS query. */
@@ -495,7 +563,11 @@ module StoredXss {
 
   private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
 
+  private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
+
   private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
+
+  private class ContainsHTMLGuard extends SanitizerGuard, Shared::ContainsHTMLGuard { }
 }
 
 /** Provides classes and predicates for the XSS through DOM query. */

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

@@ -19,6 +19,12 @@ nodes
 | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
 | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
 | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
+| ReflectedXssGood3.js:135:9:135:27 | url |
+| ReflectedXssGood3.js:135:15:135:27 | req.params.id |
+| ReflectedXssGood3.js:135:15:135:27 | req.params.id |
+| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
+| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
+| ReflectedXssGood3.js:139:24:139:26 | url |
 | etherpad.js:9:5:9:53 | response |
 | etherpad.js:9:16:9:30 | req.query.jsonp |
 | etherpad.js:9:16:9:30 | req.query.jsonp |
@@ -105,6 +111,11 @@ edges
 | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
 | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
 | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
+| ReflectedXssGood3.js:135:9:135:27 | url | ReflectedXssGood3.js:139:24:139:26 | url |
+| ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:135:9:135:27 | url |
+| ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:135:9:135:27 | url |
+| ReflectedXssGood3.js:139:24:139:26 | url | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
+| ReflectedXssGood3.js:139:24:139:26 | url | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) |
 | etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
 | etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
 | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
@@ -166,6 +177,7 @@ edges
 | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
 | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
 | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
+| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | ReflectedXssGood3.js:135:15:135:27 | req.params.id | ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | Cross-site scripting vulnerability due to $@. | ReflectedXssGood3.js:135:15:135:27 | req.params.id | user-provided value |
 | etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
 | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
 | formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |

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

@@ -49,3 +49,39 @@ app.get('/echo', function(req, res) {
 	res.setHeader('Content-Length', msg.length);
 	res.end(msg);
 });
+
+app.get('/user/:id', function(req, res) {
+  const url = req.params.id;
+  if (!/["'&<>]/.exec(url)) {
+    res.send(url); // OK
+  }
+});
+
+function escapeHtml1 (str) {
+  if (!/["'&<>]/.exec(str)) {
+      return str;
+  }
+}
+
+app.get('/user/:id', function(req, res) {
+  const url = req.params.id;
+
+  res.send(escapeHtml1(url)); // OK
+});
+
+const matchHtmlRegExp = /["'&<>]/;
+function escapeHtml2 (string) {
+    const str = '' + string;
+    const match = matchHtmlRegExp.exec(str);
+
+    if (!match) {
+        return str;
+    }
+}
+
+app.get('/user/:id', function(req, res) {
+  const url = req.params.id;
+
+  res.send(escapeHtml2(url)); // OK
+});
+

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

@@ -0,0 +1,142 @@
+var express = require('express');
+
+var app = express();
+
+function escapeHtml1(string) {
+  var str = "" + string;
+  let escape;
+  let html = '';
+  let lastIndex = 0;
+
+  for (let index = 0; index < str.length; index++) {
+    switch (str.charCodeAt(index)) {
+      case 34: // "
+        escape = '&quot;';
+        break;
+      case 38: // &
+        escape = '&amp;';
+        break;
+      case 39: // '
+        escape = '&#39;';
+        break;
+      case 60: // <
+        escape = '&lt;';
+        break;
+      case 62: // >
+        escape = '&gt;';
+        break;
+      default:
+        continue;
+    }
+
+    if (lastIndex !== index) {
+      html += str.substring(lastIndex, index);
+    }
+
+    lastIndex = index + 1;
+    html += escape;
+  }
+
+  return lastIndex !== index
+    ? html + str.substring(lastIndex, index)
+    : html;
+}
+
+function escapeHtml2(s) {
+  var buf = "";
+  while (i < s.length) {
+    var ch = s[i++];
+    switch (ch) {
+      case '&':
+        buf += '&amp;';
+        break;
+      case '<':
+        buf += '&lt;';
+        break;
+      case '\"':
+        buf += '&quot;';
+        break;
+      default:
+        buf += ch;
+        break;
+    }
+  }
+  return buf;
+}
+
+
+function escapeHtml3(value) {
+  var i = 0;
+  var XMLChars = {
+    AMP: 38, // "&"
+    QUOT: 34, // "\""
+    LT: 60, // "<"
+    GT: 62, // ">"
+  };
+
+  var parts = [value.substring(0, i)];
+  while (i < length) {
+    switch (ch) {
+      case XMLChars.AMP:
+        parts.push('&amp;');
+        break;
+      case XMLChars.QUOT:
+        parts.push('&quot;');
+        break;
+      case XMLChars.LT:
+        parts.push('&lt;');
+        break;
+      case XMLChars.GT:
+        parts.push('&gt;');
+        break;
+    }
+    ++i;
+    var j = i;
+    while (i < length) {
+      ch = value.charCodeAt(i);
+      if (ch === XMLChars.AMP ||
+        ch === XMLChars.QUOT || ch === XMLChars.LT ||
+        ch === XMLChars.GT) {
+        break;
+      }
+      i++;
+    }
+    if (j < i) {
+      parts.push(value.substring(j, i));
+    }
+  }
+  return parts.join('');
+}
+
+
+function escapeHtml4(s) {
+  var buf = "";
+  while (i < s.length) {
+    var ch = s.chatAt(i++);
+    switch (ch) {
+      case '&':
+        buf += '&amp;';
+        break;
+      case '<':
+        buf += '&lt;';
+        break;
+      case '\"':
+        buf += '&quot;';
+        break;
+      default:
+        buf += ch;
+        break;
+    }
+  }
+  return buf;
+}
+
+app.get('/user/:id', function (req, res) {
+  const url = req.params.id;
+
+  res.send(escapeHtml1(url)); // OK
+  res.send(escapeHtml2(url)); // OK
+  res.send(escapeHtml3(url)); // OK - but FP
+  res.send(escapeHtml4(url)); // OK
+});
+

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

@@ -3,6 +3,7 @@
 | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
 | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
 | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
+| ReflectedXssGood3.js:139:12:139:27 | escapeHtml3(url) | Cross-site scripting vulnerability due to $@. | ReflectedXssGood3.js:135:15:135:27 | req.params.id | user-provided value |
 | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
 | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |