run stripTags twice to ensure allowed tags are still respected

[gka]

No description provided.

[breakingsystems]

I found relying on checking > characters alone was insufficient as a simple <img alt=">" onerror... was enough to bypass the check.

I'm not quite sure I can follow. Wouldn't the event handler here be filtered out the same as in "<img alt="x" onerror..." (by the code that checks whether the attribute starts with "on")?

Currently, the <span>-mangling still leads to unexpected results. While so far we haven't found a way to exploit this for XSS, it does not seem super resilient:

> purifyHTML("<span")
"<span< span=\"\"></span<>"
> purifyHTML("<i<iframe src=")
"<i<iframe src=\"</span\"></i<iframe>"
> purifyHTML('<i\u2028frame src=x')
"<i\u2028frame src=\"x</span\"></i\u2028frame>" // -> Highlighted in browser as "<iframe>"
> purifyHTML("<!DOCTYP")
"<!--DOCTYP</span-->
> purifyHTML("<!DOCTYPE")
""
> purifyHTML("<!--")
"<!--</span>-->"

While having a closer look at the function now, we found another unrelated way to bypass it:

> purifyHTML("<a href=javAscript:alert(1) target=_self>link</a>")
"<a href=\"javAscript:alert(1)\" target=\"_self\" rel=\"nofollow noopener noreferrer\">link</a>"

While the special characters are removed when the href attribute is set, the check afterwards is case-sensitive while the URI scheme is not.
(For old browsers, data: and vbscript: could also potentially lead to XSS. In general it seems like a http(s)+mailto-AllowList would be a good idea.)

Overall it might also be a good idea to switch from the custom phpjs-strip_tags-based implementation to a fully-featured and maintained library: https://www.npmtrends.com/dompurify-vs-sanitize-html-vs-xss

I took a stab at replicating the current behavior using sanitize-html's configuration options (implementing the current link target and rel attribute handling and allowing all attributes besides event handlers):

const sanitizeHtml = require('sanitize-html');

["<a href=javascrIpt:alert(1) target=\"_self\">link</a>",
 "<a href=test target=_self>link</a>",
 "<a href=test target=_parent>link</a>",
 "<a href=test>link</a>",
 "<a href=test rel=foo foo=bar>link</a>",
 "<img src=x>",
 "<img src=x ONERROR=alert(1)>",
 "<img src=x onerror=alert(1) onerror=alert(2) onload=alert(3)>",
 "<img src=x alt=1>",
 "<iframe src=x>",
 "<unknown>",
 "<span",
 "<span style=\"color:blue\">blue</span>",
 "<iframe src=1 blub=1"].forEach( function(test_str) {
    console.log(test_str + " >>>> " + sanitizeHtml(test_str, { 
	allowedAttributes: false, 
	allowedTags: sanitizeHtml.defaults.allowedTags.concat([ 'img' ]),
	exclusiveFilter: function(frame) { 
		return Object.keys(frame.attribs).some(function(attr) { return attr.startsWith('on') } ) 
	}, 
	transformTags: {
		'a': function(tagName, attribs) { 
			attribs['rel'] = 'nofollow noopener noreferrer';
			if(attribs['target'] !== '_self') attribs['target'] = '_blank';		        
			return { 
				tagName: 'a', 
				attribs: attribs
			} 
		}
	} 
	}))
})

Output:

<a href=javascrIpt:alert(1) target="_self">link</a> >>>> <a target="_self" rel="nofollow noopener noreferrer">link</a>
<a href=test target=_self>link</a> >>>> <a href="test" target="_self" rel="nofollow noopener noreferrer">link</a>
<a href=test target=_parent>link</a> >>>> <a href="test" target="_blank" rel="nofollow noopener noreferrer">link</a>
<a href=test>link</a> >>>> <a href="test" rel="nofollow noopener noreferrer" target="_blank">link</a>
<a href=test rel=foo foo=bar>link</a> >>>> <a href="test" rel="nofollow noopener noreferrer" foo="bar" target="_blank">link</a>
<img src=x> >>>> <img src="x" />
<img src=x ONERROR=alert(1)> >>>> 
<img src=x onerror=alert(1) onerror=alert(2) onload=alert(3)> >>>> 
<img src=x alt=1> >>>> <img src="x" alt="1" />
<iframe src=x> >>>> 
<unknown> >>>> 
<span >>>> 
<span style="color:blue">blue</span> >>>> <span style="color:blue">blue</span>
<iframe src=1 blub=1 >>>>