Page MenuHomePhabricator

blacklist extension handling is broken
Closed, ResolvedPublic

Description

The global file-extension blacklist uses two different strings to identify the real extension of the file (the last .xxx at the end of the filename) and the extension that throws an error (it inexplicably checks for all 'extension' substrings in the filename, so that hello.py.zip.txt will have py, txt, and zip all checked for a blacklist match).

This should perhaps be two bugs; the most immediate is that the error thrown on the above file could be ".txt is not an allowed extension" when it is actually choking on ".py"

The second is that I can't see why extensions other than the final one should be blacklisted in the first place.


Version: 1.20.x
Severity: minor

Details

Reference
bz11142

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 9:54 PM
bzimport set Reference to bz11142.
bzimport added a subscriber: Unknown Object (MLST).

Multiple extensions are checked on upload because the web server also checks multiple extensions when it's determining the type of a file to serve out.

For instance, on an Apache server where the '.ogg' extension hasn't been configured (common on 1.3.x), a file 'Foo.php.ogg' may instead be recognized as a PHP script due to its recognized .php extension, and executed through the PHP module.

That has obvious security implications. :)

We should indeed be reporting the matching extension rather than the last one, though.

  • Bug 11529 has been marked as a duplicate of this bug. ***
  • Bug 13088 has been marked as a duplicate of this bug. ***
  • Bug 15004 has been marked as a duplicate of this bug. ***

EN.WP.ST47 wrote:

*** This bug has been marked as a duplicate of bug 28609 ***

Reopening. Does not seem to be a dupe of bug 28609

It seems that someone has already fixed this bug.

On an attempt to upload a file called bug 11142.txt.png; he.wiki and en.wiki alerts that:

".txt" is not a permitted file type. Permitted file types are png, gif, jpg, jpeg, xcf, pdf, mid, ogg, ogv, svg, djvu, tiff, tif, oga.

However, on commons I get an error message saying: "This type of file is banned". Hence, this is a problem with the upload wizard.

After some more research it seems that this isn't necessarily a bug in the upload wizard rather than a bug in the API.

It looks like someone had fixed this in the upload form but hadn't fixed this in the api.

Created attachment 9231
Patch so that the api will give more useful information when uploading a blocked file type

As it turns out SpecialUpload.php and ApiUpload.php are going in two different ways.

Assuming that the user is trying to upload a blacklisted filetype, SpecialUpload will throw the error message in 'filetype-banned-type' while ApiUpload.php will throw the error message in 'filetype-banned'. 'filetype-banned' is less informative as it doesn't tell the user what was the extension that was blocked and does not support plural text.

I could not make the API uploader work with the plural template and I suspect that it's not up to it anyway. In such a case there are few ways we can deal with it:

  1. Assume singular text and show the most inner (innerest?) file extension that was blocked.
  2. Assume plural text and implode all extensions that were blocked.
  3. Assume plural text and show all the file types that are not allowed on that Wiki.

I chose option #3. If someone could show me how to identify plural text in the API uploader it should be fairly easy to output only the file extensions that were blocked in this file.

attachment api.patch ignored as obsolete

Created attachment 9232
A patch to fix the uploadwizrd

This patch adds the relevant to the uploadwizard so that it'll give some informative message to the user when a file is blocked due to blacklisted extension.

attachment UploadWizard.i18n.patch.php ignored as obsolete

Created attachment 9233
A patch to fix the uploadwizrd

A patch to fix the uploadwizrd

This patch adds the relevant to the uploadwizard so that it'll give some
informative message to the user when a file is blocked due to blacklisted
extension.

attachment UploadWizard.i18n.patch.php ignored as obsolete

Created attachment 9234
A patch to fix the uploadwizrd

This patch adds the relevant to the uploadwizard so that it'll give some
informative message to the user when a file is blocked due to blacklisted
extension.

(Sorry guys, it takes time to get used to how stuff works here).

attachment UploadWizard.i18n.patch.php ignored as obsolete

As I suspected, this was fixed in r80766 without also fixing the API upload.

neilk wrote:

Can I get a clarification here -- the bug is that the UploadWizard does not tell you which of the extensions caused the problem, if you somehow sneak a file in that is not allowed?

I can't even cause this to happen locally, can you give me an example configuration where it would happen?

Also, I don't think just changing the message to include a '$1' is going to fix this, you also need to change how the code works.

The way I see it, the uploadWizard should get this information from the API. Therefore, $1 should be enough for that.

The real problem is that the API does not provide this info.

There's also a patch for the API in this tread that could have give more usful information had not been sitting here for two months.

It is unlikely I'll have time in the near future.

sumanah wrote:

Brad, do you have time to do any review and updates here?

I wound up rewriting both patches: the API one to provide all the blacklisted extensions and a better error message, and the UploadWizard one because due to subsequent changes the old patch wouldn't work right at all.

The new patches are Gerrit change 16040 and Gerrit change 16041.

16040 was merged, 16041 is pending review.

sumanah wrote:

16041 is now merged so I presume this is fixed, and am marking as such. Thanks, Brad.