Skip to content

Commit

Permalink
Html: workaround for innerHTML mXSS vulnerability [Closes #1496]
Browse files Browse the repository at this point in the history 
IE8 for code `<div attr="``foo=bar">` produces invalid innerHTML `<div attr=``foo=bar>`. Adding a space at the end of the attribute forces IE to put quotes around the attribute.

More info:
http://www.nds.rub.de/research/publications/mXSS-Attacks/
http://www.slideshare.net/x00mario/the-innerhtml-apocalypse
  • Loading branch information
@dg
dg committed May 24, 2014
1 parent a1f7fbc commit 18370a2
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 3 deletions.
10 changes: 8 additions & 2 deletions Nette/Utils/Html.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,10 @@ public function attributes()
foreach ($value as $k => $v) {
if ($v !== NULL && $v !== FALSE) {
$q = strpos($v, '"') === FALSE ? '"' : "'";
$s .= ' data-' . $k . '=' . $q . str_replace(array('&', $q), array('&amp;', $q === '"' ? '&quot;' : '&#39;'), $v) . $q;
$s .= ' data-' . $k . '='
. $q . str_replace(array('&', $q), array('&amp;', $q === '"' ? '&quot;' : '&#39;'), $v)
. (strpos($v, '`') === FALSE ? '' : ' ')
. $q;
}
}
continue;
Expand All @@ -549,7 +552,10 @@ public function attributes()
}

$q = strpos($value, '"') === FALSE ? '"' : "'";
$s .= ' ' . $key . '=' . $q . str_replace(array('&', $q), array('&amp;', $q === '"' ? '&quot;' : '&#39;'), $value) . $q;
$s .= ' ' . $key . '='
. $q . str_replace(array('&', $q), array('&amp;', $q === '"' ? '&quot;' : '&#39;'), $value)
. (strpos($value, '`') === FALSE ? '' : ' ')
. $q;
}

$s = str_replace('@', '&#64;', $s);
Expand Down
1 change: 1 addition & 0 deletions tests/Nette/Utils/Html.basic.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ test(function() {

test(function() { // attributes escaping
Assert::same( '<a one=\'"\' two="\'" three="<>" four="&amp;amp;"></a>', (string) Html::el('a')->one('"')->two("'")->three('<>')->four('&amp;') );
Assert::same( '<a one="``xx\' " two=\'``x" \'></a>' , (string) Html::el('a')->one("``xx'")->two('``x"') ); // mXSS
});


Expand Down
3 changes: 2 additions & 1 deletion tests/Nette/Utils/Html.data.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ test(function() {
$el->data['c'] = FALSE;
$el->data['d'] = '';
$el->data['e'] = 'two';
$el->data['mxss'] = '``two';

Assert::same( '<div data-a="one" data-d="" data-e="two"></div>', (string) $el );
Assert::same( '<div data-a="one" data-d="" data-e="two" data-mxss="``two "></div>', (string) $el );
});


Expand Down

9 comments on commit 18370a2

@Majkl578
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice BC break in 2.1.

@dg
Copy link
Member Author

@dg dg commented on 18370a2 May 24, 2014

Choose a reason for hiding this comment

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

Suggest a better solution or shut...

@mishak87
Copy link
Contributor

Choose a reason for hiding this comment

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

@Majkl578 If safe web behaves like that it is not BC break but security fix. HTMLPurifier does exactly the same. This really is workflow BC break as xkcd described it.

@Majkl578
Copy link
Contributor

Choose a reason for hiding this comment

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

@mishak87: Arrogant reply as usual for you. :) The reason why this IS a BC break is this, which may really break existing code. Imagine e.g. some client-side templating system using data attributes for passing around field names.

@mishak87
Copy link
Contributor

Choose a reason for hiding this comment

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

@Majkl578 We all understand that you mean appending whitespace at the end of attribute value is not BC.

But to real case scenarios. Breaks are in application that:

  • is using backtick in a constant that affects code behaviour
  • is comparing sanitized and unsanitized code and expect it to be the same (your example)
  • uses sanitized input with poor understanding of sanitization
  • expects that outputted value will have no spaces (previously treated by trim)
  • does not handle invalid input properly
  • trusts data from server

List can go on...

@Majkl578 Please show at least one example that is not flawed by design and BC break occurs.

All bullets above are the reason I used hyperbole with xkcd comix. If you think that that my reply was arrogant please consult Websters dictionary. Calling thing as I see it does not make me arrogant but one with opinion (and surprisingly reason behind it).

@Majkl578
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw flaw by design in the application is not a valid argument for BC break. It is more likely an excuse.

@hrach
Copy link
Contributor

@hrach hrach commented on 18370a2 May 24, 2014

Choose a reason for hiding this comment

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

Stop. 🔚

@fprochazka
Copy link
Contributor

Choose a reason for hiding this comment

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

@Majkl578 do you have a better solution?

@dg
Copy link
Member Author

@dg dg commented on 18370a2 May 24, 2014

Choose a reason for hiding this comment

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

Another solution is to use entity ` which is unknown for IE < 10, but it changes content of affected attributes in much harder way.

Please sign in to comment.