Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

Tags:

javascript

xss

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)?

like image 589
Guilherme Nascimento Avatar asked May 31 '16 19:05

Guilherme Nascimento


1 Answers

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.

like image 174
Oriol Avatar answered Oct 16 '22 08:10

Oriol