(bug 31740) JpegMetadataExtractor and friends weren't checking for unexpected end...
authorBrian Wolff <bawolff@users.mediawiki.org>
Mon, 24 Oct 2011 02:19:11 +0000 (02:19 +0000)
committerBrian Wolff <bawolff@users.mediawiki.org>
Mon, 24 Oct 2011 02:19:11 +0000 (02:19 +0000)
I created a new wrapper around unpack - wfUnpack which throws an exception if it runs out of input
(but i didn't use that on GIFMetadataExtractor/PNGMetadataExtractor because those files have a header saying that they weren't external dependencies minimized for potential re-users. I don't think anyone actually re-uses those files, but I didn't want to add a dependency on wfUnpack just in case).

I also changed fopen( blah, "r" ) -> fopen( blah, "rb" ) since these are binary formats, so we don't want newlines converted on windows.

includes/GlobalFunctions.php
includes/media/BMP.php
includes/media/GIFMetadataExtractor.php
includes/media/JpegMetadataExtractor.php
includes/media/PNGMetadataExtractor.php

index e5d17ba..34c46b3 100644 (file)
@@ -3609,3 +3609,39 @@ function wfGetParserCacheStorage() {
 function wfRunHooks( $event, $args = array() ) {
        return Hooks::run( $event, $args );
 }
+
+/**
+ * Wrapper around php's unpack.
+ *
+ * @param $format String: The format string (See php's docs)
+ * @param $data: A binary string of binary data
+ * @param $length integer or false: The minimun length of $data. This is to
+ *     prevent reading beyond the end of $data. false to disable the check.
+ *
+ * Also be careful when using this function to read unsigned 32 bit integer
+ * because php might make it negative.
+ *
+ * @throws MWException if $data not long enough, or if unpack fails
+ * @return Associative array of the extracted data
+ */
+function wfUnpack( $format, $data, $length=false ) {
+       if ( $length !== false ) {
+               $realLen = strlen( $data );
+               if ( $realLen < $length ) {
+                       throw new MWException( "Tried to use wfUnpack on a "
+                               . "string of length $realLen, but needed one "
+                               . "of at least length $length."
+                       );
+               }
+       }
+
+       wfSuppressWarnings();
+       $result = unpack( $format, $data );
+       wfRestoreWarnings();
+
+       if ( $result === false ) {
+               // If it cannot extract the packed data.
+               throw new MWException( "unpack could not unpack binary data" );
+       }
+       return $result;
+}
index f7dd008..6886e95 100644 (file)
@@ -42,7 +42,7 @@ class BmpHandler extends BitmapHandler {
         * @return array
         */
        function getImageSize( $image, $filename ) {
-               $f = fopen( $filename, 'r' );
+               $f = fopen( $filename, 'rb' );
                if( !$f ) {
                        return false;
                }
@@ -54,8 +54,12 @@ class BmpHandler extends BitmapHandler {
                $h = substr( $header, 22, 4);
 
                // Convert the unsigned long 32 bits (little endian):
-               $w = unpack( 'V' , $w );
-               $h = unpack( 'V' , $h );
+               try {
+                       $w = wfUnpack( 'V', $w, 4 );
+                       $h = wfUnpack( 'V', $h, 4 );
+               } catch ( MWException $e ) {
+                       return false;
+               }
                return array( $w[1], $h[1] );
        }
 }
index 02a9c60..5dbeb8f 100644 (file)
@@ -50,7 +50,7 @@ class GIFMetadataExtractor {
                        throw new Exception( "File $filename does not exist" );
                }
 
-               $fh = fopen( $filename, 'r' );
+               $fh = fopen( $filename, 'rb' );
 
                if ( !$fh ) {
                        throw new Exception( "Unable to open file $filename" );
@@ -95,6 +95,7 @@ class GIFMetadataExtractor {
                                self::skipBlock( $fh ); 
                        } elseif ( $buf == self::$gif_extension_sep ) {
                                $buf = fread( $fh, 1 );
+                               if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
                                $extension_code = unpack( 'C', $buf );
                                $extension_code = $extension_code[1];
 
@@ -105,6 +106,7 @@ class GIFMetadataExtractor {
                                        fread( $fh, 1 ); // Transparency, disposal method, user input
 
                                        $buf = fread( $fh, 2 ); // Delay, in hundredths of seconds.
+                                       if ( strlen( $buf ) < 2 ) throw new Exception( "Ran out of input" );
                                        $delay = unpack( 'v', $buf );
                                        $delay = $delay[1];
                                        $duration += $delay * 0.01;
@@ -112,6 +114,7 @@ class GIFMetadataExtractor {
                                        fread( $fh, 1 ); // Transparent colour index
 
                                        $term = fread( $fh, 1 ); // Should be a terminator
+                                       if ( strlen( $term ) < 1 ) throw new Exception( "Ran out of input" );
                                        $term = unpack( 'C', $term );
                                        $term = $term[1];
                                        if ($term != 0 ) {
@@ -150,6 +153,7 @@ class GIFMetadataExtractor {
                                        // Application extension (Netscape info about the animated gif)
                                        // or XMP (or theoretically any other type of extension block)
                                        $blockLength = fread( $fh, 1 );
+                                       if ( strlen( $blockLength ) < 1 ) throw new Exception( "Ran out of input" );
                                        $blockLength = unpack( 'C', $blockLength );
                                        $blockLength = $blockLength[1];
                                        $data = fread( $fh, $blockLength );
@@ -172,6 +176,7 @@ class GIFMetadataExtractor {
                                                
                                                // Unsigned little-endian integer, loop count or zero for "forever"
                                                $loopData = fread( $fh, 2 );
+                                               if ( strlen( $loopData ) < 2 ) throw new Exception( "Ran out of input" );
                                                $loopData = unpack( 'v', $loopData );
                                                $loopCount = $loopData[1];
                                                
@@ -209,6 +214,7 @@ class GIFMetadataExtractor {
                        } elseif ( $buf == self::$gif_term ) {
                                break;
                        } else {
+                               if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
                                $byte = unpack( 'C', $buf );
                                $byte = $byte[1];
                                throw new Exception( "At position: ".ftell($fh). ", Unknown byte ".$byte );
@@ -242,6 +248,7 @@ class GIFMetadataExtractor {
         * @return int
         */
        static function decodeBPP( $data ) {
+               if ( strlen( $data ) < 1 ) throw new Exception( "Ran out of input" );
                $buf = unpack( 'C', $data );
                $buf = $buf[1];
                $bpp = ( $buf & 7 ) + 1;
@@ -259,6 +266,7 @@ class GIFMetadataExtractor {
        static function skipBlock( $fh ) {
                while ( !feof( $fh ) ) {
                        $buf = fread( $fh, 1 );
+                       if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
                        $block_len = unpack( 'C', $buf );
                        $block_len = $block_len[1];
                        if ($block_len == 0) {
index ae3ca8e..1f04342 100644 (file)
@@ -128,7 +128,7 @@ class JpegMetadataExtractor {
                                continue;
                        } else {
                                // segment we don't care about, so skip
-                               $size = unpack( "nint", fread( $fh, 2 ) );
+                               $size = wfUnpack( "nint", fread( $fh, 2 ), 2 );
                                if ( $size['int'] <= 2 ) throw new MWException( "invalid marker size in jpeg" );
                                fseek( $fh, $size['int'] - 2, SEEK_CUR );
                        }
@@ -144,9 +144,11 @@ class JpegMetadataExtractor {
        * @return data content of segment.
        */
        private static function jpegExtractMarker( &$fh ) {
-               $size = unpack( "nint", fread( $fh, 2 ) );
+               $size = wfUnpack( "nint", fread( $fh, 2 ), 2 );
                if ( $size['int'] <= 2 ) throw new MWException( "invalid marker size in jpeg" );
-               return fread( $fh, $size['int'] - 2 );
+               $segment = fread( $fh, $size['int'] - 2 );
+               if ( strlen( $segment ) !== $size['int'] - 2 ) throw new MWException( "Segment shorter than expected" );
+               return $segment;
        }
 
        /**
@@ -205,7 +207,12 @@ class JpegMetadataExtractor {
                        $offset += $lenName;
 
                        // now length of data (unsigned long big endian)
-                       $lenData = unpack( 'Nlen', substr( $app13, $offset, 4 ) );
+                       $lenData = wfUnpack( 'Nlen', substr( $app13, $offset, 4 ), 4 );
+                       // PHP can take issue with very large unsigned ints and make them negative.
+                       // Which should never ever happen, as this has to be inside a segment
+                       // which is limited to a 16 bit number.
+                       if ( $lenData['len'] < 0 ) throw new MWException( "Too big PSIR (" . $lenData['len'] . ')' );
+
                        $offset += 4; // 4bytes length field;
 
                        // this should not happen, but check.
index 4c1887e..d3c44d4 100644 (file)
@@ -66,7 +66,7 @@ class PNGMetadataExtractor {
                        throw new Exception( __METHOD__ . ": File $filename does not exist" );
                }
 
-               $fh = fopen( $filename, 'r' );
+               $fh = fopen( $filename, 'rb' );
 
                if ( !$fh ) {
                        throw new Exception( __METHOD__ . ": Unable to open file $filename" );
@@ -81,20 +81,24 @@ class PNGMetadataExtractor {
                // Read chunks
                while ( !feof( $fh ) ) {
                        $buf = fread( $fh, 4 );
-                       if ( !$buf ) {
+                       if ( !$buf || strlen( $buf ) < 4 ) {
                                throw new Exception( __METHOD__ . ": Read error" );
                        }
                        $chunk_size = unpack( "N", $buf );
                        $chunk_size = $chunk_size[1];
 
+                       if ( $chunk_size < 0 ) {
+                               throw new Exception( __METHOD__ . ": Chunk size too big for unpack" );
+                       }
+
                        $chunk_type = fread( $fh, 4 );
-                       if ( !$chunk_type ) {
+                       if ( !$chunk_type || strlen( $chunk_type ) < 4 ) {
                                throw new Exception( __METHOD__ . ": Read error" );
                        }
 
                        if ( $chunk_type == "IHDR" ) {
                                $buf = self::read( $fh, $chunk_size );
-                               if ( !$buf ) {
+                               if ( !$buf || strlen( $buf ) < $chunk_size ) {
                                        throw new Exception( __METHOD__ . ": Read error" );
                                }
                                $bitDepth = ord( substr( $buf, 8, 1 ) );
@@ -122,7 +126,7 @@ class PNGMetadataExtractor {
                                }
                        } elseif ( $chunk_type == "acTL" ) {
                                $buf = fread( $fh, $chunk_size );
-                               if( !$buf ) {
+                               if( !$buf || strlen( $buf ) < $chunk_size || $chunk_size < 4 ) {
                                        throw new Exception( __METHOD__ . ": Read error" );
                                }
 
@@ -131,10 +135,13 @@ class PNGMetadataExtractor {
                                $loopCount = $actl['plays'];
                        } elseif ( $chunk_type == "fcTL" ) {
                                $buf = self::read( $fh, $chunk_size );
-                               if ( !$buf ) {
+                               if ( !$buf || strlen( $buf ) < $chunk_size ) {
                                        throw new Exception( __METHOD__ . ": Read error" );
                                }
                                $buf = substr( $buf, 20 );
+                               if ( strlen( $buf ) < 4 ) {
+                                       throw new Exception( __METHOD__ . ": Read error" );
+                               }
 
                                $fctldur = unpack( "ndelay_num/ndelay_den", $buf );
                                if ( $fctldur['delay_den'] == 0 ) {
@@ -294,7 +301,7 @@ class PNGMetadataExtractor {
                                        throw new Exception( __METHOD__ . ": tIME wrong size" );
                                }
                                $buf = self::read( $fh, $chunk_size );
-                               if ( !$buf ) {
+                               if ( !$buf || strlen( $buf ) < $chunk_size ) {
                                        throw new Exception( __METHOD__ . ": Read error" );
                                }
 
@@ -317,20 +324,24 @@ class PNGMetadataExtractor {
                                }
 
                                $buf = self::read( $fh, $chunk_size );
-                               if ( !$buf ) {
+                               if ( !$buf || strlen( $buf ) < $chunk_size ) {
                                        throw new Exception( __METHOD__ . ": Read error" );
                                }
 
                                $dim = unpack( "Nwidth/Nheight/Cunit", $buf );
                                if ( $dim['unit'] == 1 ) {
-                                       // unit is meters
-                                       // (as opposed to 0 = undefined )
-                                       $text['XResolution'] = $dim['width']
-                                               . '/100';
-                                       $text['YResolution'] = $dim['height']
-                                               . '/100';
-                                       $text['ResolutionUnit'] = 3;
-                                       // 3 = dots per cm (from Exif).
+                                       // Need to check for negative because php
+                                       // doesn't deal with super-large unsigned 32-bit ints well
+                                       if ( $dim['width'] > 0 && $dim['height'] > 0 ) {
+                                               // unit is meters
+                                               // (as opposed to 0 = undefined )
+                                               $text['XResolution'] = $dim['width']
+                                                       . '/100';
+                                               $text['YResolution'] = $dim['height']
+                                                       . '/100';
+                                               $text['ResolutionUnit'] = 3;
+                                               // 3 = dots per cm (from Exif).
+                                       }
                                }
 
                        } elseif ( $chunk_type == "IEND" ) {