Merge lp:~quam-plures-core/quam-plures/xss-in-balance-tags_alternate into lp:quam-plures

[Tilman Blumenbach]

-------------------------------------------
This is an alternative to https://code.launchpad.net/~quam-plures-core/quam-plures/xss-in-balance-tags/+merge/53602
Proposed merge to make reviewing easier.
-------------------------------------------

This branch fixes bug 736035 by basically removing any processing instructions in user-supplied HTML. It also moves some HTML hacks to better suited locations.

[Tilman Blumenbach (tblue)]

At the moment, this removes the dangerous tokens; it may be wise to escape them instead ("<?" becomes "&lt;?" and "<!" becomes "&lt;!" -- this would make it possible to simply use "<!--" instead of having to write "&lt;!--" in comments etc.).

[EdB (edb)]

I like this one better because it doesn't say "concatenate stuff due to PHP4.3.whatever" but I dunno shit from shinola wrt testing or even verificating everything looks good, so y'all figure it out and I'll just agree with whatever fix seems most popular :)

[Tilman Blumenbach (tblue)]

Forget my first comment here. It doesn't make sense as you sometimes want to include HTML comments on purpose. This branch still allows <? stuff if the rest of the HTML is valid (i. e. if there are no forbidden tags), but I don't think that's a problem. Can you do any harm by using processing instructions?

[Yabs (yabs)]

Certainly seems to work and is a better solution ;)

¥

review: Approve

=== modified file 'qp_inc/_core/_param.funcs.php'
--- qp_inc/_core/_param.funcs.php	2010-12-31 12:12:03 +0000
+++ qp_inc/_core/_param.funcs.php	2011-03-16 17:27:57 +0000
@@ -1681,6 +1681,8 @@
 
 	// Replace any & that is not a character or entity reference with &
 	$content = preg_replace( '/&(?!#[0-9]+;|#x[0-9a-fA-F]+;|[a-zA-Z_:][a-zA-Z0-9._:-]*;)/', '&', $content );
+	// Fix for smileys like LOVE <3 (and other situations with '<' before a number).
+	$content = preg_replace( '#<([0-9])#', '&lt;$1', $content );
 
 	// ANTISPAM check:
 	if( ! $bypass_antispam
@@ -1812,12 +1814,6 @@
 	$tagqueue = '';
 	$newtext = '';
 
-	# b2 bug fix for comments - in case you REALLY meant to type '< !--'
-	$text = str_replace('< !--', '<    !--', $text);
-
-	# b2 bug fix for LOVE <3 (and other situations with '<' before a number)
-	$text = preg_replace('#<([0-9]{1})#', '&lt;$1', $text);
-
 	while( preg_match('~<(\s*/?\w+)\s*(.*?)/?>~', $text, $regex) )
 	{
 		$newtext = $newtext . $tagqueue;
@@ -1902,10 +1898,6 @@
 		$newtext = $newtext . '</' . $x . '>'; // Add remaining tags to close
 	}
 
-	# b2 fix for the bug with HTML comments
-	$newtext = str_replace( '< !--', '<'.'!--', $newtext ); // the concatenation is needed to work around some strange parse error in PHP 4.3.1
-	$newtext = str_replace( '<    !--', '< !--', $newtext );
-
 	return $newtext;
 }
 

=== modified file 'qp_inc/xhtml_validator/_xhtml_validator.class.php'
--- qp_inc/xhtml_validator/_xhtml_validator.class.php	2010-12-31 12:12:03 +0000
+++ qp_inc/xhtml_validator/_xhtml_validator.class.php	2011-03-16 17:27:57 +0000
@@ -166,11 +166,11 @@
 			}
 		}
 
-		// Open comments or '<![CDATA[' are dangerous
-		$xhtml = str_replace('<!', '', $xhtml);
-
-		// Convert isolated & chars
-		$xhtml = preg_replace( '#(\s)&(\s)#', '\\1&amp;\\2', $xhtml );
+		// Remove special tokens:
+		// "<!": Open comments or '<![CDATA[' are dangerous.
+		// "<?": Processing instructions allow an attacker to inject arbitrary HTML.
+		//       (LP bug #736035).
+		$xhtml = str_replace( array( '<!', '<?' ), '', $xhtml );
 
 		$xhtml_head = '<?xml version="1.0"';
 		if( ! empty($this->encoding) )
@@ -327,4 +327,4 @@
 }
 
 
-?>
\ No newline at end of file
+?>