View Issue Details

IDProjectCategoryView StatusLast Update
0017874mantisbtsecuritypublic2014-12-05 18:33
Reporteravlidienbrunn Assigned Todregad  
PriorityhighSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version1.2.17 
Target Version1.2.18Fixed in Version1.2.18 
Summary0017874: CVE-2014-9271: Persistent XSS in file uploads/attachments
Description

It's possible to upload Flash files and make open them inline by using an image extension. Since Flash files can execute JavaScript this becomes a persistent XSS.

Originally reported under point 3. in 0017362

Steps To Reproduce

To reproduce, do this:

  1. Create/Download an SWF that executes javascript either by getURL('javascript:code()') or ExternalInterface.call('code'). You can find an example here https://github.com/evilcos/xss.swf
  2. Change the extension of the SWF to JPG (eg. "file.swf.jpg")
  3. Upload it as an attachment to an issue
  4. Directly accessing the "image" should demonstrate the XSS
TagsNo tags attached.
Attached Files
test.swf.jpg (147 bytes)   
test.swf.jpg (147 bytes)   
0001-Fix-XSS-in-file-uploads-without-spacing.patch (4,156 bytes)   
From 1c3cb7fdbb21f9e7cbbe7637b473bcd540e11e37 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Fri, 28 Nov 2014 17:35:27 -0800
Subject: [PATCH] Fix 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].

Fixes #17874
---
 file_download.php | 81 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 43 deletions(-)

diff --git a/file_download.php b/file_download.php
index 07fcdf2..12d0742 100644
--- a/file_download.php
+++ b/file_download.php
@@ -130,14 +130,10 @@
 
 	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();
@@ -145,27 +141,13 @@
 	$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,45 @@
 
 			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;
+		case DATABASE:
 		default:
 			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;
 
-			header( 'Content-Type: ' . $t_content_type );
+		# Don't allow inline flash
+		if( false !== strpos( $t_file_info_type, 'application/x-shockwave-flash' ) ) {
+			http_content_disposition_header( $t_filename );
+		} else {
+			http_content_disposition_header( $t_filename, $f_show_inline );
+		}
+	}
+
+	if ( $t_content_type_override ) {
+		$t_content_type = $t_content_type_override;
+	}
+
+	header( 'Content-Type: ' . $t_content_type );
+	header( 'Content-Length: ' . $v_filesize );
+
+	# 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:
+		default:
 			echo $v_content;
 	}
-- 
1.9.3 (Apple Git-50)

0001-Fix-XSS-in-file-uploads-with-small-update.patch (4,156 bytes)   
From 3bb794982755ac80ce4184e30e7dd300d107a658 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Fri, 28 Nov 2014 17:35:27 -0800
Subject: [PATCH] Fix 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].

Fixes #17874
---
 file_download.php | 81 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 43 deletions(-)

diff --git a/file_download.php b/file_download.php
index 07fcdf2..12d0742 100644
--- a/file_download.php
+++ b/file_download.php
@@ -130,14 +130,10 @@
 
 	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();
@@ -145,27 +141,13 @@
 	$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,45 @@
 
 			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;
+		case DATABASE:
 		default:
 			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;
 
-			header( 'Content-Type: ' . $t_content_type );
+		# Don't allow inline flash
+		if( false !== strpos( $t_file_info_type, 'application/x-shockwave-flash' ) ) {
+			http_content_disposition_header( $t_filename );
+		} else {
+			http_content_disposition_header( $t_filename, $f_show_inline );
+		}
+	}
+
+	if ( $t_content_type_override ) {
+		$t_content_type = $t_content_type_override;
+	}
+
+	header( 'Content-Type: ' . $t_content_type );
+	header( 'Content-Length: ' . $v_filesize );
+
+	# 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:
+		default:
 			echo $v_content;
 	}
-- 
1.9.3 (Apple Git-50)

test.html (43 bytes)   
<html><head></head><body>test</body></html>
test.html (43 bytes)   
0001-Fix-XSS-in-file-uploads.patch (4,129 bytes)   
From 344bcbb9e8d4580476d5b45bd695d63d90f27e44 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 29 Nov 2014 01:51:41 +0100
Subject: [PATCH 1/4] Fix 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].

Fixes #17874
---
 file_download.php | 80 +++++++++++++++++++++++++------------------------------
 1 file changed, 36 insertions(+), 44 deletions(-)

