Skip to content

Commit

Permalink
Fixed Unicode whitespace attacks based on further research after Tom'…
Browse files Browse the repository at this point in the history 
…s finding
  • Loading branch information
mario committed Jun 3, 2014
1 parent fe15730 commit 40586b5
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 6 deletions.
5 changes: 4 additions & 1 deletion purify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@
var regex = /^(\w+script|data):/gi,
clonedNode = currentNode.cloneNode(true),
tmp, clobbering;

/* This needs to be extensive thanks to Webkit/Blink's behavior */
var whitespace = /[\x00-\x20\xA0\u1680\u180E\u2000-\u2029\u205f\u3000]/g;

/* Check if we have attributes; if not we might have a text node */
if(currentNode.attributes) {
Expand All @@ -340,7 +343,7 @@
(ALLOW_DATA_ATTR && tmp.name.match(/^data-[\w-]+/i)))

/* Get rid of script and data URIs */
&& (!tmp.value.replace(/[\x00-\x20\u2028-\u2029]/g,'').match(regex)
&& (!tmp.value.replace(whitespace,'').match(regex)

/* Keep image data URIs alive if src is allowed */
|| (tmp.name === 'src'
Expand Down
10 changes: 5 additions & 5 deletions test/expect.json
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
[
{
"title": "JavaScript URIs using Unicode LS/PS",
"title": "JavaScript URIs using Unicode LS/PS I",
"payload": "123<a href='\u2028javascript:alert(1)'>I am a dolphin!</a>",
"expected": "123<a>I am a dolphin!</a>"
},
{
"title": "JavaScript URIs using Unicode LS/PS",
"title": "JavaScript URIs using Unicode LS/PS II",
"payload": "123<a href='\u2028javascript:alert(1)'>I am a dolphin too!</a>",
"expected": "123<a>I am a dolphin too!</a>"
},
{
"title": "JavaScript URIs using Unicode LS/PS",
"payload": "123<a href='j\u2028avascript:alert(1)'>I am a dolphin too!</a>",
"expected": "123<a>I am a dolphin too!</a>"
"title": "JavaScript URIs using Unicode Whitespace",
"payload": "123<a href=' javascript:alert(1)'>CLICK</a><a href='&#xA0javascript:alert(1)'>CLICK</a><a href='&#x1680;javascript:alert(1)'>CLICK</a><a href='&#x180E;javascript:alert(1)'>CLICK</a><a href='&#x2000;javascript:alert(1)'>CLICK</a><a href='&#x2001;javascript:alert(1)'>CLICK</a><a href='&#x2002;javascript:alert(1)'>CLICK</a><a href='&#x2003;javascript:alert(1)'>CLICK</a><a href='&#x2004;javascript:alert(1)'>CLICK</a><a href='&#x2005;javascript:alert(1)'>CLICK</a><a href='&#x2006;javascript:alert(1)'>CLICK</a><a href='&#x2006;javascript:alert(1)'>CLICK</a><a href='&#x2007;javascript:alert(1)'>CLICK</a><a href='&#x2008;javascript:alert(1)'>CLICK</a><a href='&#x2009;javascript:alert(1)'>CLICK</a><a href='&#x200A;javascript:alert(1)'>CLICK</a><a href='&#x200B;javascript:alert(1)'>CLICK</a><a href='&#x205f;javascript:alert(1)'>CLICK</a><a href='&#x3000;javascript:alert(1)'>CLICK</a>",
"expected": "123<a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a><a>CLICK</a>"
},
{
"title": "Image with data URI src",
Expand Down

5 comments on commit 40586b5

@cweb
Copy link

@cweb cweb commented on 40586b5 Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, which browser are these working in? They weren't active some years ago when I tested, save for Opera.

But I do remember, the HTML 4.01 specification defines four whitespace characters, and explicitly does not define other cases. So, any character can be treated as whitespace by an HTML4 conforming User-Agent.

The HTML 5 specification changes that by defining five types of "space characters", and explicitly nothing else.

@mathiasbynens
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can haz link to “Tom’s finding”?

@cure53
Copy link
Owner

@cure53 cure53 commented on 40586b5 Aug 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cweb These surprisingly worked in Chrome - but only in case the DOM is rewritten.

@mathiasbynens Just try the characters in combination with document.write() or innerHTML (or anything comparable). Will work :)

@mathiasbynens
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking the link on this page does not trigger an alert for me in Chrome 38:

data:text/html;charset=utf-8,<script>document.write("123<a%20href='%5Cu2028javascript%3Aalert(1)'>I%20am%20a%20dolphin!</a>")</script>

Has this “browser bug” (debatable) been fixed since, or is my test wrong?

@cure53
Copy link
Owner

@cure53 cure53 commented on 40586b5 Oct 26, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it on different Chrome versions without using the white-space regex and it still repros for me (Chromium 37, Chrome 38, Chrome Canary 40). So I'd assume there's no fix yet.

The code I used for testing was:

document.body.innerHTML=DOMPurify.sanitize(
    "123<a href='\u2028javascript:alert(1)'>I am a dolphin too!</a>"
)

I haven't seen any tickets on crbug for this - and agree that calling it browser bug is debatable. However, it was WebKit/Blink-only behavior, so I'd be surprised it was expected :)

Please sign in to comment.