sanitize non-strings from unsafe-html-construction

2026-04-28 10:15:14 +02:00 · 2022-06-27 13:53:44 +02:00
parent c72377cf2c
commit 34e7589844
5 changed files with 50 additions and 17 deletions
--- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
@@ -1126,6 +1126,21 @@ module TaintTracking {
    )
  }

+  /**
+   * Holds if `test` is of the form `typeof x === "something"`, preventing `x` from being a string in some cases.
+   */
+  predicate isStringTypeGuard(EqualityTest test, Expr operand, boolean polarity) {
+    exists(TypeofTag tag | TaintTracking::isTypeofGuard(test, operand, tag) |
+      // typeof x === "string" sanitizes `x` when it evaluates to false
+      tag = "string" and
+      polarity = test.getPolarity().booleanNot()
+      or
+      // typeof x === "object" sanitizes `x` when it evaluates to true
+      tag != "string" and
+      polarity = test.getPolarity()
+    )
+  }
+
  /** Holds if `guard` is a test that checks if `operand` is a number. */
  predicate isNumberGuard(DataFlow::Node guard, Expr operand, boolean polarity) {
    exists(DataFlow::CallNode isNaN |
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -180,4 +180,18 @@ module UnsafeHtmlConstruction {

    override string describe() { result = "Markdown rendering" }
  }
+
+  /** A test of form `typeof x === "something"`, preventing `x` from being a string in some cases. */
+  class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
+    override EqualityTest astNode;
+    Expr operand;
+    boolean polarity;
+
+    TypeTestGuard() { TaintTracking::isStringTypeGuard(astNode, operand, polarity) }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = operand
+    }
+  }
 }
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
@@ -4,7 +4,6 @@
 */

 import javascript
-private import semmle.javascript.dataflow.InferredTypes
 private import XssThroughDomCustomizations::XssThroughDom
 private import semmle.javascript.security.dataflow.DomBasedXssCustomizations
 private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQuery
@@ -52,27 +51,13 @@ class Configuration extends TaintTracking::Configuration {
  }
 }

-/**
- * A test of form `typeof x === "something"`, preventing `x` from being a string in some cases.
- *
- * This sanitizer helps prune infeasible paths in type-overloaded functions.
- */
+/** A test of form `typeof x === "something"`, preventing `x` from being a string in some cases. */
 class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
  override EqualityTest astNode;
  Expr operand;
  boolean polarity;

-  TypeTestGuard() {
-    exists(TypeofTag tag | TaintTracking::isTypeofGuard(astNode, operand, tag) |
-      // typeof x === "string" sanitizes `x` when it evaluates to false
-      tag = "string" and
-      polarity = astNode.getPolarity().booleanNot()
-      or
-      // typeof x === "object" sanitizes `x` when it evaluates to true
-      tag != "string" and
-      polarity = astNode.getPolarity()
-    )
-  }
+  TypeTestGuard() { TaintTracking::isStringTypeGuard(astNode, operand, polarity) }

  override predicate sanitizes(boolean outcome, Expr e) {
    polarity = outcome and
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
@@ -46,6 +46,10 @@ nodes
 | main.js:66:35:66:41 | attrVal |
 | main.js:67:63:67:69 | attrVal |
 | main.js:67:63:67:69 | attrVal |
+| main.js:79:34:79:36 | val |
+| main.js:79:34:79:36 | val |
+| main.js:81:35:81:37 | val |
+| main.js:81:35:81:37 | val |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:2:29:2:29 | s |
@@ -107,6 +111,10 @@ edges
 | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
 | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
 | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
+| main.js:79:34:79:36 | val | main.js:81:35:81:37 | val |
+| main.js:79:34:79:36 | val | main.js:81:35:81:37 | val |
+| main.js:79:34:79:36 | val | main.js:81:35:81:37 | val |
+| main.js:79:34:79:36 | val | main.js:81:35:81:37 | val |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -132,5 +140,6 @@ edges
 | main.js:47:65:47:73 | this.step | main.js:52:41:52:41 | s | main.js:47:65:47:73 | this.step | $@ based on $@ might later cause $@. | main.js:47:65:47:73 | this.step | HTML construction | main.js:52:41:52:41 | s | library input | main.js:47:54:47:85 | "<span> ... /span>" | cross-site scripting |
 | main.js:62:19:62:31 | settings.name | main.js:56:28:56:34 | options | main.js:62:19:62:31 | settings.name | $@ based on $@ might later cause $@. | main.js:62:19:62:31 | settings.name | HTML construction | main.js:56:28:56:34 | options | library input | main.js:62:11:62:40 | "<b>" + ...  "</b>" | cross-site scripting |
 | main.js:67:63:67:69 | attrVal | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | $@ based on $@ might later cause $@. | main.js:67:63:67:69 | attrVal | HTML construction | main.js:66:35:66:41 | attrVal | library input | main.js:67:47:67:78 | "<img a ...  "\\"/>" | cross-site scripting |
+| main.js:81:35:81:37 | val | main.js:79:34:79:36 | val | main.js:81:35:81:37 | val | $@ based on $@ might later cause $@. | main.js:81:35:81:37 | val | HTML construction | main.js:79:34:79:36 | val | library input | main.js:81:24:81:49 | "<span> ... /span>" | cross-site scripting |
 | typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
 | typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
@@ -75,3 +75,13 @@ module.exports.intentionalTemplate = function (obj) {
    const html = "<span>" + obj.spanTemplate + "</span>"; // OK
    document.querySelector("#template").innerHTML = html;
 }
+
+module.exports.types = function (val) {
+    if (typeof val === "string") {
+        $("#foo").html("<span>" + val + "</span>"); // NOT OK
+    } else if (typeof val === "number") {
+        $("#foo").html("<span>" + val + "</span>"); // OK
+    } else if (typeof val === "boolean") {
+        $("#foo").html("<span>" + val + "</span>"); // OK
+    }
+}