adding entity check for xml parser + xml sanitizer

Author: adl-trey

player/service/package-lock.json

@@ -43,7 +43,8 @@
     "mysql": "^2.18.1",
     "node-stream-zip": "^1.13.6",
     "uuid": "^9.0.1",
-    "wait-port": "^0.2.9"
+    "wait-port": "^0.2.9",
+    "xml-sanitizer": "^2.0.1"
   },
   "devDependencies": {
     "chai": "^4",

player/service/package.json

@@ -18,6 +18,7 @@
 const Boom = require("@hapi/boom");
 const Requirements = require("@cmi5/requirements");
 const axios = require("axios").default;
+const xmlSanitizer = require("xml-sanitizer");
 
 let user = process.env.LRS_USERNAME;
 let pass = process.env.LRS_PASSWORD;
@@ -133,5 +134,29 @@ module.exports = {
         ];
 
         return concurrencyPaths.includes(resourcePath);
+    },
+
+    /**
+     * Checks the given XML content, returning false if the content
+     * appears potentially malicious.
+     * @param {String|Buffer} xmlContent 
+     */
+    isPotentiallyMaliciousXML: async(xmlContent) => {
+
+        if (xmlContent.includes("<!ENTITY")) {
+            console.error(">> Invalid XML -- Attempts an Entity Tag: ", "\n--------------\n", xmlContent);
+            return true;
+        }
+        
+        return false;
+    },
+
+    /**
+     * Checks the given XML content, returning false if the content
+     * appears potentially malicious.
+     * @param {String|Buffer} xmlContent 
+     */
+    sanitizeXML: async(xmlContent) => {
+        return xmlSanitizer(xmlContent);
     }
 };

player/service/plugins/routes/lib/helpers.js

@@ -28,6 +28,7 @@ const uuidv4 = require("uuid").v4;
 const Helpers = require("../lib/helpers");
 const Registration = require("../lib/registration");
 const Session = require("../lib/session");
+const helpers = require("../lib/helpers");
 const readFile = util.promisify(fs.readFile);
 const copyFile = util.promisify(fs.copyFile);
 const mkdir = util.promisify(fs.mkdir);
@@ -349,7 +350,7 @@ module.exports = {
                             //
                             isZip = contentType === "application/zip" || contentType === "application/x-zip-compressed";
 
-                        let courseStructureData,
+                        let courseStructureDataRaw,
                             zip;
 
                         if (!isZip && contentType !== "text/xml") {
@@ -365,7 +366,7 @@ module.exports = {
                             }
 
                             try {
-                                courseStructureData = await zip.entryData("cmi5.xml");
+                                courseStructureDataRaw = await zip.entryData("cmi5.xml");
                             }
                             catch (ex) {
                                 if (ex.message === "Bad archive") {
@@ -377,13 +378,18 @@ module.exports = {
                         }
                         else {
                             try {
-                                courseStructureData = await readFile(req.payload.path);
+                                courseStructureDataRaw = await readFile(req.payload.path);
                             }
                             catch (ex) {
                                 throw Boom.internal(`Failed to read structure file: ${ex}`);
                             }
                         }
 
+                        let courseStructureData = helpers.sanitizeXML(courseStructureDataRaw);
+                        if (courseStructureData != undefined && helpers.isPotentiallyMaliciousXML(courseStructureData)) {
+                            throw Boom.internal(`Invalid XML data provided: ${ex}`);
+                        }
+
                         let courseStructureDocument;
 
                         try {

player/service/plugins/routes/v1/courses.js

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="ISO-8859-1"?>
+<!DOCTYPE foo [
+<!ENTITY xxe SYSTEM "file:///some/bad/place" >]>
+<username>&xxe;</username>
+</xml>

player/service/tests/files/entity.xml

@@ -34,19 +34,12 @@ describe("Basic LRS Communications", async () => {
         };
 
         let path = "/statements?statementId=" + statement.id;
-        let pathWithoutID = "/statements";
 
         console.log("Sending statement ...");
 
         let res = await helpers.sendDocumentToLRS(path, "PUT", statement);
-        let ids = res.responseBody;
 
-        delete res.res;
-        
-        console.log("Received response:", res);
-        
         chai.expect(res.status).to.be.lessThan(400);
         chai.expect(res.status).to.be.greaterThanOrEqual(200);
-        chai.expect(ids).to.have.length(1);
     });
 });

player/service/tests/lrs.spec.js

@@ -0,0 +1,25 @@
+const path = require("path");
+require("dotenv").config({
+    path: path.join(__dirname, "../../.env")
+});
+
+const fs = require("fs");
+
+const helpers = require("../plugins/routes/lib/helpers");
+const chai = require("chai");
+
+describe("Libxmljs Usage", async () => {
+
+    /**
+     * https://www.stackhawk.com/blog/nodejs-xml-external-entities-xxe-guide-examples-and-prevention/
+     */
+    const PATH_XML_DOC_ENTITY_USAGE = path.join(__dirname, "files/entity.xml");
+
+    it ("Rejects anything using an ENTITY tag", async() => {
+       
+        let entityBody = fs.readFileSync(PATH_XML_DOC_ENTITY_USAGE);
+        let suspicious = await helpers.isPotentiallyMaliciousXML(entityBody);
+
+        chai.expect(suspicious).to.be.equal(true, "The provided XML should have thrown a validity issue for its use of an <!ENTITY tag");
+    });
+});