Page MenuHomePhabricator

MimeMagic: ZIP types not properly detected
Closed, ResolvedPublic

Description

ZIP-type file with fake extension.

In MimeMagic.php, there is a function that is supposed to detect zips - ::doGuessMimeType() - and zip types - ::detectZipType(). However, it fails when supplying small files like attached. Neither small ZIPs are detected nor other kind of zip-types.

So I did some debugging and found that $tail in ::doGuessMimeType() appears to be empty when the file size is below about 8192 bytes. Lowering the number of bytes from the end of the file to set the cursor to in the call to fseek to the actual file size "fixes" the issue.

Reproducible:

  • Production: Always.
    • Evidence: When uploading c.jpg to Commons, the following response is issued: File extension ".jpg" does not match the detected MIME type of the file (unknown/unknown).
    • Evidence: When uploading c_big.jpg to Commons, the following response is issued: File extension ".jpg" does not match the detected MIME type of the file (application/vnd.oasis.opendocument.spreadsheet).
  • Debug environment with PHP 5.4.20 (apache2handler): Always.
    • Evidence: Debug logging and inspecting variables using xDebug and Eclipse.

Version: 1.24rc
Severity: normal

Attached:

Details

Reference
bz66428

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:15 AM
bzimport set Reference to bz66428.

Created attachment 15613
This file will be detected by MediaWiki because $tail is not empty.

Attached:

Cc ing csteipp. I suspect this might be exploitable...

Thanks Rainer / Brian, I'm going to put this into security while I play with it. I agree, that seems like it could be used for malicious purposes, but hopefully not.

Was it wrong to publicly submit a patch with I182128c6958244 ? How could I privately submit to Gerrit?

(In reply to Rainer Rillke @commons.wikimedia from comment #4)

Was it wrong to publicly submit a patch with I182128c6958244 ? How could I
privately submit to Gerrit?

You can't. If its a super critical security issue, people usually put the patches on bugzilla I believe.

Adding -D (draft) to git review will keep it from being advertised, but gerrit doesn't have a good way to deal with private patches.

This isn't horrible since the detection is still done when finfo is installed, or the wiki shells out for the detection. Also, since it's unknown/unknown, the file isn't allowed to be uploaded (unless $wgVerifyMimeType, in which case there are plenty of other ways to upload malicious files).

So I'm not worried about this being public, although I think it's good hardening. But unless someone finds a way to get the upload to succeed with a relatively secure config, I don't think we need to be overly private about this bug.

Making this public again. I think we should also backport the fix, since it's a good security hardening measure.

Change 139070 had a related patch set uploaded by Rillke:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139070

Change 139073 had a related patch set uploaded by Rillke:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139073

Change 139074 had a related patch set uploaded by Rillke:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139074

Release managers would be notified if Backport_to_Stable is set to "?", the docs say. Does this still work?

(In reply to Rainer Rillke @commons.wikimedia from comment #11)

Release managers would be notified if Backport_to_Stable is set to "?", the
docs say. Does this still work?

The docs at https://www.mediawiki.org/wiki/Backporting_fixes state this and tarball release managers are aware of the flag; but there was no automatic bugmail notification sent to them so far, sigh. I've fixed this now.

There is no automatic method that I know of. We're still checking these manually.

Cherry-picking, as was done here, is the best route to go because then it will be in the queue during our monthly maintenance releases. If that isn't possible, then adding us as CC on the bug at least sends us email.

Change 139070 merged by Mglaser:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139070

Change 139074 merged by Mglaser:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139074

Change 139073 merged by Mglaser:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/139073

Updating the flag from ? to + now that this has been backported.

Change 142041 had a related patch set uploaded by Mglaser:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/142041

Change 142041 merged by Mglaser:
MimeMagic: Don't seek before BOF

https://gerrit.wikimedia.org/r/142041

Gilles raised the priority of this task from High to Unbreak Now!.Dec 4 2014, 10:11 AM
Gilles moved this task from Untriaged to Done on the Multimedia board.
Gilles lowered the priority of this task from Unbreak Now! to High.Dec 4 2014, 11:22 AM