Fix #11952: Arbitrary inline attachment rendering (security issue)

Kornel Lesinski reported a significant security problem with Mantis'
handling of attachments and MIME types. A user could upload a HTML file
renamed to .gif and MantisBT would use Fileinfo to calculate the actual
MIME type to be text/html. This is a problem because a user could be
directed to click on a link purporting to be a .gif attachment when in
fact it is a HTML document. Browsers would attempt to render the
text/html attachment within the browser instead of treating it as a file
download (or even just plaintext).

Internet Explorer exaggerates this problem by ignoring the Content-Type
header so that it can perform its own analysis of the file type. With
the expectation that Fileinfo and any browser MIME detection are
inherently insecure and unpredictable, we don't want to allow users to
upload random files that *could* be misinterpreted by MIME detectors for
malicious purposes. As we send our own Content-Type headers where
appropriate, we can tell Internet Explorer 8+ to stop it's own MIME type
sniffing, resulting in a performance boost for IE8+ users.

To solve this problem, a new 'show_inline' parameter has been introduced to
file_download.php which serves the purpose of specifying whether the
file should be served to the user as an attachment (within the
Content-Disposition header) or as an inline file that can be rendered
within the browser.

This parameter has also been protected using a CSRF token to ensure that
cross-domain attacks are not possible whereby a malicious user uploads a
HTML document and sends a link to someone else with &show_inline=1 set.
This is particularly an issue where an attacker places a MantisBT
instance within an invisible iframe such that the HTML attachment is
rendered unknown to users (and this HTML attachment could invoke
JavaScript to do nasty things within the domain of the MantisBT
installation).

In summary, MantisBT will treat all attachments as file downloads when
it prepares the Content-Disposition header. An exception currently
exists for inline image previews where MantisBT will create img src URLs
that point to file_download.php with show_inline=1 and a CSRF token
parameter provided. The MIME type isn't a significant concern in this
case as browsers won't try to render HTML where an image is expected.

core/file_api.php

@@ -323,13 +323,11 @@ function file_get_visible_attachments( $p_bug_id ) {
 		$t_ext = strtolower( file_get_extension( $t_attachment['display_name'] ) );
 		$t_attachment['alt'] = $t_ext;
 
-		if( $t_attachment['exists'] ) {
-			if ( $t_can_download && ( $t_filesize != 0 ) && ( $t_filesize <= config_get( 'preview_attachments_inline_max_size' ) ) && ( in_array( $t_ext, $t_preview_text_ext, true ) ) ) {
+		if ( $t_attachment['exists'] && $t_can_download && $t_filesize != 0 && $t_filesize <= config_get( 'preview_attachments_inline_max_size' ) ) {
+			if ( in_array( $t_ext, $t_preview_text_ext, true ) ) {
 				$t_attachment['preview'] = true;
 				$t_attachment['type'] = 'text';
-			}
-
-			if ( $t_can_download && ( $t_filesize != 0 ) && ( $t_filesize <= config_get( 'preview_attachments_inline_max_size' ) ) && ( in_array( utf8_strtolower( file_get_extension( $t_attachment['display_name'] ) ), $t_preview_image_ext, true ) ) ) {
+			} else if ( in_array( $t_ext, $t_preview_image_ext, true ) ) {
 				$t_attachment['preview'] = true;
 				$t_attachment['type'] = 'image';
 			}

core/http_api.php

@@ -120,7 +120,10 @@ function http_caching_headers( $p_allow_caching=false ) {
  */
 function http_content_headers() {
 	if ( !headers_sent() ) {
-		header( 'Content-type: text/html;charset=utf-8' );
+		header( 'Content-Type: text/html; charset=utf-8' );
+		# For Internet Explorer 8 as per http://blogs.msdn.com/ie/archive/2008/07/02/ie8-security-part-v-comprehensive-protection.aspx
+		# Don't let IE second guess our content-type!
+		header( 'X-Content-Type-Options: nosniff' );
 	}
 }
 

core/print_api.php

@@ -1674,7 +1674,7 @@ function print_bug_attachments_list( $p_bug_id ) {
 		}
 
 		if ( $t_attachment['can_download'] ) {
-			$t_href_start = "<a href=\"file_download.php?file_id={$t_attachment['id']}&amp;type=bug\">";
+			$t_href_start = "<a href=\"${t_attachment['download_url']}\">";
 			$t_href_end = '</a>';
 		} else {
 			$t_href_start = '';
@@ -1762,7 +1762,9 @@ function swap_content( span ) {
 				$t_preview_style = 'style="' . $t_preview_style . '"';
 				$t_title = file_get_field( $t_attachment['id'], 'title' );
 
-				echo "\n<br />$t_href_start<img alt=\"$t_title\" $t_preview_style src=\"file_download.php?file_id={$t_attachment['id']}&amp;type=bug\" />$t_href_end";
+				$t_image_url = $t_attachment['download_url'] . '&amp;show_inline=1' . form_security_param( 'file_show_inline' );
+
+				echo "\n<br />$t_href_start<img alt=\"$t_title\" $t_preview_style src=\"$t_image_url\" />$t_href_end";
 				$image_previewed = true;
 			}
 		}

file_download.php

@@ -52,8 +52,27 @@
 
 auth_ensure_user_authenticated();
 
-$f_file_id	= gpc_get_int( 'file_id' );
-$f_type		= gpc_get_string( 'type' );
+$f_show_inline = gpc_get_bool( 'show_inline', false );
+
+# To prevent cross-domain inline hotlinking to attachments we require a CSRF
+# token from the user to show any attachment inline within the browser.
+# Without this security in place a malicious user could upload a HTML file
+# attachment and direct a user to file_download.php?file_id=X&type=bug&show_inline=1
+# and the malicious HTML content would be rendered in the user's browser,
+# violating cross-domain security.
+if ( $f_show_inline ) {
+	# Disable errors for form_security_validate as we need to first need to
+	# send HTTP headers prior to raising an error (the error handler within
+	# error_api.php doesn't check that headers have been sent, it just
+	# makes the assumption that they've been sent already).
+	if ( !@form_security_validate( 'file_show_inline' ) ) {
+		http_all_headers();
+		trigger_error( ERROR_FORM_TOKEN_INVALID, ERROR );
+	}
+}
+
+$f_file_id = gpc_get_int( 'file_id' );
+$f_type	= gpc_get_string( 'type' );
 
 $c_file_id = (integer)$f_file_id;
 
@@ -129,15 +148,12 @@
 header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s \G\M\T', $v_date_added ) );
 
 $t_filename = file_get_display_name( $v_filename );
-$t_show_inline = false;
-$t_inline_files = explode( ',', config_get( 'inline_file_exts' ) );
-if ( $t_inline_files !== false && !is_blank( $t_inline_files[0] ) ) {
-	if ( in_array( utf8_strtolower( file_get_extension( $t_filename ) ), $t_inline_files ) ) {
-		$t_show_inline = true;
-	}
-}
 
-http_content_disposition_header( $t_filename, $t_show_inline );
+# For Internet Explorer 8 as per http://blogs.msdn.com/ie/archive/2008/07/02/ie8-security-part-v-comprehensive-protection.aspx
+# Don't let IE second guess our content-type!
+header( 'X-Content-Type-Options: nosniff' );
+
+http_content_disposition_header( $t_filename, $f_show_inline );
 
 header( 'Content-Length: ' . $v_filesize );