diff --git a/file_download.php b/file_download.php
index 07fcdf2..6d60817 100644
--- a/file_download.php
+++ b/file_download.php
@@ -130,14 +130,10 @@
 
 	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();
@@ -145,27 +141,13 @@
 	$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;
+
+		# Don't allow inline flash
+		if( false !== strpos( $t_file_info_type, 'application/x-shockwave-flash' ) ) {
+			http_content_disposition_header( $t_filename );
+		} else {
+			http_content_disposition_header( $t_filename, $f_show_inline );
+		}
+	}
+
+	if( $t_content_type_override ) {
+		$t_content_type = $t_content_type_override;
+	}
+
+	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;
 	}
-- 
1.9.1

0002-Improve-comment-for-nosniff-header.patch (1,222 bytes)   
From 42c9b01114b00dbb19efee72e0aa99c60c6a1ce5 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 29 Nov 2014 16:50:21 +0100
Subject: [PATCH 2/4] Improve comment for 'nosniff' header

- Reworded the part about IE8 second-guessing content type
- Added a note about Flash, as per Mathias Karlsson's recommendation in
  issue #17874
---
 file_download.php | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/file_download.php b/file_download.php
index 6d60817..803b083 100644
--- a/file_download.php
+++ b/file_download.php
@@ -187,8 +187,10 @@
 	header( 'Content-Type: ' . $t_content_type );
 	header( 'Content-Length: ' . $v_filesize );
 
-	# 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!
+	# Don't let Internet Explorer second-guess our content-type [1]
+	# Also disable Flash content-type sniffing [2]
+	# [1] http://blogs.msdn.com/b/ie/archive/2008/07/02/ie8-security-part-v-comprehensive-protection.aspx
+	# [2] http://50.56.33.56/blog/?p=242,
 	header( 'X-Content-Type-Options: nosniff' );
 
 	# dump file content to the connection.
-- 
1.9.1

0001-Fix-XSS-in-file-uploads-again.patch (4,148 bytes)   
From 749944df2d2401855b18c6f8e458fd382852445d Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Fri, 28 Nov 2014 17:35:27 -0800
Subject: [PATCH] Fix 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].

Fixes #17874
---
 file_download.php | 81 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 43 deletions(-)

diff --git a/file_download.php b/file_download.php
index 07fcdf2..6221524 100644
--- a/file_download.php
+++ b/file_download.php
@@ -130,14 +130,10 @@
 
 	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();
@@ -145,27 +141,13 @@
 	$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,45 @@
 
 			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;
+		case DATABASE:
 		default:
 			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:
+		default:
 			echo $v_content;
 	}
-- 
1.9.3 (Apple Git-50)

Relationships

related to 0011952 closeddhx Arbitrary inline attachment rendering could lead to cross-domain scripting or other browser attacks 
child of 0017362 closeddregad Multiple vulnerabilities in MantisBT 

Activities

dregad

dregad

2014-11-28 20:00

developer   ~0041939

Last edited: 2014-11-28 20:01

Hi Matthias,

Please see attached a proposed patch. Because of my limited understanding of the problem and how the vulnerability can be exploited, I'm not really sure this is the best solution to the problem - or even the right one... It kind of feels like a hack, but as far as I can tell it fixes the issue.

Looking forward to your feedback.

vboctor

vboctor

2014-11-28 20:43

manager   ~0041940

@dregad.

0001-Fix-XSS-in-file-uploads-without-spacing.patch
This is your patch without the formatting changes.

0001-Fix-XSS-in-file-uploads-with-small-update.patch
This is the patch without the formatting changes + a change to make sure the check for flash file is applied independent of how the content type was overriden.

You could also add the flash extensions to $g_file_download_content_type_overrides to make sure they are detected correctly. Not sure if this is needed.

Do you have similar issues with attached html files that have embedded javascript?

avlidienbrunn

avlidienbrunn

2014-11-29 06:19

reporter   ~0041941

So, after some digging and refreshing my memories about this code, I do think that this patch will solve this specific issue, but I suggest trying to solve it on a more fundamental level.

There is a whitelist in the upload part to determine which extension should be allowed to be shown inline (file_api.php, file_get_visible_attachments, "in_array( $t_ext, $t_preview_text_ext, true )").

My suggestion is also having a whitelist of allowed Content-Type's for inline view. Maybe make that check under the switch() in the bottom of file_download.php to avoid duplicate code in the switch(). The problem is serving dangerous Content-Type's inline and only allowing a specific set of image Content-Types would solve this.

