From: Brian Wolff Date: Mon, 24 Oct 2011 02:19:11 +0000 (+0000) Subject: (bug 31740) JpegMetadataExtractor and friends weren't checking for unexpected end... X-Git-Tag: 1.31.0-rc.0~26943 X-Git-Url: http://git.cyclocoop.org/%28?a=commitdiff_plain;h=8cbf9be869d7f20f11c11c9d9637a838d45331e8;p=lhc%2Fweb%2Fwiklou.git (bug 31740) JpegMetadataExtractor and friends weren't checking for unexpected end of input properly 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. --- diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index e5d17ba04c..34c46b32ac 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -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; +} diff --git a/includes/media/BMP.php b/includes/media/BMP.php index f7dd00889a..6886e95058 100644 --- a/includes/media/BMP.php +++ b/includes/media/BMP.php @@ -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] ); } } diff --git a/includes/media/GIFMetadataExtractor.php b/includes/media/GIFMetadataExtractor.php index 02a9c60dfc..5dbeb8f852 100644 --- a/includes/media/GIFMetadataExtractor.php +++ b/includes/media/GIFMetadataExtractor.php @@ -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) { diff --git a/includes/media/JpegMetadataExtractor.php b/includes/media/JpegMetadataExtractor.php index ae3ca8ed61..1f04342599 100644 --- a/includes/media/JpegMetadataExtractor.php +++ b/includes/media/JpegMetadataExtractor.php @@ -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. diff --git a/includes/media/PNGMetadataExtractor.php b/includes/media/PNGMetadataExtractor.php index 4c1887e951..d3c44d4f18 100644 --- a/includes/media/PNGMetadataExtractor.php +++ b/includes/media/PNGMetadataExtractor.php @@ -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" ) {