Replace download.php src by direct src for inline images

[TerryE]

Not sure whether this is a 3.1 or 3.2 issue, but I manage the OpenOffice.org user forums and we have roughly ½-1M page-views per week, so I try to keep an eye on performance issues. However, this one is one which will hits the smaller sites with occasional visitors.

At the moment any user-related image references use reference back to a php script. For example, on the viewtopic.php page user avatars are image tags with an src="./download/file.php?avatar=XXXX" and similarly for any images embedded in posts. This mean that when a user browser needs to download an image it must call a php script to do so. The Wikimedia engine uses a similar strategy of image management with the image meta-data in the database, but in this case the location is unwrapped and exposed as a direct src ref to the actual static file in the image store. I think that we should adopt a similar strategy for phpBB.

Why?
There are pros and cons to the scripted vs. direct static reference approach. For larger sites running PHP under mod_apache with a PHP accelerator such as Xcache or APC, the performance issues are probably a wash. However, for smaller sites running on an ISPs shared web service (which will be the vast majority of your forums), they will be running the PHP scripts under CGI/suexec or suPHP and without the benefit of a code cache. For such sites, there is a considerably difference in response and throughput for static images which are handled by the directly Apache core and those preprocessed through a PHP script.

I know that download.php does some validation logic and sets various HTTP headers to assist in caching, but the Apache core sets Etag header by default and most ISPs (and apps through .htaccess directive) also set the mod_expires directives to do likewise for static image file types, so this header logic in the download.php is really redundant.

I also realise that the advantage of moving the validation logic into download.php is that this is lazily evaluated only when the browser decides to load the image (and not if it is cached in the browser). However, this code is light compared to the overall phpBB start-up overhead, so moving it into a call in viewtopic which returns the static src reference would still in most cases result in a net processing saving.

Based on a sample of a million page request from our forums, we have:

• View Topic -- 71%
• ATOM feeds --9%
• View Forum -- 8%
• Home Page -- 4%
• UCP (mostly logon) 2%
• Search -- 2%
• Posting -- 1%
• All the other functions -- 3%

and for this 100% page views we have an extra 22% style loads and 65% download/file, so uncached style and image hits almost equal dynamic page requests. If these percentages are typical, then moving these to static references would almost double performance for shared-service hosted sites.

[Meis2M]

very good idea.. i like it

[igorw]

download.php is necessary for protecting against a wide range of vulnerabilities that generally occur with uploading files.

See also: viewtopic.php?p=211923#p211923

The performance overhead can be reduced though, by using apache's X-Sendfile, which we actually have planned: http://tracker.phpbb.com/browse/PHPBB3-9790

[TerryE]

@igorw, I am suggesting a optimisation on two specific cases: (i) avatar images and (ii) images embedded in posts, where the image file types are PNG, JPG or GIFs (which are 95+% of image references on my site). Your response on Re: [Patch] Direct (php-less) attachment downloads appear authoritative but aren't applicable in these cases.

• How are IE mime sniffing, flash's cross domain and java GIFAR attacks relevant to these static image types?
• MIME header are already processed by mod_mime (and equiv functionality in IIS), so the concept of "enforcing" MIME types (for png, jpg and gifs) within the application is quite redundant. Also the reason that I quote "enforcing" is that the phpBB doesn't enforce MIME types in the sense of mod_mime_magic but simply allocates MIME type according to similar rules to mod_mime.
• Your reference to avoiding "potential LFI by keeping file names secret" confuses me. What does filename transparency of image files have to do with Local File Inclusion attacks??

As I said, Wikimedia uses this approach and I would generally regard WP as a more locked-down and proven (in security terms) than phpBB. Use of transparent filenames for static images isn't per se a security threat. What you really seem to be saying is that what could be a 50% performance hit for shared service forums is acceptable to prevent an a vulnerability that has nothing to do with this suggested change.

Also X-Sendfile is irrelevent to the image activation, rundown and PHP script compilation overheads in a LAMP + suPHP type service offering that most phpBB forums use.

[Kellanved]

How are IE mime sniffing, flash's cross domain and java GIFAR attacks relevant to these static image types?

Sadly, all of these attacks use static images as their vector.

