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 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.") 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./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)?

• javascript
• xss

1 Answer 1

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.