Wikimedia Bugzilla is closed!

I would like to add another suggestion: Show an extra warning on the
description-page of media in a potentially dangerous format (or rather for any
non-image, non-sound format). The message could read something like this:

 "This file may contain executable code that might damage your system.
 If you download this file and open it on your computer, potentially
 harmfult contents may be run. Please make sure you know what you
 are doing. The Wikimedia Foundation does not take any resposibility for
 the contents of this file or any harm it may do to your system."

This is not only aimed at macro-viruses hidden in doc-fiels etc (which the virus
scanner will hopefully find on upload), but maily at "trivial trojans" like
batch files containing a "format C:" or some such.

In that context, it meight also be good to block all files with the extensions
.exe, .bat, .cmd, .reg, .js as well as any files starting with a she-bang (#!),
regardles of the guessed mime-type. That is, there should be a list of forbidden
extensions in addition to a list of forbidden mime-types (or does that already
exist?), and an additional check for the she-bang. If you like, i could add that
to the sample code, but it's trivial. 


When discussing this on wikitech-l, Rowan Collins suggested the following:

That leads to an interesting thought: presumably, non-image files are
already blocked from being included "inline"; but what about the magic
[[Media:...]] links? I know it's pretty dumb to open a file that you
don't know what it is, but some people genuinely are, and it would be
nice to do everything we can to help them.  In other words, perhaps
it would make sense to have *three* whitelists:

1) Images: can be uploaded, linked to direct, and included inline
2) Other trusted files (e.g. sounds, videos): can be uploaded, linked
to directly with [[Media:]], but not included inline with [[Image:]]
3) Potentially harmful files (e.g. .zip, .doc): can be uploaded (if
they pass a virus scan), but not linked to directly; users are always
directed to the description page, where they are presented with a
warning message.

The Wikimedia Foundation's site (http://wikimediafoundation.org/) 
should allow uploading of any file immediately, since only trusted 
users can access it.

I can see three problems with virus scanning at this time:
* we like free software...but can we get a free virus scanner with 
decent definition updates?
* some people may mistakenly think that it's foolproof
* it will take a lot of processor time (expensive)

The second problem can be resolved with decent warnings, I suppose. 
If we are relying on external hosting then we don't want to waste 
processor time.

I don't like virus scanning...some are bound to slip through the net.

Be careful with that warning! Some old MS-DOS manuals contain some 
rubbish about users not being able to harm their computers whatever 
they did - people may not take that warning seriously, particularly 
in places where computers are just being introduced.

(In reply to comment #2)
> The Wikimedia Foundation's site (http://wikimediafoundation.org/) 
> should allow uploading of any file immediately, since only trusted 
> users can access it.

Maybe, maybe not. Trusted users can have conterminated files on their box too.

> * we like free software...but can we get a free virus scanner with 
> decent definition updates?

Yes, camav. Also, the code I designed is very felxible, it can easily interface
to any scanner that can be run from the command line.

> * some people may mistakenly think that it's foolproof

Not more so than before - there is no reason to put up a sign "all files are
scanned for viruses". The user will not notice the scanning unless a virus is found.

> * it will take a lot of processor time (expensive)

Not really. We see about one image per minute uploaded to the commons. Scanning
a big jpg (1.8 MB) on my box (1.9 GHz P4) takes about 1 sec of CPU time.

> I don't like virus scanning...some are bound to slip through the net.

Yes. But our alternatives are 1) to ave no net at all and b) to limit allowed
formats to a minimum. Note that there are virusses that can infeect JPGs, too
(they use a bug in the windows image rendering libs and effect all browsers).
Those are not detected right now.


I belive may suggestion should be implemented as soon as possible, maybe even
for 1.5; I already supplied code the implements the functionality, it should not
be too hard to integrate it into mediawiki. Maybe I'll try to do it mayself, but
i'm not sure when I get around to it.

The disability to supply sources for graphics is frustrating to many users who
would like to contribute SVG (we even have a renderer for that!), MIDI and
similar formats that can easily be edited by others, unlike bitmap images.

oops, sorry, that should be ClamAV - see http://www.clamav.net/