MIME header are already processed by mod_mime (and equiv functionality in IIS), so the concept of "enforcing" MIME types (for png, jpg and gifs) within the application is quite redundant. Also the reason that I quote "enforcing" is that the phpBB doesn't enforce MIME types in the sense of mod_mime_magic but simply allocates MIME type according to similar rules to mod_mime.

That'S completely true for people in control of their servers, however a large group of users is on shared hosting. Also, please note that both IIS and apache use incorrect headers in their default configuration.

Your reference to avoiding "potential LFI by keeping file names secret" confuses me. What does filename transparency of image files have to do with Local File Inclusion attacks??

LFI attacks are usually exploited using a "static image" with a php payload.

As I said, Wikimedia uses this approach and I would generally regard WP as a more locked-down and proven (in security terms) than phpBB.

Wikimedia had its share of problems with image uploads and has come up with very elaborate solutions. (look at the dev blog).

Use of transparent filenames for static images isn't per se a security threat. What you really seem to be saying is that what could be a 50% performance hit for shared service forums is acceptable to prevent an a vulnerability that has nothing to do with this suggested change.

Personally, I would use varnish to deliver the images, at least as long as there is no access control required.

The bottom line is: uploaded images are a possible attack vector and should under no circumstances be directly accessible, unless the server is configured to handle things properly without the help of php. There are IMHO two ways to get where you want to be:

• Strict validation of images on upload: reject all images containing php, zip (gifar), javascript, html etc. Note that images can be valid, even if they have such a payload - comments, stuff after the end of the image data, etc.. This is the wikimedia approach (phpBB also does a bit of scanning, but not quite enough).
• Remake all images after upload - i.e. run them through imagemagic (or GD) to create a new file, losing all nasty surprises. This is the facebook approach.

[TerryE]

OK, so what we are talking about as far as I can see (as far as java GIFAR attacks et al) are not attacks on the phpBB site itself, though the acronym LFI originally referred to exploitation of server-side loopholes to do with sloppy PHP scripting, but rather to exploitation of a separate set of vulnerabilities client-side -- particularly in obsolete MSIE versions and some plugins. Is LFI really the correct description here? What I still can't see is why wrapping such an attack in a files/download.php will do anything to prevent this.

Kellanved's suggestion of washing image uploads using GD seems a very sound approach. On my site image uploads account for less than 0.01% of web requests (captcha based registration which uses GD is at least an order of magnitude more than this). I don't know of any ISP shared web service which doesn't include GD, so the flexibility and performance impacts of this as a default option would be minimal.

I keep coming back to the issue of performance: if we are looking at large percentage improvements for the "personal service" forum space -- which dominates phpBB installs if not overall web transactions, then its something that we shouldn't dismiss lightly.

[ToonArmy]

I don't know of any ISP shared web service which doesn't include GD, so the flexibility and performance impacts of this as a default option would be minimal.

They exist, and there are others with worse restrictions for example realpath() returning an empty string for any input.

[TerryE]

If the concern here is injection attacks against client browsers via the site, then shouldn't you ban image tags?

[igorw]

Is LFI really the correct description here?

You are right, LFI and client-side attacks are two completely different issues. The LFI is of course only potential, there would need to be an actual LFI vulnerability on the server for one to exploit this, a wide range of PHP applications however do have such holes.

What I still can't see is why wrapping such an attack in a files/download.php will do anything to prevent this.

It makes sure the file is served with content-disposition attachment for IE < 8 (preventing mime sniffing), denies requests using CONTENT_TYPE equals 'application/x-java-archive' and user agents containing 'Java' (preventing GIFAR), and the serving file being in a separate folder (download/file.php instead of the original download.php) to prevent flash cross-domain attacks.

It may be possible to drop some of these measures in the future, after all it's been over 2 years since the GIFAR and flash attacks were patched (afaik). The amount of people still using IE 6 and 7 is however a major issue.

[bantu]

[...] What does filename transparency of image files have to do with Local File Inclusion attacks??

Let's assume someone is running at least two applications as the same user (which is probably the default on shared hosting).

• Application A: vulnerable to LFI, no ability to upload files.
• Application B (phpBB): Ability to upload files.

When you now expose the local filename in Application B to the user, you allow an LFI attack against Application A.

Also, if A == B, which means that phpBB itself is vulnerable to LFI, it would not be possible to attack phpBB because attachment filenames are randomly generated and avatar names are prefixed with a random string.