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.

Reminder sent to jlatour

Jeroen, what do you think about this one?

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?

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.

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?

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

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?

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

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.

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

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

Proposed diff to file_api.php attached.

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.

Fixed in CVS.

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