Created attachment 497 [details]
proof-of-concept patch implementing my suggestions. Please read the comment

Created a proof of concept patch. This is just for reference - it uses an ugly
bitfield logic, which has the advantage of being compatible iwth the current
1.5 DB schema. The Real Thing should however be based on three separate fields
in the database, for media class (image, drawing, audio, video, etc) and major
and minor mime type, as suggested by brion and tim.

This patch consists of three parts (which should probably go into separate
patches eventually): 

1) Changes to the verification code in SpecialUpload.php:
* improved mime detection / verification
* detection of javascript
* virus scanning

2) Changes to Image.php that allow to determine if a medium is fit to be shown
inline, to be linked to directly, or is non-trusted formats that could contain
malicious code (i.e. anything else).

3) Changes to ImagePage.php and Linker.php
* put a warning on the description page of non-trusted files
* disallow direct [[media:...]] links to non-trusted files

The patch also contains changes to DefaultSettings, Defines and Languages to
support the above.

Please have a look at the patch and comment, so I can implement changes when I
adopt this to the new and beter database schema.

Created attachment 511 [details]
Full patch for this bug. Needs additional files and a database change, files follow.

Created attachment 512 [details]
database patch for inserting new columns into the image table (path: maintenance/archives/patch-img_media_type.sql)

Created attachment 513 [details]
mime type detection module

Created attachment 514 [details]
standard mime.types file (mapping mime types to file extensions), used by MimeMagic.php (path: includes/mime.types)

Created attachment 515 [details]
mime.info file defining mime type aliases and media types, used by MimeMeagic.php (path: includes/mime.info)

The attachments I just mad ned to be applied together. Note that this patch
changes the columns in the image table (this is done by the
patch-img_media_type.sql database patch): img_type is obsolete and replaced by
img_media_type, img_major_mime and img_minor_mime.

updater.inc was adjusted to apply this database patch, but it has not been tested.

Note that the patch introduces several new setup variables into
DefaultSettings.php. To take full advantage of the new upload behaviour, set the
following in LocalSettings.php:

$wgStrictFileExtensions = false;
$wgVerifyMimeType= true;
$wgAntivirus= "clamav";  #or "f-prot"
$wgMimeDetectorCommand= "file -bi"; #on linux

Note that if $wgMimeDetectorCommand is net set, the new code tries to use
mime_content_type or finfo to detect the mime type, if either is available.
Those extensions need to be compiled into PHP, their use has not been tested yet.

This patch has been installed and tested successfully on Wegge's test wiki
<http://playwiki.wegge.dk/>, you can try it out there. I hope it can be applied
to the 1.5 branch soon.

Created attachment 516 [details]
Danish translations for the new strings introduced by this patch

Besides submitting the LanguageDa.php diff, I can tell that so far the test has
given no warnings or errors from the MediaWiki installation, except for those
introduced by my forgetfullness in the patching phase. If anybody experiences
problems on http://playwiki.wegge.dk I'll be happy to investigate, given an
approximate time.

Here's where the new files go (sorry, I should have put that into the
description of each attachment). The patches are obviously applied as patches.
The other files are, in order:

* database patch: maintenance/archives/patch-img_media_type.sql
* mime type module: includes/MimeMagic.php
* mime type file: includes/mime.types
* mime info file: includes/mime.info

that should be it :)

There are some things you change that aren't directly related to this
feature/functionality:

This should probably be enabled at some point, but isn't related to this feature:
""""
-$wgFileExtensions = array( 'png', 'gif', 'jpg', 'jpeg' );
+$wgFileExtensions = array( 'png', 'gif', 'jpg', 'jpeg', 'ogg' );
""""

