Merge pull request github#5098 from erik-krogh/xml2js

Approved by asgerf

Author: codeql-ci

javascript/change-notes/2021-02-08-xml-parser-taint.md

@@ -0,0 +1,8 @@
+lgtm,codescanning
+* The security queries now track taint through XML parsers. 
+  Affected packages are
+    [xml2js](https://www.npmjs.com/package/xml2js),
+    [sax](https://www.npmjs.com/package/sax),
+    [xml-js](https://www.npmjs.com/package/xml-js),
+    [htmlparser2](https://www.npmjs.com/package/htmlparser2), and
+    [node-expat](https://www.npmjs.com/package/node-expat)

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

@@ -25,6 +25,9 @@ module XML {
 
     /** Holds if this call to the XML parser resolves entities of the given `kind`. */
     abstract predicate resolvesEntities(EntityKind kind);
+
+    /** Gets a reference to a value resulting from parsing the XML. */
+    js::DataFlow::Node getAResult() { none() }
   }
 
   /**
@@ -98,10 +101,11 @@ module XML {
    * An invocation of `expat.Parser.parse` or `expat.Parser.write`.
    */
   class ExpatParserInvocation extends ParserInvocation {
+    js::DataFlow::NewNode parser;
+
     ExpatParserInvocation() {
-      exists(string m | m = "parse" or m = "write" |
-        this = moduleMethodCall("node-expat", "Parser", m)
-      )
+      parser = js::DataFlow::moduleMember("node-expat", "Parser").getAnInstantiation() and
+      this = parser.getAMemberCall(["parse", "write"]).asExpr()
     }
 
     override js::Expr getSourceArgument() { result = getArgument(0) }
@@ -110,6 +114,10 @@ module XML {
       // only internal entities are resolved by default
       kind = InternalEntity()
     }
+
+    override js::DataFlow::Node getAResult() {
+      result = parser.getAMemberCall("on").getABoundCallbackParameter(1, _)
+    }
   }
 
   /**
@@ -160,4 +168,122 @@ module XML {
 
     override predicate resolvesEntities(XML::EntityKind kind) { kind = InternalEntity() }
   }
+
+  /**
+   * An invocation of `xml2js`.
+   */
+  private class Xml2JSInvocation extends XML::ParserInvocation {
+    js::DataFlow::CallNode call;
+
+    Xml2JSInvocation() {
+      exists(js::API::Node imp | imp = js::API::moduleImport("xml2js") |
+        call = [imp, imp.getMember("Parser").getInstance()].getMember("parseString").getACall() and
+        this = call.asExpr()
+      )
+    }
+
+    override js::Expr getSourceArgument() { result = getArgument(0) }
+
+    override predicate resolvesEntities(XML::EntityKind kind) {
+      // sax-js (the parser used) does not expand entities.
+      none()
+    }
+
+    override js::DataFlow::Node getAResult() {
+      result = call.getABoundCallbackParameter(call.getNumArgument() - 1, 1)
+    }
+  }
+
+  /**
+   * An invocation of `sax`.
+   */
+  private class SaxInvocation extends XML::ParserInvocation {
+    js::DataFlow::InvokeNode parser;
+
+    SaxInvocation() {
+      exists(js::API::Node imp | imp = js::API::moduleImport("sax") |
+        parser = imp.getMember("parser").getACall()
+        or
+        parser = imp.getMember("SAXParser").getAnInstantiation()
+      ) and
+      this = parser.getAMemberCall("write").asExpr()
+    }
+
+    override js::Expr getSourceArgument() { result = getArgument(0) }
+
+    override predicate resolvesEntities(XML::EntityKind kind) {
+      // sax-js does not expand entities.
+      none()
+    }
+
+    override js::DataFlow::Node getAResult() {
+      result =
+        parser
+            .getAPropertyWrite(any(string s | s.matches("on%")))
+            .getRhs()
+            .getAFunctionValue()
+            .getAParameter()
+    }
+  }
+
+  /**
+   * An invocation of `xml-js`.
+   */
+  private class XmlJSInvocation extends XML::ParserInvocation {
+    XmlJSInvocation() {
+      this =
+        js::DataFlow::moduleMember("xml-js", ["xml2json", "xml2js", "json2xml", "js2xml"])
+            .getACall()
+            .asExpr()
+    }
+
+    override js::Expr getSourceArgument() { result = getArgument(0) }
+
+    override predicate resolvesEntities(XML::EntityKind kind) {
+      // xml-js does not expand custom entities.
+      none()
+    }
+
+    override js::DataFlow::Node getAResult() { result.asExpr() = this }
+  }
+
+  /**
+   * An invocation of `htmlparser2`.
+   */
+  private class HtmlParser2Invocation extends XML::ParserInvocation {
+    js::DataFlow::NewNode parser;
+
+    HtmlParser2Invocation() {
+      parser = js::DataFlow::moduleMember("htmlparser2", "Parser").getAnInstantiation() and
+      this = parser.getAMemberCall("write").asExpr()
+    }
+
+    override js::Expr getSourceArgument() { result = getArgument(0) }
+
+    override predicate resolvesEntities(XML::EntityKind kind) {
+      // htmlparser2 does not expand entities.
+      none()
+    }
+
+    override js::DataFlow::Node getAResult() {
+      result =
+        parser
+            .getArgument(0)
+            .getALocalSource()
+            .getAPropertySource()
+            .getAFunctionValue()
+            .getAParameter()
+    }
+  }
+
+  private class XMLParserTaintStep extends js::TaintTracking::AdditionalTaintStep {
+    XML::ParserInvocation parser;
+
+    XMLParserTaintStep() { this.asExpr() = parser }
+
+    override predicate step(js::DataFlow::Node pred, js::DataFlow::Node succ) {
+      pred.asExpr() = parser.getSourceArgument() and
+      succ = parser.getAResult()
+    }
+  }
 }

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -145,3 +145,8 @@ typeInferenceMismatch
 | tst.js:2:13:2:20 | source() | tst.js:45:10:45:24 | x.map(x2 => x2) |
 | tst.js:2:13:2:20 | source() | tst.js:47:10:47:30 | Buffer. ...  'hex') |
 | tst.js:2:13:2:20 | source() | tst.js:48:10:48:22 | new Buffer(x) |
+| xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text |
+| xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result |
+| xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr |
+| xml.js:26:27:26:34 | source() | xml.js:26:10:26:39 | convert ... (), {}) |
+| xml.js:34:18:34:25 | source() | xml.js:31:18:31:21 | name |

javascript/ql/test/library-tests/TaintTracking/xml.js

@@ -0,0 +1,37 @@
+(function () {
+    var Parser = require("node-expat").Parser
+    var parser = new Parser();
+
+    parser.write(source());
+
+    parser.on("text", text => {
+        sink(text); // NOT OK
+    });
+
+    var parseString = require('xml2js').parseString;
+    parseString(source(), function (err, result) {
+        sink(result); // NOT OK
+    });
+
+    var sax = require("sax");
+    var parser = sax.parser(strict);
+    
+    parser.onattribute = function (attr) {
+        sink(attr); // NOT OK
+    };
+    
+    parser.write(source()).close();
+
+    var convert = require('xml-js');
+    sink(convert.xml2json(source(), {})); // NOT OK
+
+    const htmlparser2 = require("htmlparser2");
+    const parser = new htmlparser2.Parser({
+        onopentag(name, attributes) {
+            sink(name) // NOT OK
+        }
+    });
+    parser.write(source());
+    parser.end();
+
+})();