View Issue Details

IDProjectCategoryView StatusLast Update
0003540mantisbtsecuritypublic2006-10-09 11:54
ReporterTomVogt Assigned Tothraxisp  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Fixed in Version0.19.0rc1 
Summary0003540: Arbitrary code execution through uploads
Description

If the upload path is in the webroot, .php files can be uploaded as attachments to bug reports and will be executed when accessed.

TagsNo tags attached.

Relationships

related to 0003349 closedthraxisp File upload suggestion 
related to 0001137 closedthraxisp using per bug subdirectories for file uploading 
related to 0004435 closedvboctor Auto-preview of attached images is broken 
related to 0009454 new upload a file with his name or a hash conversion ? 
child of 0003987 closedvboctor Mantis 0.19.0 Release 

Activities

vboctor

vboctor

2004-02-08 03:41

manager   ~0004982

Lets put the best approach for fixing this. I will put some ideas and you are welcome to add others:

  • Make "php" a disallowed extension by default (we can even displayed other stuff like asp, aspx, ???)
  • Add a warning in the documentation and/or next to the upload path that it is recommended that this directory be outside the webroot.
vboctor

vboctor

2004-02-08 03:42

manager   ~0004983

Reminder sent to jlatour

Jeroen, what do you think about this one?

jlatour

jlatour

2004-02-08 03:49

reporter   ~0004985

You can make PHP a disallowed extension, but it will still allow people to read the files of any bug, or execute ASP/JSP/etc. code.

So I think perhaps warn when it's in the document root with admin/check.php? We could add a .htaccess file, but we don't package an attachment directory, do we?

TomVogt

TomVogt

2004-02-08 05:18

reporter   ~0004987

At least in 0.17, putting the upload path outside the webroot doesn't work well, as stuff isn't shown anymore.

But it would definitely be the best solution, with a short php script that just passes the content through.

I am in contact with the guy who compromised my database using this problem, so if there are any more details required, I can probably ask him.

jlatour

jlatour

2004-08-06 12:21

reporter   ~0006729

Is there anything we can or are going to do about this, apart from perhaps a note in the documentation or a check in check.php?

grangeway

grangeway

2004-08-06 18:59

reporter   ~0006737

well, a link to download an attachment looks like : /file_download.php?file_id=320&type=bug

So, neither the location or the name of the file are required to be known at any time. e.g. why not store an uploaded test.php file as '320' or even md5('test.php') as the filename ?

As the files should only be grabbed via file_download.php (as this checks access), seems like a reasonable option imo

jlatour

jlatour

2004-08-07 03:30

reporter   ~0006741

Yes, I think that'll work, at least to prevent this arbitrary code execution problem. Good thinking!

There still remains the problem of files being readable to anyone though. Do we still need to add a check for that?

grangeway

grangeway

2004-08-07 05:12

reporter   ~0006744

Possibly cover the 'default' case?
e.g. Create a directory in mantisbt called /uploads.
Stick an index.php file in there that just contains a print_redirect to ../

