Harden xss_clean()

2 files changed, 59 insertions, 42 deletions

diff --git a/system/core/Security.php b/system/core/Security.php
index ab85e2239..36dea4cf2 100644
--- a/system/core/Security.php
+++ b/system/core/Security.php
@@ -803,43 +803,55 @@ class CI_Security {
 		// For other tags, see if their attributes are "evil" and strip those
 		elseif (isset($matches['attributes']))
 		{
-			// We'll need to catch all attributes separately first
-			$pattern = '#'
-				.'([\s\042\047/=]*)' // non-attribute characters, excluding > (tag close) for obvious reasons
+			// We'll store the already fitlered attributes here
+			$attributes = array();
+
+			// Attribute-catching pattern
+			$attributes_pattern = '#'
 				.'(?<name>[^\s\042\047>/=]+)' // attribute characters
 				// optional attribute-value
 				.'(?:\s*=(?<value>[^\s\042\047=><`]+|\s*\042[^\042]*\042|\s*\047[^\047]*\047|\s*(?U:[^\s\042\047=><`]*)))' // attribute-value separator
 				.'#i';
 
-			if ($count = preg_match_all($pattern, $matches['attributes'], $attributes, PREG_SET_ORDER | PREG_OFFSET_CAPTURE))
+			// Blacklist pattern for evil attribute names
+			$is_evil_pattern = '#^('.implode('|', $evil_attributes).')$#i';
+
+			// Each iteration filters a single attribute
+			do
 			{
-				// Since we'll be using substr_replace() below, we
-				// need to handle the attributes in reverse order,
-				// so we don't damage the string.
-				for ($i = $count - 1; $i > -1; $i--)
+				// Strip any non-alpha characters that may preceed an attribute.
+				// Browsers often parse these incorrectly and that has been a
+				// of numerous XSS issues we've had.
+				$matches['attributes'] = preg_replace('#^[^a-z]+#i', '', $matches['attributes']);
+
+				if ( ! preg_match($attributes_pattern, $matches['attributes'], $attribute, PREG_OFFSET_CAPTURE))
 				{
-					if (
-						// Is it indeed an "evil" attribute?
-						preg_match('#^('.implode('|', $evil_attributes).')$#i', $attributes[$i]['name'][0])
-						// Or an attribute not starting with a letter? Some parsers get confused by that
-						OR ! ctype_alpha($attributes[$i]['name'][0][0])
-						// Does it have an equals sign, but no value and not quoted? Strip that too!
-						OR (trim($attributes[$i]['value'][0]) === '')
-					)
-					{
-						$matches['attributes'] = substr_replace(
-							$matches['attributes'],
-							' [removed]',
-							$attributes[$i][0][1],
-							strlen($attributes[$i][0][0])
-						);
-					}
+					// No (valid) attribute found? Discard everything else inside the tag
+					break;
 				}
 
-				// Note: This will strip some non-space characters and/or
-				//       reduce multiple spaces between attributes.
-				return '<'.$matches['slash'].$matches['tagName'].' '.trim($matches['attributes']).'>';
+				if (
+					// Is it indeed an "evil" attribute?
+					preg_match($is_evil_pattern, $attribute['name'][0])
+					// Or does it have an equals sign, but no value and not quoted? Strip that too!
+					OR (trim($attribute['value'][0]) === '')
+				)
+				{
+					$attributes[] = 'xss=removed';
+				}
+				else
+				{
+					$attributes[] = $attribute[0][0];
+				}
+
+				$matches['attributes'] = substr($matches['attributes'], $attribute[0][1] + strlen($attribute[0][0]));
 			}
+			while ($matches['attributes'] !== '');
+
+			$attributes = empty($attributes)
+				? ''
+				: ' '.implode(' ', $attributes);
+			return '<'.$matches['slash'].$matches['tagName'].$attributes.'>';
 		}
 
 		return $matches[0];
diff --git a/tests/codeigniter/core/Security_test.php b/tests/codeigniter/core/Security_test.php
index 52967dc2f..2ef822863 100644
--- a/tests/codeigniter/core/Security_test.php
+++ b/tests/codeigniter/core/Security_test.php
@@ -115,7 +115,7 @@ class Security_test extends CI_TestCase {
 	public function test_xss_clean_entity_double_encoded()
 	{
 		$input = '<a href="&#38&#35&#49&#48&#54&#38&#35&#57&#55&#38&#35&#49&#49&#56&#38&#35&#57&#55&#38&#35&#49&#49&#53&#38&#35&#57&#57&#38&#35&#49&#49&#52&#38&#35&#49&#48&#53&#38&#35&#49&#49&#50&#38&#35&#49&#49&#54&#38&#35&#53&#56&#38&#35&#57&#57&#38&#35&#49&#49&#49&#38&#35&#49&#49&#48&#38&#35&#49&#48&#50&#38&#35&#49&#48&#53&#38&#35&#49&#49&#52&#38&#35&#49&#48&#57&#38&#35&#52&#48&#38&#35&#52&#57&#38&#35&#52&#49">Clickhere</a>';
-		$this->assertEquals('<a >Clickhere</a>', $this->security->xss_clean($input));
+		$this->assertEquals('<a>Clickhere</a>', $this->security->xss_clean($input));
 	}
 
 	// --------------------------------------------------------------------
@@ -134,7 +134,7 @@ class Security_test extends CI_TestCase {
 	public function test_xss_clean_js_img_removal()
 	{
 		$input = '<img src="&#38&#35&#49&#48&#54&#38&#35&#57&#55&#38&#35&#49&#49&#56&#38&#35&#57&#55&#38&#35&#49&#49&#53&#38&#35&#57&#57&#38&#35&#49&#49&#52&#38&#35&#49&#48&#53&#38&#35&#49&#49&#50&#38&#35&#49&#49&#54&#38&#35&#53&#56&#38&#35&#57&#57&#38&#35&#49&#49&#49&#38&#35&#49&#49&#48&#38&#35&#49&#48&#50&#38&#35&#49&#48&#53&#38&#35&#49&#49&#52&#38&#35&#49&#48&#57&#38&#35&#52&#48&#38&#35&#52&#57&#38&#35&#52&#49">Clickhere';
-		$this->assertEquals('<img >', $this->security->xss_clean($input));
+		$this->assertEquals('<img>', $this->security->xss_clean($input));
 	}
 
 	// --------------------------------------------------------------------
@@ -146,7 +146,7 @@ class Security_test extends CI_TestCase {
 		$this->assertEquals('<fubar>', $this->security->xss_clean('<fubar>'));
 
 		$this->assertEquals(
-			'<img [removed]> src="x">',
+			'<img svg=""> src="x">',
 			$this->security->xss_clean('<img <svg=""> src="x">')
 		);
 
@@ -160,21 +160,21 @@ class Security_test extends CI_TestCase {
 
 	public function test_xss_clean_sanitize_naughty_html_attributes()
 	{
-		$this->assertEquals('<foo [removed]>', $this->security->xss_clean('<foo onAttribute="bar">'));
-		$this->assertEquals('<foo [removed]>', $this->security->xss_clean('<foo onAttributeNoQuotes=bar>'));
-		$this->assertEquals('<foo [removed]bar>', $this->security->xss_clean('<foo onAttributeWithSpaces = bar>'));
+		$this->assertEquals('<foo xss=removed>', $this->security->xss_clean('<foo onAttribute="bar">'));
+		$this->assertEquals('<foo xss=removed>', $this->security->xss_clean('<foo onAttributeNoQuotes=bar>'));
+		$this->assertEquals('<foo xss=removed>', $this->security->xss_clean('<foo onAttributeWithSpaces = bar>'));
 		$this->assertEquals('<foo prefixOnAttribute="bar">', $this->security->xss_clean('<foo prefixOnAttribute="bar">'));
 		$this->assertEquals('<foo>onOutsideOfTag=test</foo>', $this->security->xss_clean('<foo>onOutsideOfTag=test</foo>'));
 		$this->assertEquals('onNoTagAtAll = true', $this->security->xss_clean('onNoTagAtAll = true'));
-		$this->assertEquals('<foo [removed]>', $this->security->xss_clean('<foo fscommand=case-insensitive>'));
-		$this->assertEquals('<foo [removed]>', $this->security->xss_clean('<foo seekSegmentTime=whatever>'));
+		$this->assertEquals('<foo xss=removed>', $this->security->xss_clean('<foo fscommand=case-insensitive>'));
+		$this->assertEquals('<foo xss=removed>', $this->security->xss_clean('<foo seekSegmentTime=whatever>'));
 
 		$this->assertEquals(
-			'<foo bar=">" baz=\'>\' [removed]>',
+			'<foo bar=">" baz=\'>\' xss=removed>',
 			$this->security->xss_clean('<foo bar=">" baz=\'>\' onAfterGreaterThan="quotes">')
 		);
 		$this->assertEquals(
-			'<foo bar=">" baz=\'>\' [removed]>',
+			'<foo bar=">" baz=\'>\' xss=removed>',
 			$this->security->xss_clean('<foo bar=">" baz=\'>\' onAfterGreaterThan=noQuotes>')
 		);
 
@@ -194,7 +194,7 @@ class Security_test extends CI_TestCase {
 		);
 
 		$this->assertEquals(
-			'<a [removed]>',
+			'<a xss=removed>',
 			$this->security->xss_clean('<a< onmouseover="alert(1)">')
 		);
 
@@ -204,19 +204,24 @@ class Security_test extends CI_TestCase {
 		);
 
 		$this->assertEquals(
-			'<image src="<>" [removed]>',
+			'<image src="<>" xss=removed>',
 			$this->security->xss_clean('<image src="<>" onerror=\'alert(1)\'>')
 		);
 
 		$this->assertEquals(
-			'<b [removed] [removed]>',
+			'<b xss=removed>',
 			$this->security->xss_clean('<b "=<= onmouseover=alert(1)>')
 		);
 
 		$this->assertEquals(
-			'<b [removed] [removed]alert&#40;1&#41;,1>1">',
+			'<b xss=removed xss=removed>1">',
 			$this->security->xss_clean('<b a=<=" onmouseover="alert(1),1>1">')
 		);
+
+		$this->assertEquals(
+			'<b x=" onmouseover=alert&#40;1&#41;//">',
+			$this->security->xss_clean('<b "="< x=" onmouseover=alert(1)//">')
+		);
 	}
 
 	// --------------------------------------------------------------------
@@ -228,7 +233,7 @@ class Security_test extends CI_TestCase {
 	public function test_naughty_html_plus_evil_attributes()
 	{
 		$this->assertEquals(
-			'&lt;svg<img &gt; src="x" [removed]>',
+			'&lt;svg<img src="x" xss=removed>',
 			$this->security->xss_clean('<svg<img > src="x" onerror="location=/javascript/.source+/:alert/.source+/(1)/.source">')
 		);
 	}