JS: Add ajv error as source of ExceptionXss

asgerf · asgerf · commit 7afa755597c2 · 2021-03-02T12:39:04.000Z
diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp b/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp
@@ -5,8 +5,8 @@
 
 <overview>
 <p>
-Directly writing exceptions to a webpage without sanitization allows for a cross-site scripting
-vulnerability if the value of the exception can be influenced by a user.
+Directly writing error messages to a webpage without sanitization allows for a cross-site scripting
+vulnerability if parts of the error message can be influenced by a user.
 </p>
 </overview>
 
@@ -27,6 +27,19 @@ leaving the website vulnerable to cross-site scripting.
 <sample src="examples/ExceptionXss.js" />
 </example>
 
+<example>
+<p>
+This second example shows an input being validated using the JSON schema validator <code>ajv</code>,
+and in case of an error, the error message is sent directly back in the response.
+</p>
+<sample src="examples/ExceptionXssAjv.js" />
+<p>
+This is unsafe, because the error message can contain parts of the input.
+For example, the input <code>{'&lt;img src=x onerror=alert(1)&gt;': 'foo'}</code> will generate the error
+<code>data/&lt;img src=x onerror=alert(1)&gt; should be number</code>, causing reflected XSS.
+</p>
+</example>
+
 <references>
 <li>
 OWASP:
diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql
@@ -15,8 +15,18 @@ import javascript
 import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss
 import DataFlow::PathGraph
 
+/**
+ * Gets a description of the source.
+ */
+string getSourceDescription(DataFlow::Node source) {
+  result = source.(ErrorSource).getDescription()
+  or
+  not source instanceof ErrorSource and
+  result = "Exception text"
+}
+
 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   "$@ is reinterpreted as HTML without escaping meta-characters.", source.getNode(),
-  "Exception text"
+  getSourceDescription(source.getNode())
diff --git a/javascript/ql/src/Security/CWE-079/examples/ExceptionXssAjv.js b/javascript/ql/src/Security/CWE-079/examples/ExceptionXssAjv.js
@@ -0,0 +1,13 @@
+import express from 'express';
+import Ajv from 'ajv';
+
+let app = express();
+let ajv = new Ajv();
+
+ajv.addSchema({type: 'object', additionalProperties: {type: 'number'}}, 'pollData');
+
+app.post('/polldata', (req, res) => {
+    if (!ajv.validate('pollData', req.body)) {
+        res.send(ajv.errorsText());
+    }
+});
diff --git a/javascript/ql/src/semmle/javascript/JsonSchema.qll b/javascript/ql/src/semmle/javascript/JsonSchema.qll
@@ -38,6 +38,31 @@ module JsonSchema {
         or
         result = ref().getMember(chainedMethod()).getReturn()
       }
+
+      /**
+       * Gets an API node for a function produced by `new Ajv().compile()` or similar. 
+       *
+       * Note that this does not include the instance method `new Ajv().validate` as its
+       * signature is different.
+       */
+      API::Node getAValidationFunction() {
+        result = ref().getMember(["compile", "getSchema"]).getReturn()
+        or
+        result = ref().getMember("compileAsync").getPromised()
+      }
+
+      /**
+       * Gets an API node that refers to an error produced by this Ajv instance.
+       */
+      API::Node getAValidationError() {
+        exists(API::Node base |
+          base = [ref(), getAValidationFunction()]
+        |
+          result = base.getMember("errors")
+          or
+          result = base.getMember("errorsText").getReturn()
+        )
+      }
     }
 
     /** A call to the `validate` method of `ajv`. */
