Fix #17874: XSS in file uploads

An attacker can upload a Flash file with an image extension. If such an
attachment is displayed inline, it becomes a vector for XSS attacks.

This issue was reported by Matthias Karlsson (http://mathiaskarlsson.me)
as part of Offensive Security's bug bounty program [1].

Patch with contribution from Victor Boctor.

file_download.php

@@ -130,42 +130,24 @@
 
 	header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s \G\M\T', $v_date_added ) );
 
+	$t_upload_method = config_get( 'file_upload_method' );
 	$t_filename = file_get_display_name( $v_filename );
-	# 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 );
+	# Content headers
 
 	# If finfo is available (always true for PHP >= 5.3.0) we can use it to determine the MIME type of files
 	$finfo = finfo_get_if_available();
 
 	$t_content_type = $v_file_type;
 
 	$t_content_type_override = file_get_content_type_override ( $t_filename );
+	$t_file_info_type = false;
 
-	# dump file content to the connection.
-	switch ( config_get( 'file_upload_method' ) ) {
+	switch( $t_upload_method ) {
 		case DISK:
 			$t_local_disk_file = file_normalize_attachment_path( $v_diskfile, $t_project_id );
-
-			if ( file_exists( $t_local_disk_file ) ) {
-				if ( $finfo ) {
-					$t_file_info_type = $finfo->file( $t_local_disk_file );
-
-					if ( $t_file_info_type !== false ) {
-						$t_content_type = $t_file_info_type;
-					}
-				}
-
-				if ( $t_content_type_override ) {
-					$t_content_type = $t_content_type_override;
-				}
-
-				header( 'Content-Type: ' . $t_content_type );
-				readfile( $t_local_disk_file );
+			if( file_exists( $t_local_disk_file ) && $finfo ) {
+				$t_file_info_type = $finfo->file( $t_local_disk_file );
 			}
 			break;
 		case FTP:
@@ -179,32 +161,42 @@
 
 			if ( $finfo ) {
 				$t_file_info_type = $finfo->file( $t_local_disk_file );
-
-				if ( $t_file_info_type !== false ) {
-					$t_content_type = $t_file_info_type;
-				}
 			}
-
-			if ( $t_content_type_override ) {
-				$t_content_type = $t_content_type_override;
-			}
-
-			header( 'Content-Type: ' . $t_content_type );
-			readfile( $t_local_disk_file );
 			break;
-		default:
+		case DATABASE:
 			if ( $finfo ) {
 				$t_file_info_type = $finfo->buffer( $v_content );
-
-				if ( $t_file_info_type !== false ) {
-					$t_content_type = $t_file_info_type;
-				}
 			}
+	}
 
-			if ( $t_content_type_override ) {
-				$t_content_type = $t_content_type_override;
-			}
+	if( $t_file_info_type !== false ) {
+		$t_content_type = $t_file_info_type;
+	}
+
+	if( $t_content_type_override ) {
+		$t_content_type = $t_content_type_override;
+	}
+
+	# Don't allow inline flash
+	if( false !== strpos( $t_content_type, 'application/x-shockwave-flash' ) ) {
+		http_content_disposition_header( $t_filename );
+	} else {
+		http_content_disposition_header( $t_filename, $f_show_inline );
+	}
+
+	header( 'Content-Type: ' . $t_content_type );
+	header( 'Content-Length: ' . $v_filesize );
 
-			header( 'Content-Type: ' . $t_content_type );
+	# 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' );
+
+	# dump file content to the connection.
+	switch( $t_upload_method ) {
+		case DISK:
+		case FTP:
+			readfile( $t_local_disk_file );
+			break;
+		case DATABASE:
 			echo $v_content;
 	}