This would remove the issue I have presented and also another issue where a person can upload a file of any MIME, and then access it inline with an inline_token from another uploaded file, since the file_show_inline_token's aren't connected to a specific file.

Example, I uploaded a HTML file and here it is inline: https://www.mantisbt.org/bugs/file_download.php?file_id=5334&amp;type=bug&amp;show_inline=1&amp;file_show_inline_token=20141129oz5fKos-pB-1qPPKaqhPa8XP95kS15G4

Last, I suggest adding this to the comment about "X-Content-Type-Options: nosniff": http://50.56.33.56/blog/?p=242, Flash also does Content-Type sniffing which is disabled with that header.

PS. sorry for using this issue as my test env DS.

dregad

dregad

2014-11-29 10:40

developer   ~0041942

Last edited: 2014-11-29 11:32

@vboctor Thanks for looking at this.

0001-Fix-XSS-in-file-uploads-without-spacing.patch
0001-Fix-XSS-in-file-uploads-with-small-update.patch

Looks like you uploaded the same patch twice. I'll fix the whitespace.

You could also add the flash extensions to $g_file_download_content_type_overrides
to make sure they are detected correctly. Not sure if this is needed.

I don't think this is really useful in our case, because the flash file's extension will by definition be one of the allowed extensions in $g_preview_image_extensions (or possibly $g_preview_text_extensions as well, didn't test).

The root cause of the problem IMO is that we determine what can be viewed inline based on the file's extension instead of its MIME type.

In other words, as avlidienbrunn rightly said in 0017874:0041941, my fix is just a quick workaround and we should look into patching this in a better way. Since we have a fix now, I propose to go ahead with it for 1.2.18 and open a follow-up issue for the "proper" fix.

OK with that approach ?

Do you have similar issues with attached html files that have embedded javascript?

I don't think so, because html is not allowed inline by default. That said, if someone were to add 'html' to $g_preview_text_extensions, all bets are off. We can't really protect people from their own stupidity ;-)

@avlidienbrunn

Many thanks for your review and recommendations.

access it inline with an inline_token from another uploaded file,
since the file_show_inline_token's aren't connected to a specific file.

Nice catch ! I've open #17931 to track this.

Note that the tokens are user-specific though, so the links you posted would only work for you (and only as long as your session remains valid) and for a maximum of 3 days (tokens are deleted after that).

Last, I suggest adding this to the comment about "X-Content-Type-Options: nosniff":

Done. See attached 0002-Improve-comment-for-nosniff-header.patch

dregad

dregad

2014-11-29 11:03

developer   ~0041943

PS. sorry for using this issue as my test env DS.

No problem. Let me know if any of the test files you uploaded have any value or if I can simply delete them.

avlidienbrunn

avlidienbrunn

2014-11-29 13:41

reporter   ~0041945

You can remove my test files

vboctor

vboctor

2014-11-29 14:16

manager   ~0041946

@dregad, I've uploaded the second patch again 0001-Fix-XSS-in-file-uploads-again.patch -- this patch moves the check for flash after all content type assignments are done.

Your plan with going agead makes sense.

dregad

dregad

2014-12-01 02:28

developer   ~0041950

CVE request sent http://thread.gmane.org/gmane.comp.security.oss.general/14956

Related Changesets

MantisBT: master-1.2.x 9fb8cf36

2014-11-28 14:51

dregad


Details Diff
Fix 0017874: 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.
Affected Issues
0017874
mod - file_download.php Diff File

MantisBT: master 26f209a2

2014-11-28 14:51

dregad


Details Diff
Fix 0017874: 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.
Affected Issues
0017874
mod - file_download.php Diff File

MantisBT: master-1.2.x a552b37b

2014-11-29 05:50

dregad


Details Diff
Improve comment for 'nosniff' header

- Reworded the part about IE8 second-guessing content type
- Added a note about Flash, as per Mathias Karlsson's recommendation in
issue 0017874
Affected Issues
0017874
mod - file_download.php Diff File

MantisBT: master dfe664a1

2014-11-29 05:50

dregad


Details Diff
Improve comment for 'nosniff' header

- Reworded the part about IE8 second-guessing content type
- Added a note about Flash, as per Mathias Karlsson's recommendation in
issue 0017874
Affected Issues
0017874
mod - core/http_api.php Diff File
mod - css/common_config.php Diff File
mod - css/status_config.php Diff File
mod - file_download.php Diff File
mod - javascript_config.php Diff File
mod - javascript_translations.php Diff File