Reduce false positives for PHP in MimeMagic

(bug 16583) This was detecting PHP if any of a few three-byte strings
occurred anywhere in the first 1024 bytes of the file.  This is too
paranoid -- it creates a significant number of false positives for
binary files, reportedly on the order of about one every 4096 uploads.

It's hard to see what security advantage this check every conveyed,
because it only looks in the first 1024 bytes anyway.  For the purposes
of upload it could surely be removed entirely, but I didn't check all
callers, so maybe some caller wants to guess whether the file is PHP for
some purpose other than banning it.  So for now I only removed the
checks for the shortest strings, which were most likely to get hit.

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 1255ecc..182811d 100644 (file)
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -622,6 +622,7 @@ Hopefully we will remove this configuration var soon)
 * (bug 6579) Fixed protecting images from uploading only 
 * (bug 18609) Search index was empty for some pages
 * (bug 13453) rebuildrecentchanges maintenance script works on PG again
+* (bug 16583) Reduce false positives when checking for PHP (on upload, etc.)
 
 == API changes in 1.16 ==
 

diff --git a/includes/MimeMagic.php b/includes/MimeMagic.php
index d52de99..5e28ebe 100644 (file)
--- a/includes/MimeMagic.php
+++ b/includes/MimeMagic.php
@@ -469,16 +469,18 @@ class MimeMagic {
                }
 
                /*
-                * look for PHP
-                * Check for this before HTML/XML...
-                * Warning: this is a heuristic, and won't match a file with a lot of non-PHP before.
-                * It will also match text files which could be PHP. :)
+                * Look for PHP.  Check for this before HTML/XML...  Warning: this is a
+                * heuristic, and won't match a file with a lot of non-PHP before.  It
+                * will also match text files which could be PHP. :)
+                *
+                * FIXME: For this reason, the check is probably useless -- an attacker
+                * could almost certainly just pad the file with a lot of nonsense to
+                * circumvent the check in any case where it would be a security
+                * problem.  On the other hand, it causes harmful false positives (bug
+                * 16583).  The heuristic has been cut down to exclude three-character
+                * strings like "<? ", but should it be axed completely?
                 */
                if( ( strpos( $head, '<?php' ) !== false ) ||
-                   ( strpos( $head, '<? ' ) !== false ) ||
-                   ( strpos( $head, "<?\n" ) !== false ) ||
-                   ( strpos( $head, "<?\t" ) !== false ) ||
-                   ( strpos( $head, "<?=" ) !== false ) ||
 
                    ( strpos( $head, "<\x00?\x00p\x00h\x00p" ) !== false ) ||
                    ( strpos( $head, "<\x00?\x00 " ) !== false ) ||