@@ -48,10 +73,7 @@ module JsonSchema {
       AjvValidationCall() {
         this = instance.ref().getMember("validate").getACall() and argIndex = 1
         or
-        this = instance.ref().getMember(["compile", "getSchema"]).getReturn().getACall() and
-        argIndex = 0
-        or
-        this = instance.ref().getMember("compileAsync").getPromised().getACall() and argIndex = 0
+        this = instance.getAValidationFunction().getACall() and argIndex = 0
       }
 
       override DataFlow::Node getInput() { result = getArgument(argIndex) }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll
@@ -130,6 +130,37 @@ module ExceptionXss {
     result = getCallbackErrorParam(pred)
   }
 
+  /**
+   * A source of error values that is likely to contain unencoded user input.
+   */
+  abstract class ErrorSource extends DataFlow::Node {
+    /**
+     * Gets a human-readable description of what type of error this refers to.
+     *
+     * The result should be captialized and usable in the context of a noun.
+     */
+    abstract string getDescription();
+  }
+
+  /**
+   * An error produced by validating using `ajv`.
+   *
+   * Such an error can contain property names from the input if the
+   * underlying schema uses `additionalProperties` or `propertyPatterns`.
+   *
+   * For example, an input of form `{"<img src=x onerror=alert(1)>": 45}` might produce the error
+   * `data/<img src=x onerror=alert(1)> should be string`.
+   */
+  private class JsonSchemaValidationError extends ErrorSource {
+    JsonSchemaValidationError() {
+      this = any(JsonSchema::Ajv::Instance i).getAValidationError().getAnImmediateUse()
+    }
+
+    override string getDescription() {
+      result = "JSON schema validation error"
+    }
+  }
+
   /**
    * A taint-tracking configuration for reasoning about XSS with possible exceptional flow.
    * Flow labels are used to ensure that we only report taint-flow that has been thrown in
@@ -140,6 +171,9 @@ module ExceptionXss {
 
     override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
       source instanceof Xss::Shared::Source and label instanceof NotYetThrown
+      or
+      source instanceof ErrorSource and
+      label.isTaint()
     }
 
     override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ExceptionXss.expected
@@ -1,4 +1,7 @@
 nodes
+| ajv.js:11:18:11:33 | ajv.errorsText() |
+| ajv.js:11:18:11:33 | ajv.errorsText() |
+| ajv.js:11:18:11:33 | ajv.errorsText() |
 | exception-xss.js:2:6:2:28 | foo |
 | exception-xss.js:2:12:2:28 | document.location |
 | exception-xss.js:2:12:2:28 | document.location |
@@ -87,6 +90,7 @@ nodes
 | exception-xss.js:182:19:182:23 | error |
 | exception-xss.js:182:19:182:23 | error |
 edges
+| ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() |
 | exception-xss.js:2:6:2:28 | foo | exception-xss.js:9:11:9:13 | foo |
 | exception-xss.js:2:6:2:28 | foo | exception-xss.js:15:9:15:11 | foo |
 | exception-xss.js:2:6:2:28 | foo | exception-xss.js:21:11:21:13 | foo |
@@ -169,6 +173,7 @@ edges
 | exception-xss.js:180:26:180:30 | error | exception-xss.js:182:19:182:23 | error |
 | exception-xss.js:180:26:180:30 | error | exception-xss.js:182:19:182:23 | error |
 #select
+| ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() | ajv.js:11:18:11:33 | ajv.errorsText() | $@ is reinterpreted as HTML without escaping meta-characters. | ajv.js:11:18:11:33 | ajv.errorsText() | JSON schema validation error |
 | exception-xss.js:11:18:11:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:11:18:11:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text |
 | exception-xss.js:17:18:17:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:17:18:17:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text |
 | exception-xss.js:23:18:23:18 | e | exception-xss.js:2:12:2:28 | document.location | exception-xss.js:23:18:23:18 | e | $@ is reinterpreted as HTML without escaping meta-characters. | exception-xss.js:2:12:2:28 | document.location | Exception text |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ajv.js b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/ajv.js
@@ -0,0 +1,13 @@
+import express from 'express';
+import Ajv from 'ajv';
+
+let app = express();
+let ajv = new Ajv();
+
+ajv.addSchema({type: 'object', additionalProperties: {type: 'number'}}, 'pollData');
+
+app.post('/polldata', (req, res) => {
+    if (!ajv.validate('pollData', req.body)) {
+        res.send(ajv.errorsText()); // NOT OK
+    }
+});
