Can we change this patch to:

1) Drop the new configuration option (especially in 1.2.x)

2) Use the following code (modified where necessary) from file_download.php:

$t_content_type = '??? (some default value if the user has PHP 5.2 and hasn't got the fileinfo extension installed)';

if ( $finfo_available ) {

$t_file_info_type = $finfo->file( $t_local_disk_file );

            if ( $t_file_info_type !== false ) {
                $t_content_type = $t_file_info_type;
            }
        }

        header( 'Content-Type: ' . $t_content_type );
        if ( config_get( 'file_download_xsendfile_enabled' ) ) {
            $t_xsendfile_header_name = config_get( 'file_download_xsendfile_header_name' );
            header( $t_xsendfile_header_name . ': ' . $t_local_disk_file );
        } else {
            readfile( $t_local_disk_file );
        }

3) Add warnings to the new file_send function as well as plugin_file_include function notifying developers of the security implications of these functions. Specifically, things like the ability to serve up user-supplied HTML or Javascript and have it rendered in-browser (XSS), serving up any file from the disk (including MantisBT config_inc.php file), etc.

Happy to help.

David, thanks for reviewing and commenting. My initial attempts to use fileinfo were not succesful, but I will extract and reuse the code from file_download.php .

2) I've implemented the file_send function, but I'm back to square one with the JS and CSS files. They get detected incorrectly as

jquery-ui.css: ASCII C program text, with very long lines
jquery-ui-min.js: ASCII English text, with very long lines

the png files are detected correctly, but those never were problematic to start with.

3) plugin_file_include only serves plugin-supplied files, so the warning is not really neded.

What would you think of having the possibility to override the content-type only for files detected as text/something? The man page of the file command does state that

"File uses several algorithms that favor speed over accuracy, thus it can be misled about the contents of text files.

The support for text files (primarily for programming languages) is simplistic, inefficient and requires recompilation to update."

I don't think fileinfo will ever get css and javascript right.

Thanks Robert, and sorry for the delayed response.

I'd be happy to see an override for CSS/Javascipt, as long as the override applies only to files that were detected as text/plain (or whatever fileinfo is detecting them as) and have the correct file name extension.

Note: I am still concerned about returning content that is rendered in the browser. With Javascript, this could introduce arbitrary Javascript into the "trusted domain", thus leading to XSS attacks.

HTTP headers can be modified to forcefully tell the browser not to render content but instead treat it as a file download. This should really apply all the time, except in very specific, controlled and well thought out situations. For instance, text/plain is OK to show in the browser.

Images are also likely to be OK, but there is no guarantee of this when you factor in the possibility of image rendering security bugs in browsers. I added CSRF protection for inline image display (from attached images) in MantisBT a while ago to solve this potential problem (and hotlinking too).

David, to follow up on your comments:

1. I plan to introduce - for plugins only - a way of specialising the content type which is sent to the client given that the file is detected as text/* and the file extension is correct. I only see the need to do this for CSS and Javascript right now, not images. Since the file is served by the plugin I think that we do not need any extra security settings, as this is a trusted application - at least by the server administrator.
2. In the related issue 0013439 I plan to introduce - for attachements only - a way of overriding the content type which is sent to the client. Similar to the javascript issue, .docx files will be detected as ZIP files , which is technically correct, but not very useful.

Please let me know if you see any problems with the above approach.

This is now fixed as outlined above for both text/ and image/ content types families.

Marking as 'acknowledged' not resolved/closed to track that change gets ported to master-2.0.x branch