Can a "new DOMParser.parseFromString" be safer than "createElement"?

javascript, xss

Solution

Effectively, your current code is not safe. `innerHTML` doesn't run scripts in created `<script>` elements, but it does run event handler content attributes.

function createDOM(str) {
  document.createElement("div").innerHTML = str;
}
createDOM('<img src="//" onerror="console.log(\'You are pwned!\')" />');

function createDOM(str) {
  new DOMParser().parseFromString(str, "text/html");
}
createDOM('<img src="//" onerror="console.log(\'You are safe\')" />');

However, note that `DOMParser` provides safety if you only want to manipulate the DOM elements from an untrusted HTML string. It's like a sandbox. But if then you get these elements and append it in the current document, they will still be able to run JS.

function createDOM(str) {
  document.body.appendChild(new DOMParser().parseFromString(str, "text/html").body);
}
createDOM('<img src="//" onerror="console.log(\'You are pwned!\')" />');

If you really need something like this, I would use a small whitelist of allowed elements and attributes, and get rid of everything else.

Problem

I create a script for try remove insecure content (I'm using it for browser extensions): ``` var str = "<strong>Hello</strong> mundo <script src="http://site/badscript.js"></script>"; CreateDOM(str); function RemoveAttrs(target) { var attrs = target.attributes, currentAttr; var validAttrs = [ "href", "class", "id", "target" ]; for (var i = attrs.length - 1; i >= 0; i--) { currentAttr = attrs[i].name; if (attrs[i].specified && validAttrs.indexOf(currentAttr) === -1) { target.removeAttribute(currentAttr); } if ( currentAttr === "href" && /^(#|javascript[:])/gi.test(target.getAttribute("href")) ) { target.parentNode.removeChild(currentAttr); } } } function RemoveEls(target) { var current; //Remove elements insecure (blacklist) var list = target.querySelectorAll("script,link,..."); for (var i = list.length - 1; i >= 0; i--) { current = list[i]; current.parentNode.removeChild(current); } //Remove insecure attributes (whitelist) list = target.getElementsByTagName("*"); for (i = list.length - 1; i >= 0; i--) { RemoveAttrs(list[i]); } return target; } function CreateDOM(MinhaString) { var tmpDom = document.createElement("div"); tmpDom.innerHTML = MinhaString; tmpDom = RemoveEls(tmpDom); //Inject in container document.getElementById("container").appendChild(tmpDom); } ``` I'm using this script in an addon I created for opera and Google Chorme, however the site moderator ("addons.opera.com") said to me this: Your cleanDomString method is not safe, please replace: tmpDom.innerHTML = data; with: var tmpDom = (new DOMParser).parseFromString(data, "text/html").body; and remove: var tmpDom = document.createElement("div"); or use: https://github.com/operatester/safeResponse/blob/1.1/safeResponse.js dmichnowicz; May 30, 2016 8:46:57 AM UTC The code looks like this: ``` var str = "<strong>Hello</strong> mundo <script src="http://site/badscript.js"></script>"; CreateDOM(str); function RemoveAttrs(target) { var attrs = target.attributes, currentAttr; var validAttrs = [ "href", "class", "id", "target" ]; for (var i = attrs.length - 1; i >= 0; i--) { currentAttr = attrs[i].name; if (attrs[i].specified && validAttrs.indexOf(currentAttr) === -1) { target.removeAttribute(currentAttr); } if ( currentAttr === "href" && /^(#|javascript[:])/gi.test(target.getAttribute("href")) ) { target.parentNode.removeChild(currentAttr); } } } function RemoveEls(target) { var current; //Remove elements insecure (blacklist) var list = target.querySelectorAll("script,link,..."); for (var i = list.length - 1; i >= 0; i--) { current = list[i]; current.parentNode.removeChild(current); } //Remove insecure attributes (whitelist) list = target.getElementsByTagName("*"); for (i = list.length - 1; i >= 0; i--) { RemoveAttrs(list[i]); } return target; } function CreateDOM(MyString) { var tmpDom = (new DOMParser).parseFromString(MyString, "text/html").body; tmpDom = RemoveEls(tmpDom); //Inject in container document.getElementById("container").appendChild(tmpDom); } ``` I made the change, but I would like to understand what it became safer my code. For me they both seem to do the same thing. What are the differences (terms of their safety)?

Original source