HTML filter escaping html comments

This was broken by #101775. I committed a fix that restores the old behaviour, which simply removed all HTML comments.

Note that leaving HTML comments in place is very bad from a security point of view, because of various browser inconsistencies in parsing them. It is better to be safe than sorry.

Steven, can you provide some examples of how enabling the HTML comments would be a security issue?

We currently need to utilize them in order to allow a standards compliant way of embedding flash video. See http://code.google.com/p/swfobject/

Thanks

I've attached a small patch to the filter.module that enables HTML comments to pass through the filter. It needs more testing and is probably the wrong route to approach the problem. I'll have to read through the filter.module code again to find the correct way.

This patch adds

to the list of matches which will be passed into filter_xss_split. It also removes the > character as a match (probably incorrectly) in order to catch the case of <!-- [if IE]>--> being turned into <!--[if IE]&gt;-->

Here is an updated patch. It now is deactivated by default (filter will remove the html comments) and can be enabled per Input Format.

Tested using the following: regular comment, conditional comment, multi line comment, and SWFObject:

<!-- Shouldn't see this but it should be in the source -->
<!--[if gte IE 6]> 
<h2>Should see this on IE6+</h2> <![endif]--> 
<!--[if IE]>
<script>
alert('testo on IE')</script> 
<![endif]-->
<!-- 
Multi
Line
Comment
-->
<!-- -->
<h2>SWFObject Object Embed</h2>
<object classid="clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" height="260" id="media-container" width="320">
	<param name="movie" value="http://video.cws.oregonstate.edu/player.swf?file=gqzvh-std.mp4&amp;image=http%3A//video.cws.oregonstate.edu/gqzvh-std.jpg&amp;fullscreen=true">
	</param>
	<!--[if !IE]>--> 	
	<object data="http://video.cws.oregonstate.edu/std/gqzvh.swf" height="260" type="application/x-shockwave-flash" width="320">
		<!--<![endif]--> 	
		<param name="allowfullscreen" value="true">
		</param>
		<param name="allowscriptaccess" value="always">
		</param>
		<div class="shadow-left-caption">
		<img alt="Space Shuttle screenshot" src="http://video.cws.oregonstate.edu/gqzvh-std.jpg" />
		<p>
		This media requires <a href="http://www.adobe.com/go/getflashplayer">Adobe Flash Player</a>. 
		</p>
		</div>
		<!--[if !IE]>--> 	
	</object>
	<!--<![endif]--> 	
	<param name="allowfullscreen" value="true">
	</param>
	<param name="allowscriptaccess" value="always">
	</param>
</object>

subscribing

The conditional comments show exactly how HTML comments can do more than just commenting. I am not aware of other malicious uses offhand, but there may be cases where a browser interprets a specially-crafted comment.

You are already allowing object tags, the user can include remote applets, flash, movies and other plugins. Go ahead and just don't use HTML filtering in this case, such as the default Full HTML input format. Do make sure you trust users with permission to do this; know who they are and make sure they have an understanding of phishing and basic computer security.

Alternatively, use a module designed to embed media, such as http://drupal.org/project/emfield.

The patch in #7 works for me against Drupal 5.16. Pending a resolution about the validity of this I can do a full review.

I and others have written modules which use comments as markup for content. The main advantage is that when the input filters are disabled, the markup doesn't end up displaying to users (like how it would for img_assist's markup). I used this because I "did what core did", modelling after <!-- break-->. While for the object tag full HTML makes sense, there are many other use cases where it doesn't. This essentially represents an API change, so even if such markup is discouraged in the future, I think it should be supported for D5 (and possibly D6; haven't had a chance to test yet).

In terms of browser security, comments are a part of the XHTML specification. If they are doing things beyond the spec that they shouldn't do, then IMO that's not our problem and it's up to the browser to be fixed.

Other modules which are affected by this can be found at a similar issue against HTML corrector at #222926: HTML Corrector filter escapes HTML comments.

Is this an API change? When did Drupal 5 support HTML comments passing through the HTML filters?

We have to consider browser security of what browsers actually do, not the specs.

This might want to be moved up to the development version.

#222926: HTML Corrector filter escapes HTML comments