Opened 12 years ago

Last modified 8 years ago

#51 closed defect (FIXED)

Handing of bad media types (html!)

Reported by: Elrond Owned by: Jakob Kramer
Priority: major Milestone: 0.0.5
Component: programming Keywords:
Cc: Parent Tickets:


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

Change History (12)

comment:1 by Christopher Allan Webber, 12 years ago

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.

comment:2 by Jakob Kramer, 12 years ago

Milestone: 0.0.3
Owner: set to Jakob Kramer
Status: NewIn Progress

comment:2 by Jakob Kramer, 12 years ago

[\ :sub:`gandaro/mediagoblin/gandaros-mediagoblin/commits/324-bad-media-types](`\ gandaro/mediagoblin/gandaros-mediagoblin/commits/324-bad-media-types)

comment:3 by Elrond, 12 years ago

Owner: changed from Jakob Kramer to Christopher Webber

1. Added `#361 </issues/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

comment:4 by Christopher Allan Webber, 12 years ago

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?

comment:5 by Elrond, 12 years ago

Owner: changed from Christopher Webber to Jakob Kramer
okay, first bug turns up in the security code (on irc, by

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.

comment:6 by Christopher Allan Webber, 12 years ago

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

comment:7 by Christopher Allan Webber, 12 years ago

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.

comment:8 by Christopher Allan Webber, 12 years ago

Targeting for 0.0.4

comment:9 by Will Kahn-Greene, 12 years ago

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

comment:10 by Christopher Allan Webber, 12 years ago

Component: Programming
Status: In ProgressClosed
Good news, with the recent conversion overhaul this is finished :)

comment:11 by Will Kahn-Greene, 11 years ago

The original url for this bug was .
#76: related

Note: See TracTickets for help on using tickets.