Some more intrusive XSS cleaning

narfbg · narfbg · commit f0f47da9ae42 · 2015-10-05T12:37:16.000+03:00
diff --git a/system/core/Security.php b/system/core/Security.php
@@ -498,8 +498,8 @@ public function xss_clean($str, $is_image = FALSE)
 			.'(?<attributes>(?:[\s\042\047/=]*' // non-attribute characters, excluding > (tag close) for obvious reasons
 			.'[^\s\042\047>/=]+' // attribute characters
 			// optional attribute-value
-				.'(?:\s*=\s*' // attribute-value separator
-					.'(?:\042[^\042]*\042|\047[^\047]*\047|[^\s\042\047=><`]*)' // single, double or non-quoted value
+				.'(?:\s*=' // attribute-value separator
+					.'(?:[^\s\042\047=><`]+|\s*\042[^\042]*\042|\s*\047[^\047]*\047|\s*(?U:[^\s\042\047=><`]*))' // single, double or non-quoted value
 				.')?' // end optional attribute-value group
 			.')*)' // end optional attributes group
 			.'[^>]*)(?<closeTag>\>)?#isS';
@@ -808,7 +808,7 @@ protected function _sanitize_naughty_html($matches)
 				.'([\s\042\047/=]*)' // non-attribute characters, excluding > (tag close) for obvious reasons
 				.'(?<name>[^\s\042\047>/=]+)' // attribute characters
 				// optional attribute-value
-				.'(?:\s*=(?:[^\s\042\047=><`]+|\s*\042[^\042]+\042|\s*\047[^\047]+\047|\s*(?U:[^\s\042\047=><`]*)))' // attribute-value separator
+				.'(?:\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))
@@ -818,8 +818,14 @@ protected function _sanitize_naughty_html($matches)
 				// so we don't damage the string.
 				for ($i = $count - 1; $i > -1; $i--)
 				{
-					// Is it indeed an "evil" attribute?
-					if (preg_match('#^('.implode('|', $evil_attributes).')$#i', $attributes[$i]['name'][0]))
+					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'],
diff --git a/tests/codeigniter/core/Security_test.php b/tests/codeigniter/core/Security_test.php
@@ -146,7 +146,7 @@ public function test_xss_clean_sanitize_naughty_html_tags()
 		$this->assertEquals('<fubar>', $this->security->xss_clean('<fubar>'));
 
 		$this->assertEquals(
-			'<img <svg=""> src="x">',
+			'<img [removed]> src="x">',
 			$this->security->xss_clean('<img <svg=""> src="x">')
 		);
 
@@ -209,9 +209,14 @@ public function test_xss_clean_sanitize_naughty_html_attributes()
 		);
 
 		$this->assertEquals(
-			'<b "=<=  [removed]>',
+			'<b [removed] [removed]>',
 			$this->security->xss_clean('<b "=<= onmouseover=alert(1)>')
 		);
+
+		$this->assertEquals(
+			'<b [removed] [removed]alert&#40;1&#41;,1>1">',
+			$this->security->xss_clean('<b a=<=" onmouseover="alert(1),1>1">')
+		);
 	}
 
 	// --------------------------------------------------------------------
