Handing of bad media types (html!)

[Elrond]

Currently you can try to upload a html file.

What happens currebtly? It sticks in the queue, because celery goes crazy on it. The good news: It does not end up in a public place.

What should NEVER, ever happen: The file being put in a public place. It's the best XSS attack to come up with.

Rating this high, because this needs to be right for security reasons.

[Christopher Allan Webber]

Yeah, I'm not sure how to best check / fix this in all scenarios. Right now we're just supporting images, so we only need to check that the thing is an image. We're doing that... PIL has to open the file, maybe actually checking that the file can open is the best way to make sure that it's a file of the appropriate type?

I'm not sure in the glorious future of multiple media types if we should add something to the processing interface that does a security check or not, but it's something to think about.

A related thing is that we need to have a way to track "results" of the processing and have a panel for users that can help them figure out why it failed. Right now celery does store this information, but I think this is in the scope of a different but related ticket.

[Jakob Kramer]

Fix: [https://gitorious.org/_{gandaro/mediagoblin/gandaros-mediagoblin/commits/324-bad-media-types](https://gitorious.org/}gandaro/mediagoblin/gandaros-mediagoblin/commits/324-bad-media-types)

[Elrond]

Hi,

1. Added #361 as related, as browser "upsniffing" (or whatever it's called) might be related to this bug.
2. Assigned to cwebber for review/merging. Jakob seems to be happy with his branch, I guess
3. Chris: Please review, merge, close, and/or assign back to Jakob.

[Christopher Allan Webber]

Hm, okay, I've merged that for now. The way to handle the mimetype stuff is right on, but loading up and actually checking the image I think should happen (and will happen) during the processing stage.

I think the ideal system is this two step process:

• Check the mime type for the media type that's being uploaded
• If conversion fails somehow or a problem is detected during the processing step, an appropriate error is filed that the application can know about and that can be displayed in some sort of "upload progress" panel.

In the glorious future of multiple media types, media types will whitelist the accepted mimetypes and will handle the failure stuff in their own processing functions, but in a standardized way.

I guess we need to handle conversion failures properly though, and we haven't thought about how to record that properly. Maybe we should start thinking about that now?

[Elrond]

Hi, okay, first bug turns up in the security code (on irc, by "iAlecs"):

guess_type from mimetypes returns a tuple, the code only wants the first element. So the code should probably read:

if not guess_type(posted_file.filename)[0] in ALLOWED:

Jakob: I have reassigned back to you for clearance.

[Christopher Allan Webber]

I've removed the PIL check and made the change Elrond suggested for now, and things seem to submit okay.

[Christopher Allan Webber]

So as for the actual processing detecting if there was a problem during conversion, we should probably update the state of the task if a failure happens and have a standard error code like CONVERSION_FAILED. So I think that will be round 2 of catching bad formats, that they're caught in the conversion failure cycle.

[Christopher Allan Webber]

Targeting for 0.0.4

[Will Kahn-Greene]

We release 0.0.4, so I'm bumping this to 0.0.5.

[Christopher Allan Webber]

Good news, with the recent conversion overhaul this is finished :)

[Will Kahn-Greene]

The original url for this bug was ​http://bugs.foocorp.net/issues/324 .
Relations:
#76: related