Page MenuHomePhabricator

SpecialListfiles.php throws a fatal error when a spooky file File: exists (for one reason or another)
Closed, ResolvedPublic

Description

Summary: if for one reason or another a spooky entry exists in table "image" (file with a blank name), the sort function http://server/wiki/index.php?title=Special:ListFiles&sort=img_name&limit=500 throws a FATAL ERROR as explained. This should be fixed for security reasons.

I noticed a buggy behaviour of $IP/includes/specials/SpecialListfiles.php -- only under certain circumstances which require code review:

IF a spooky "file" exists in the database table "image" - in my case this was a consequence from a failed/aborted file upload in a very old REL_1.4 MediaWiki from 2005 ...

THEN when clicking on the page header NAME on the file list on Special:Listfiles because I wanted to change the sort order

I received a reproducible Fatal error: Call to a member function getURL() on a non-object in /.../includes/specials/SpecialListfiles.php on line 138 (MediaWiki 1.15.1)

Fixed this by changing line 138 from
$url = $image->getURL();
to
if ($value !== 0 && $value != '') $url = $image->getURL();

and found the reason: a file in the table with an empty filename (not ok) and 0 Byte (not ok), upload date (ok.), uploader name (ok.)

Summary: if for one reason or another a spooky entry exists in table "image" (file with a blank name), the sort function http://server/wiki/index.php?title=Special:ListFiles&sort=img_name&limit=500 throws a FATAL ERROR as explained. This should be fixed for security reasons.

P.S. I deleted the spooky file by

  • manually changing the emtpy filename in the database to a dummy name dummy.jpg
  • over-uploading a second file dummy.jpg through the wiki
  • deleting the file through the wiki using action=delete

I intentionally assigned 1.16-svn because reviewing the SVN I found that the bug might still be present.


Version: 1.16.x
Severity: major

Details

Reference
bz22227

Event Timeline

bzimport raised the priority of this task from to High.Nov 21 2014, 10:53 PM
bzimport set Reference to bz22227.

wfLocalFile is supposed to "Return a valid placeholder object if the file does
not exist." :/
Something is going wrong inside the LocalRepo.

(In reply to comment #1)

wfLocalFile is supposed to "Return a valid placeholder object if the file does
not exist." :/

Can you reproduce the reported problem (e.g. by manually emptying a filename in table image and setting byte count to 0) ?

(In reply to comment #1)

wfLocalFile is supposed to "Return a valid placeholder object if the file does
not exist." :/
Something is going wrong inside the LocalRepo.

Except it lies. If it's not fed a valid title (in this case, an empty string), it will return null before trying to construct a file object. See FileRepo::newFile()

Might be worth making a FakeFile class to return in that scenario.

(In reply to comment #3)

(In reply to comment #1)

wfLocalFile is supposed to "Return a valid placeholder object if the file does
not exist." :/
Something is going wrong inside the LocalRepo.

Except it lies. If it's not fed a valid title (in this case, an empty string),
it will return null before trying to construct a file object. See
FileRepo::newFile()

Okay, and all callers receiving the null "object" shall not throw a fatal error

Fixed in r87347 for Special:ListFiles.

Fix in r87347 doesn't look right; it's checking the return value of Title::makeTitle for truthiness, but Title::makeTitle doesn't validate and never returns something that evaluates to false.