Changing the Exif read/get functions doesn't have much to do with this:
""""
@@ -1146,7 +1289,10 @@
         * @return array
         */
        function retrieveExifData () {
-               if ( $this->type !== '2' ) return array ();
+               global $wgShowEXIF ;
+               if ( !$wgShowEXIF or !$this->fileExists) return array ();
+               
+               if ( $this->getMimeType() !== "image/jpeg" ) return array ();

                $exif = exif_read_data( $this->imagePath );
                foreach($exif as $k => $v) {
@@ -1162,11 +1308,14 @@
                                unset($exif[$k]);
                        }
                }
+               
                return $exif;
        }

        function getExifData () {
-               global $wgRequest;
+               global $wgRequest, $wgShowEXIF;
+               
+               if (!$wgShowEXIF or !isset($this->exif)) return array();

                $purge = $wgRequest->getVal( 'action' ) == 'purge';
                $ret = unserialize ( $this->metadata );
@@ -1416,7 +1565,7 @@
""""

Thera are also some stray whitespace changes:
""""
@@ -881,6 +1026,7 @@
                        }
                        wfPurgeSquidServers( $urls );
                }
+               
        }
 
        /**
@@ -924,16 +1070,6 
""""

""""
                        );
-
+                       
                        # Update the current image row
""""

""""
@@ -1162,11 +1308,14 @@
                                unset($exif[$k]);
                        }
                }
+               
                return $exif;
        }
""""

""""
@@ -1443,7 +1592,6 @@

        return array_key_exists( $name, $titleList );
 }
-       
""""

""""
                }
+               
                chmod( $this->mSavedFile, 0644 );
                return true;
        }
@@ -640,153 +677,207 @@
""""

""""
+       
 }
 ?>
""""

""""
        }
+       
        readfile( $fname );
 }
""""

And regarding this:

""""
@@ -1,17 +1,20 @@
 -- Extra image metadata, added for 1.5

+-- NOTE: as by patch-img_media_type.sql, the img_type
+-- column is no longer used and has therefore be removed from this patch
+
 ALTER TABLE /*$wgDBprefix*/image ADD (
   img_width int(5) NOT NULL default 0,
   img_height int(5) NOT NULL default 0,
   img_bits int(5) NOT NULL default 0,
-  img_type int(5) NOT NULL default -1
+  -- img_type int(5) NOT NULL default -1
 );

 ALTER TABLE /*$wgDBprefix*/oldimage ADD (
   oi_width int(5) NOT NULL default 0,
   oi_height int(5) NOT NULL default 0,
   oi_bits int(3) NOT NULL default 0,
-  oi_type int(3) NOT NULL default 0
+  -- oi_type int(3) NOT NULL default 0
 );
""""
if img_type and oi_type aren't used anymore please add a DELETE statement to
patch-img_media_type.sql that removes them, and change
maintenance/parserTests.php so that it doesn't create this obsolete table.

Otherwise it looks pretty solid, I haven't really tested it on a wide range of
input files yet, which I might just do;)

Some further comments after looking at it a bit more closely:

* The whole MimeMagic.php file is just a row of functions most of which have to
be parsed on each invocation, it should be a class that's loded on demand.

* The whole wfInitMimeMaps() function just serves to make arrays like:

Array
(
    [application/ogg] => ogg ogg
    [application/pdf] => pdf pdf
    [application/x-javascript] => js js
    [application/x-shockwave-flash] => swf swf
    [audio/midi] => mid midi kar mid midi kar
    [audio/mpeg] => mpga mpa mp2 mp3 mpga mp2 mp3
    <snip>

Array
(
    [OFFICE] => Array
        (
            [0] => application/pdf
            [1] => application/pdf
            [2] => application/acrobat
     <snip>

Array
(
    [] => text/rtf
    [application/x-javascript] => text/javascript
    [audio/mpeg] => audio/mp3
    [application/ogg] => audio/ogg
    <snip>

This could be simplified with something like:

$mime = array(
        'ogg' => array (
                'type' => AUDIO,
                'mime' => array(
                        'audio/ogg',
                        'application/x-ogg',
                        'application/ogg',
                        'audio/x-ogg'
                )
        )
);

Which would then be a native php datastructure, it would load alot faster and be
a bit more understandable, the former will not really be a problem if it's
rewritten as a class and invoced only when needed, but currently the whole thing
is being parsed in full and arrays generated even when they're not needed such
as on pages that don't have images, since Image.php is loaded even then.

* There's some usage of @ to hide errors, this is generally a bad thing™ and
should indicate that things aren't being done like they should.

(In reply to comment #16)

I'd like to point out a few things about avar's comments. In general, he has
some valid points, but with others I disagree. It would be good to have some
more input on this.

> * The whole MimeMagic.php file is just a row of functions most of which have to
> be parsed on each invocation, it should be a class that's loaded on demand.

It should indeed be loaded on demand, but being a class does not help with that
(a class needs to be parsed in full, too, even if it's never instantiated). That
is, the MimeMagic.php file should only be *included* on demand (ther's even a
note about this in Image.php where it's included). This would require the
include statement to be inside a function - I know this is possible, but brings
with it other problems, like global variables defined in MimeMagic to be
declared global explicitely (otherwise they would end up in the scope of the
calling function). This is rather obscure, but could be done. I did not do that
because I expected this to raise objections (as I belive doing includes inside
functions is obscure and should be avoided).

> * The whole wfInitMimeMaps() function just serves to make arrays like:
...
> This could be simplified with something like:
> 
> $mime = array(
>         'ogg' => array (
>                 'type' => AUDIO,
>                 'mime' => array(
>                         'audio/ogg',
>                         'application/x-ogg',
>                         'application/ogg',
>                         'audio/x-ogg'
>                 )
>         )
> );
> 
> Which would then be a native php datastructure, it would load alot faster and be
> a bit more understandable, 

I disagree:

* Complex nested array structures are less readable and harder to maintain (I
tried it, it was like that initially). 

* the map above would have to be process to build the reverse mappings (mime to
ext, mime to mime aliases, etc). Maintaining the reverse mappings by hand
seperately is redundant and error prone. Building several maps from a file has
the advantag of reducing redundancy and easing maintanance.

* Also, mime.type files are a standard structure on *nix systems and it should
be possible to use the systems standard mime map (/etc/mime.types) instead of
the on provided with mediawiki.

* The structure of mime.types and mime.info is much easier to understand and
maintain by non-programmers, and they are also more robust against syntax errors.

It is true however that native php structures would load faster. Even faster
would be serialized php structures, but they are not really maintainable by
hand. I belive this to be a non-issue, because those files are never parsed in
normal "viewing" operation, but only under three conditions:

* a new file is uploaded
* an old version of a file is restored
* the entry in the image table is upgraded from 1.4 format

> the former will not really be a problem if it's
> rewritten as a class and invoced only when needed, but currently the whole thing
> is being parsed in full and arrays generated even when they're not needed such
> as on pages that don't have images, since Image.php is loaded even then.

That is not true: the reason the arrays are build inside a function is precisely
to only do that on demand. The MimeMagic.php is included but non of the detector
functions is ever called, the arrays are not initialized. Including them in the
files as native structures would, among other things, cause them to always be
parsed. Also, as I said before, if the function is wrapped in a class or not is
irrelevant to the fact that it is always parsed in full.

> * There's some usage of @ to hide errors, this is generally a bad thing™ and
> should indicate that things aren't being done like they should.

The @ to hide errors is used *only* for lookups in arrays when it is not known
in advance if ther's a value for that key. The same could be done with
isdefined, if that is preferred. Supressing errors is necessary in some
situation and done throughout the code.

please comment, so I can fix issues. I'm currently inclined to do the
conditional include in Image.php, but i'll wait for more input for now. 

Created attachment 525 [details]
new version of MimeMagic.php, code is now wrapped in a class; this version matches the patch that follows. (path: includes/MimeMagic.php)

Created attachment 526 [details]
new patch, using the new, OO version of MimeMagic.php

New patch that hopefully takes care of avar's concerns. Main changes are

* MimeMagic code is now wrapped in a class. One one global instance is needed
* To get an instance of MimeMagic, use wfGetMimeMagic(). This function also
includes the code file if neccessary.
* I also adjusted ParserTest.php to use the new structure of the image table.

enjoy!

Created attachment 527 [details]
"upgrade" patch in case you already applied the first full patch (from 2005-05-10 20:46 UTC)

(In reply to comment #15)
> This should probably be enabled at some point, but isn't related to this feature:
> """"
> -$wgFileExtensions = array( 'png', 'gif', 'jpg', 'jpeg' );
> +$wgFileExtensions = array( 'png', 'gif', 'jpg', 'jpeg', 'ogg' );

I removed ogg from the default whitelist because many web servers do not have a MIME 
type set for the .ogg extension. This makes such files more susceptible to various 
attacks: client-side type autodetection bugs are more easily triggered on text/plain 
or application/octet-stream, and server-side type detection bugs like interpreting 
files as executable scripts is easier to trigger when there's not a default file type 
for it to match.

While the various other precautions make these less likely (checking for multiple 
extensions and blacklisting known bad ones, checking for known client-side script 
triggers, etc), I prefer a 'secure by default' philosophy. Admins can explicitly add 
it to the whitelist (as we have) if it's known to be safe in the environment they've 
created.

> New patch that hopefully takes care of avar's concerns. Main changes are

It still changes $wgFileExtensions, the exif handling, whitespace and now has
$wgUploadDescriptionPattern (although unused) in DefaultSettings.php, please
make sure that the patches you send in only change things directly related to
this particular feature, having other changes in there makes it harder to review.

*** Bug 667 has been marked as a duplicate of this bug. ***

*** Bug 1683 has been marked as a duplicate of this bug. ***

*** Bug 1790 has been marked as a duplicate of this bug. ***

Created attachment 542 [details]
updated patch, implementing most of brions suggestions. Requires the new version of the other files, see below.

Created attachment 543 [details]
MIME detection module, new version (includes/MimeMagic.php)

Created attachment 544 [details]
updated mime info (includes/mime.info)

Created attachment 545 [details]
updated mime types (includes/mime.types)

Created attachment 546 [details]
updated database patch, now also removed the obsolete img_type column. (maintenance/archives/patch-img_media_type.sql)

I just uploded a new version of my patch. I hope I have implemented everything
brion and avar suggested.

Major changes are:

* Databse consistence: 
-- automatically rebuild memcached image info
-- give a descriptive error if the database schema was not updated
-- delete obsolete column in the database patch
* removed the code for disabling [[media:...]] links
* Cleaned up changes to whitespace and code style. At least I tried.
* Some changes to ImagePage.php - more info about non-image files, links to
source of pre-rendered images, etc.
* Tweaked the code to also render TIFF, BMP etc if ImageMagic is available.
* PHP code is now detected an rejected
* cleaned up mustRender/canRender/allowInlienDisplay

Ther has been some more cleanup and minor fixed. Please have a look.

Created attachment 547 [details]
MIME detection module, new version (includes/MimeMagic.php)

The version by Daniel Kinzler had an undefined variable ($e) in
guessTypesForExtension()

Created attachment 548 [details]
MIME detection module, new version (includes/MimeMagic.php)

Fixes a typo in guessTypesForExtension() (pregt_replace => preg_replace)

attachment 542 [details] contains alot of changes to the enotif code that tim cleaned
up/removed recently, please go over the patch manually to make sure it doesn't
revert any recent changes.

And there's a syntax error on line 258 of MimeMagic.php, you need to add slashes
around that regular expression.

Created attachment 549 [details]
Changes to Language.php, should be applied to a clean tree (not directly after attachment 542 [details])

replaces the includes/Language.php part of attachment 542 [details], tones down the
warning a little and removes the link to Wikipedia.

Created attachment 550 [details]
Changes to ImagePage.php, should be applied to a clean tree (not directly after attachment 542 [details])

replaces the includes/ImagePage.php part of attachment 542 [details], moves the links
around so that the direct link to the file is always immitiately after the TOC
(as it was previously).

Created attachment 554 [details]
MIME detection module (includes/MimeMagic.php), fixing bugs mentioned in #32, #33, #34

Created attachment 555 [details]
new version of the FULL PATCH, fixing bugs mentioned in #32, #33, #34

Ok, I hope that's all. 

FYI: this patch includes things mentioned in #32, #33, #34, but not the changes
by attachment 549 [details] and attachment 550 [details].

Commited to HEAD, marking this as FIXED.

*** Bug 1410 has been marked as a duplicate of this bug. ***