Version 2.0.8 VS 2.2.7 different output on the same input

[albanx]

BUG: version 2.0.8 VS 2.2.7 different behaviour on the same input when running from phantomJS tests.

Background & Context

Consider this HTML to sanitize:

<allowStyleAsFirstTag/><span class="btn" href="" data-cwdb="%7B%22confirmation%22%3A%22Some%20confirmation%20here%22%2C%22action%22%3A%7B%22html%22%3A%22%23%23%23%20HTML%20content%20%23%23%23%22%7D%2C%22display%22%3A%22widget%22%7D">Test button</span>

When running on casper / phantonJS the version 2.0.8 works fine and returns the full html stripping out just <allowStyleAsFirstTag/>

The last version of Dompurify 2.2.7 instead removes all HTML returning an empty string.

Bug

Remove HTML that shouldn't be removed on specific browser engines.

Input

<allowStyleAsFirstTag/><span class="btn" href="" data-cwdb="%7B%22confirmation%22%3A%22Some%20confirmation%20here%22%2C%22action%22%3A%7B%22html%22%3A%22%23%23%23%20HTML%20content%20%23%23%23%22%7D%2C%22display%22%3A%22widget%22%7D">Test button</span>

Given output

EMPTY

Expected output

<span class="btn" href="" data-cwdb="%7B%22confirmation%22%3A%22Some%20confirmation%20here%22%2C%22action%22%3A%7B%22html%22%3A%22%23%23%23%20HTML%20content%20%23%23%23%22%7D%2C%22display%22%3A%22widget%22%7D">Test button</span>

Digging into the code all comes to this lines of DOMPurify 2.2.7:

              if (KEEP_CONTENT && !FORBID_CONTENTS[tagName]) {
                    console.error('===========>>>KEEP_CONTENT', KEEP_CONTENT);
                    var parentNode = getParentNode(currentNode);
                    var childNodes = getChildNodes(currentNode);

                    if (childNodes && parentNode) {
                        var childCount = childNodes.length;

                        for (var i = childCount - 1; i >= 0; --i) {
                            parentNode.insertBefore(cloneNode(childNodes[i], true), getNextSibling(currentNode));
                        }
                    }
                }

Replacing the above with the old method in 2.0.8, works fine:

                /* Keep content except for black-listed elements */
                if (KEEP_CONTENT && !FORBID_CONTENTS[tagName] && typeof currentNode.insertAdjacentHTML === 'function') {
                    try {
                        var htmlToInsert = currentNode.innerHTML;
                        currentNode.insertAdjacentHTML(
                            'AfterEnd',
                            trustedTypesPolicy ? trustedTypesPolicy.createHTML(htmlToInsert) : htmlToInsert
                        );
                    } catch (error) {}
                }

Any thoughts?

Thanks
Alban

[albanx]

from some further investigation I believe that the bug consist in these lines:

var parentNode = getParentNode(currentNode);
var childNodes = getChildNodes(currentNode);

The lookupGetter method is failing for some reasons.

By replacing with

                    var parentNode = currentNode.parentNode;
                    var childNodes = currentNode.childNodes;

it works as expected

[cure53]

Heya, this should address your problem afaics :)

#502

[albanx]

what is the reason to use a wrapper function getParentNode instead of going directly to the property parentNode?

[cure53]

I think possible DOM Clobbering was the reason here, cc @securitum-mb

[albanx]

I am trying to integrate the proposal here #502 as polyfill in the phantomJS, but seems not workinig

[cure53]

I don't think there is much we can (or want to) do here.

PhantomJS has been discontinued, there is known vulnerabilities as well - it makes no sense for us to change anything in the core to support PhantomJS.

[albanx]

Unfortunaately we have a lot of legacy tests, we need still to maintain in the transition process that will take a long time. In the mean time this is a local patch I am using to solve the issue:

var parentNode = getParentNode(currentNode) || currentNode.parentNode;
var childNodes = getChildNodes(currentNode) || currentNode.childNodes;

IYO is this a valid patch without introducing xss attacks?

[cure53]

I think it might in the worst case enable DOM clobbering, but not sure about XSS.

[cure53]

Closing this for now, nothing - afaics - we can do here.

[albanx]

I wouldn't close this issue simply because it breaks the backward compatibility. I think this patch is valid:

var parentNode = getParentNode(currentNode) || currentNode.parentNode;
var childNodes = getChildNodes(currentNode) || currentNode.childNodes;

the code should fall in the second case only in not supporting browsers.

[cure53]

Yeah, I think this might be a valid patch indeed. Let me try and run some tests.

[cure53]

@albanx Please check, does this do the trick for you?

[albanx]

Yes, looks good

[cure53]

Cool, thanks for being adamant!

[albanx]

Just tested the new version. It introduced another break for phantomJS, exactly this line:

doc = implementation.createDocument(NAMESPACE, 'template', null);

Seems that phantomJS does not support createDocument with parameters.

By replacing with the old line doc = implementation.createHTMLDocument(''); works fine.
We are in path of depracation and as temporary measure I can use this patch so I am not suggesting any more fix.

[cure53]

This extra parameter is needed by MSIE10 and MSIE11 - things will break without it.