And modify whatever the hash is so it takes an unknown value when hashing (e.g. rand(0,100000) + filename). Looking at the mantis_bug_file_table, which is currently bug_id, title,description,diskfile,filename,folder,filesize,file_type,date_added,content where diskfile/filename/folder/file_type are all 250 chars we could probably optimise this in the process (depending on whether there's a reason for all those fields)
Presumably 'folder' refers to the place the file is stored on the server? and 'diskfiler' is folder + filename ? (i've not looked at the code).

In the above scenario, diskfile would become 32 bytes (e.g. a md5 hash), folder would largely become pointless storing (as if the filenames were hashed, you wouldn't be able to download them directly (problem?), so the folder structure that we used to store files would become largely irrelevant to the user.

Questions being:
a) do users need/want to be able to download files directly i.e. would using hashes for names break things for them.
b) could you remove 'folder' from the table and replace with some other logical structure e.g. uploads/bugNum/md5hash (bearing in mind we could 'protect' a single level easily by creating an index.php file, but multiple levels would be harder to protect. Or even uploads/project1/md5hash

jlatour

jlatour

2004-08-07 08:27

reporter   ~0006747

Yeah, I guess we could do that, but I still think we should add a check for this. Might just prevent a few more leaks.

The file database probably has to be looked at. And if we're going to reduce the size of the disk filename field, we'll have to convert existing filenames.

grangeway

grangeway

2004-08-07 08:55

reporter   ~0006751

i've linked this to 'possible to include in next release (after 0.19'

jlatour

jlatour

2004-08-07 09:47

reporter   ~0006764

Actually, this sounds more like something for 0.19.0 to me.

2004-08-25 20:08

 

filename.diff (3,596 bytes)   
Index: core/file_api.php
===================================================================
RCS file: /cvsroot/mantisbt/mantisbt/core/file_api.php,v
retrieving revision 1.52
diff -u -r1.52 file_api.php
--- core/file_api.php	21 Aug 2004 00:13:52 -0000	1.52
+++ core/file_api.php	26 Aug 2004 00:53:59 -0000
@@ -423,6 +423,47 @@
 	}
 
 	# --------------------
+	# Generate a string to use as the identifier for the file
+	# It is not guaranteed to be unique and should be checked
+	# The string returned should be 32 characters in length
+	function file_generate_name( $p_seed ) {
+		$t_val = md5( $p_seed . time() );
+
+		return substr( $t_val, 0, 32 );
+	}
+
+	# --------------------
+	# Generate a UNIQUE string to use as the identifier for the file
+	# The string returned should be 64 characters in length
+	function file_generate_unique_name( $p_seed , $p_filepath ) {
+		do {
+			$t_string = file_generate_name( $p_seed );
+		} while ( !file_is_name_unique( $t_string , $p_filepath ) );
+
+		return $t_string;
+	}
+
+	# --------------------
+	# Return true if the file name identifier is unique, false otherwise
+	function file_is_name_unique( $p_name , $p_filepath ) {
+		$t_file_table = config_get( 'mantis_bug_file_table' );
+
+		$c_name = db_prepare_string( $p_filepath . $p_name );
+
+		$query = "SELECT COUNT(*)
+				  FROM $t_file_table
+				  WHERE diskfile='$c_name'";
+		$result = db_query( $query );
+		$t_count = db_result( $result );
+
+		if ( $t_count > 0 ) {
+			return false;
+		} else {
+			return true;
+		}
+	}
+
+	# --------------------
 	function file_add( $p_bug_id, $p_tmp_file, $p_file_name, $p_file_type='' ) {
 		$c_bug_id		= db_prepare_int( $p_bug_id );
 		$c_file_type	= db_prepare_string( $p_file_type );
@@ -440,9 +481,10 @@
 			# prepare variables for insertion
 			$t_file_path = project_get_field( $t_project_id, 'file_path' );
 			$c_file_path = db_prepare_string( $t_file_path );
+			$c_new_file_name = db_prepare_string( $p_file_name );
 
-			$t_new_file_name = $t_bug_id . '-' . $p_file_name;
-			$c_new_file_name = db_prepare_string( $t_new_file_name );
+			$t_disk_file_name = $t_file_path . file_generate_unique_name( $t_bug_id . '-' . $p_file_name, $t_file_path );
+			$c_disk_file_name = db_prepare_string( $t_disk_file_name );
 
 			if ( is_readable ( $p_tmp_file ) ) { 
 				$t_file_size = filesize( $p_tmp_file );
@@ -462,15 +504,15 @@
 				case DISK:
 					file_ensure_valid_upload_path( $t_file_path );
 
-					if ( !file_exists( $t_file_path . $t_new_file_name ) ) {
+					if ( !file_exists( $t_disk_file_name ) ) {
 						if ( FTP == $t_method ) {
 							$conn_id = file_ftp_connect();
-							file_ftp_put ( $conn_id, $t_new_file_name, $p_tmp_file );
+							file_ftp_put ( $conn_id, $t_disk_file_name, $p_tmp_file );
 							file_ftp_disconnect ( $conn_id );
 						}
 
 						umask( 0333 );  # make read only
-						move_uploaded_file( $p_tmp_file, $t_file_path . $t_new_file_name );
+						move_uploaded_file( $p_tmp_file, $t_disk_file_name );
 
 						$c_content = '';
 					} else {
@@ -487,7 +529,7 @@
 			$query = "INSERT INTO $t_bug_file_table
 						(bug_id, title, description, diskfile, filename, folder, filesize, file_type, date_added, content)
 					  VALUES
-						($c_bug_id, '', '', '$c_file_path$c_new_file_name', '$c_new_file_name', '$c_file_path', $c_file_size, '$c_file_type', " . db_now() .", '$c_content')";
+						($c_bug_id, '', '', '$c_disk_file_name', '$c_new_file_name', '$c_file_path', $c_file_size, '$c_file_type', " . db_now() .", '$c_content')";
 			db_query( $query );
 
 			# updated the last_updated date
filename.diff (3,596 bytes)   
thraxisp

thraxisp

2004-08-25 20:10

reporter   ~0007174

Proposed diff to file_api.php attached.

grangeway

grangeway

2004-08-26 17:40

reporter   ~0007199

so for this:

  1. block file extensions [not needed with hash code]
  2. store filenames as a hash [ done ]
  3. Optimise SQL table [not done, future / can't do]
  4. Create a default 'uploads' directory -> with index.php and .htaccess file. [not done]

Would seem to be accurate of where we are.

thraxisp

thraxisp

2004-08-26 19:45

reporter   ~0007213

Fixed in CVS.

The hashed filename is stored in the 'diskfile' column.

Issue History

Date Modified Username Field Change
2004-02-07 13:53 TomVogt New Issue
2004-02-08 03:41 vboctor Note Added: 0004982
2004-02-08 03:42 vboctor Note Added: 0004983
2004-02-08 03:49 jlatour Note Added: 0004985
2004-02-08 05:18 TomVogt Note Added: 0004987
2004-08-06 12:21 jlatour Note Added: 0006729
2004-08-06 12:21 jlatour Status new => acknowledged
2004-08-06 18:59 grangeway Note Added: 0006737
2004-08-07 03:30 jlatour Note Added: 0006741
2004-08-07 03:30 jlatour Status acknowledged => confirmed
2004-08-07 05:12 grangeway Note Added: 0006744
2004-08-07 08:27 jlatour Note Added: 0006747
2004-08-07 08:55 grangeway Relationship added child of 0004181
2004-08-07 08:55 grangeway Note Added: 0006751
2004-08-07 09:46 jlatour Relationship deleted 0004181
2004-08-07 09:47 jlatour Relationship added child of 0003987
2004-08-07 09:47 jlatour Note Added: 0006764
2004-08-25 20:08 thraxisp File Added: filename.diff
2004-08-25 20:10 thraxisp Note Added: 0007174
2004-08-25 20:11 thraxisp Assigned To => thraxisp
2004-08-25 20:11 thraxisp Status confirmed => assigned
2004-08-26 17:40 grangeway Note Added: 0007199
2004-08-26 19:35 thraxisp Relationship added related to 0003349
2004-08-26 19:37 thraxisp Relationship added related to 0001137
2004-08-26 19:45 thraxisp Status assigned => resolved
2004-08-26 19:45 thraxisp Fixed in Version => 0.19.0rc1
2004-08-26 19:45 thraxisp Resolution open => fixed
2004-08-26 19:45 thraxisp Note Added: 0007213
2004-08-29 01:29 vboctor Status resolved => closed
2004-08-30 06:25 vboctor Relationship added related to 0004435
2006-10-09 11:54 thraxisp View Status private => public
2008-07-30 22:59 thraxisp Relationship added related to 0009454