Proper IE9 support

[edg2s]

As mentioned in #191 there are a few issues to overcome:

• DOMParser only parses XML in IE9, so a fallback would be needed. There are some options here: https://github.com/wikimedia/VisualEditor/blob/master/src/ve.utils.js#L812. From what I can see the current fallback ends up with doc.body being null after outerHTML is set.

      if (!doc || !doc.documentElement) {
          doc = implementation.createHTMLDocument('');
          body = doc.body;
          body.parentNode.removeChild(body.parentNode.firstElementChild);
          body.outerHTML = dirty;
      }

• Other parsing issues: @cure53: "Check this example in IE9: <%/z=%&gt&lt;p/&#111;nresize&#x3d;alert(1)//> Right now, DOMPurify allows passing in DOM nodes. But it returns a string. This is not ideal yet of course but likely can be fixed."

[edg2s]

Perhaps I'm misunderstanding that fallback, but shouldn't it be body.innerHTML = dirty;?

[cure53]

All that document-handling is not the reason why we don't support MSIE9 and fall-back to toStaticHTML - the reason is, that MSIE9 and older enable too many mXSS vectors like the one with <% and others (especially CSS expressions, XML namespace attacks and others).

Re-adding MSIE9 support will take us many days of testing and tons of extra code.

Right now, DOMPurify allows passing in DOM nodes. But it returns a string. This is not ideal yet of course but likely can be fixed.

This can indeed be fixed easily and PRs are welcome. Yet, we won't implement full support for MSIE9 - we have been there and it's hell :D

[cure53]

Or, in other words - we tried hard to make MSIE9 support work (back in early 2015) and agreed after a lot of work that it's not feasible. Not interested in going back to that dark place again :D

[edg2s]

Thanks, do you have any links to the previous issues and commits for my curiosity?

(BTW when I re-enabled IE9 locally and passed '<%/z=%&gt<p/onresize=alert(1)//>' I got a empty string result)

[cure53]

Check here:

https://cure53.de/pentest-report_dompurify.pdf http://www.nds.rub.de/research/publications/mXSS-Attacks/

(BTW when I re-enabled IE9 locally and passed '<%/z=%&gt<p/onresize=alert(1)//>' I got a empty string result)

Yep, that is because we removed the document handling IE9 requires to work properly - see above :D

[edg2s]

Thanks. Is the fallback toStaticHtml considered to be safe?

[cure53]

Sort of :D We tested it excessively for MS back then. A lot of effort went into it. But I cannot give any guarantees.

Yet - when you bypass it, you have a browser bug - eligible for bounty.

[cure53]

Moving over to this ticket again... So, factually, there is one case we need to cover:

var clean = DOMPurify.sanitize(dirty, {RETURN_DOM: true});

This doesn't work properly in legacy MSIE. Including MSIE9. However. Most of the other config flags don't work either. Or am I thinking the wrong direction?

[edg2s]

Yes, I think it's important to get the return type correct. I'm not a huge fan of having option flags that change the return type, but given that's where we are, a user should at least be able to know what type a method will return.

[cure53]

So, I checked the code again and I think we shouldn't do it.

The legacy MSIE handling happens, before the config is even parsed. We'd have to move the config parsing to a different place, make sure that we have different code branches for different MSIE versions, explain in the docs why certain config flags have effect. others not... not a very pleasant thing.

I'd say we leave things as the are. On legacy MSIE, DOMPurify works but the config flags won't. I think that's consistent and as far as we can go with old messy browsers. Thoughts?

[edg2s]

I would agree for config flags that change the normalisation settings, however I think the flags that change the return type shouldn't have been flags in the first place. Under most circumstances, methods should have a clearly defined return type. In this case the API should've been something like "sanitizeAsHtml", "sanitizeAsDom", "sanitizeAsDocumentFragment" etc. These could even been implemented without breaking the current API.

With that in mind I don't think it's unreasonable to treat the return type flags differently from the other option flags.

[cure53]

Yeah, I see your point. But the person proposing those flags back then saw it exactly the other way round :D I am not sure how many people influence the return value via configuration right now - so I cannot make any judgment whether it's good to change things or not.

I recommend to leave things as they are for now and leave it to the implementer to deal with old IE. We sanitize on old IE as good as we can - and everything else is outside our scope.

[edg2s]

I can't really see the benefits of the sanitize('', {return_type}) approach. The only way that could be useful is if you wanted to change the return type based on parameters - but I can't think of a realistic use case for that.

The problem with the current situation is that every user of return_dom has to worry about handling strings, unless they can guarantee that isSupported is always true for their target audience.

[cure53]

Oh, sorry - closed this while you were typing.

I agree, but fact is, people wanted it and people use it. So we cannot and will not take it away. And it works well on all supported browsers - including MSIE10.

Websites with many MSIE9 and MSIE8 users will have to find their own ways to deal with the fall-back we offer. Then again, users of ancient garbage-browsers like MSIE9 and MSIE8 have bigger problems than XSS :D

[edg2s]

I don't think you should take away the old API, backwards compatibility is very important, and I'm not too bothered if another API is added or not, but I think it's very important to give the correct return type. Anyone currently using DOM returns and IE9 should be checking the return type, so this won't affect them - although if you really want to preserve the old behaviour, you could have the consist DOM return be only part of a new method.

[cure53]

Yeah, this is a flaw - but also an edge-case. And right now, the effort is too high (re-structure the library, re-structure the tests, add new internal methods - this will take several days to do right).

I would rather add a line to the docs stating that, on legacy MSIE none of the configuration flags, including the return type have any effect. This is not 100% clear yet in the docs.

Heck, some of the return types don't even exist on older MSIE :D Like document fragments etc.