r100572 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r100571‎ | r100572 | r100573 >
Date:02:19, 24 October 2011
Author:bawolff
Status:ok
Tags:
Comment:
(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.
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/media/BMP.php (modified) (history)
  • /trunk/phase3/includes/media/GIFMetadataExtractor.php (modified) (history)
  • /trunk/phase3/includes/media/JpegMetadataExtractor.php (modified) (history)
  • /trunk/phase3/includes/media/PNGMetadataExtractor.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -3609,3 +3609,39 @@
36103610 function wfRunHooks( $event, $args = array() ) {
36113611 return Hooks::run( $event, $args );
36123612 }
 3613+
 3614+/**
 3615+ * Wrapper around php's unpack.
 3616+ *
 3617+ * @param $format String: The format string (See php's docs)
 3618+ * @param $data: A binary string of binary data
 3619+ * @param $length integer or false: The minimun length of $data. This is to
 3620+ * prevent reading beyond the end of $data. false to disable the check.
 3621+ *
 3622+ * Also be careful when using this function to read unsigned 32 bit integer
 3623+ * because php might make it negative.
 3624+ *
 3625+ * @throws MWException if $data not long enough, or if unpack fails
 3626+ * @return Associative array of the extracted data
 3627+ */
 3628+function wfUnpack( $format, $data, $length=false ) {
 3629+ if ( $length !== false ) {
 3630+ $realLen = strlen( $data );
 3631+ if ( $realLen < $length ) {
 3632+ throw new MWException( "Tried to use wfUnpack on a "
 3633+ . "string of length $realLen, but needed one "
 3634+ . "of at least length $length."
 3635+ );
 3636+ }
 3637+ }
 3638+
 3639+ wfSuppressWarnings();
 3640+ $result = unpack( $format, $data );
 3641+ wfRestoreWarnings();
 3642+
 3643+ if ( $result === false ) {
 3644+ // If it cannot extract the packed data.
 3645+ throw new MWException( "unpack could not unpack binary data" );
 3646+ }
 3647+ return $result;
 3648+}
Index: trunk/phase3/includes/media/GIFMetadataExtractor.php
——@@ -50,7 +50,7 @@
5151 throw new Exception( "File $filename does not exist" );
5252 }
5353
54 - $fh = fopen( $filename, 'r' );
 54+ $fh = fopen( $filename, 'rb' );
5555
5656 if ( !$fh ) {
5757 throw new Exception( "Unable to open file $filename" );
——@@ -95,6 +95,7 @@
9696 self::skipBlock( $fh );
9797 } elseif ( $buf == self::$gif_extension_sep ) {
9898 $buf = fread( $fh, 1 );
 99+ if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
99100 $extension_code = unpack( 'C', $buf );
100101 $extension_code = $extension_code[1];
101102
——@@ -105,6 +106,7 @@
106107 fread( $fh, 1 ); // Transparency, disposal method, user input
107108
108109 $buf = fread( $fh, 2 ); // Delay, in hundredths of seconds.
 110+ if ( strlen( $buf ) < 2 ) throw new Exception( "Ran out of input" );
109111 $delay = unpack( 'v', $buf );
110112 $delay = $delay[1];
111113 $duration += $delay * 0.01;
——@@ -112,6 +114,7 @@
113115 fread( $fh, 1 ); // Transparent colour index
114116
115117 $term = fread( $fh, 1 ); // Should be a terminator
 118+ if ( strlen( $term ) < 1 ) throw new Exception( "Ran out of input" );
116119 $term = unpack( 'C', $term );
117120 $term = $term[1];
118121 if ($term != 0 ) {
——@@ -150,6 +153,7 @@
151154 // Application extension (Netscape info about the animated gif)
152155 // or XMP (or theoretically any other type of extension block)
153156 $blockLength = fread( $fh, 1 );
 157+ if ( strlen( $blockLength ) < 1 ) throw new Exception( "Ran out of input" );
154158 $blockLength = unpack( 'C', $blockLength );
155159 $blockLength = $blockLength[1];
156160 $data = fread( $fh, $blockLength );
——@@ -172,6 +176,7 @@
173177
174178 // Unsigned little-endian integer, loop count or zero for "forever"
175179 $loopData = fread( $fh, 2 );
 180+ if ( strlen( $loopData ) < 2 ) throw new Exception( "Ran out of input" );
176181 $loopData = unpack( 'v', $loopData );
177182 $loopCount = $loopData[1];
178183
——@@ -209,6 +214,7 @@
210215 } elseif ( $buf == self::$gif_term ) {
211216 break;
212217 } else {
 218+ if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
213219 $byte = unpack( 'C', $buf );
214220 $byte = $byte[1];
215221 throw new Exception( "At position: ".ftell($fh). ", Unknown byte ".$byte );
——@@ -242,6 +248,7 @@
243249 * @return int
244250 */
245251 static function decodeBPP( $data ) {
 252+ if ( strlen( $data ) < 1 ) throw new Exception( "Ran out of input" );
246253 $buf = unpack( 'C', $data );
247254 $buf = $buf[1];
248255 $bpp = ( $buf & 7 ) + 1;
——@@ -259,6 +266,7 @@
260267 static function skipBlock( $fh ) {
261268 while ( !feof( $fh ) ) {
262269 $buf = fread( $fh, 1 );
 270+ if ( strlen( $buf ) < 1 ) throw new Exception( "Ran out of input" );
263271 $block_len = unpack( 'C', $buf );
264272 $block_len = $block_len[1];
265273 if ($block_len == 0) {
Index: trunk/phase3/includes/media/BMP.php
——@@ -42,7 +42,7 @@
4343 * @return array
4444 */
4545 function getImageSize( $image, $filename ) {
46 - $f = fopen( $filename, 'r' );
 46+ $f = fopen( $filename, 'rb' );
4747 if( !$f ) {
4848 return false;
4949 }
——@@ -54,8 +54,12 @@
5555 $h = substr( $header, 22, 4);
5656
5757 // Convert the unsigned long 32 bits (little endian):
58 - $w = unpack( 'V' , $w );
59 - $h = unpack( 'V' , $h );
 58+ try {
 59+ $w = wfUnpack( 'V', $w, 4 );
 60+ $h = wfUnpack( 'V', $h, 4 );
 61+ } catch ( MWException $e ) {
 62+ return false;
 63+ }
6064 return array( $w[1], $h[1] );
6165 }
6266 }
Index: trunk/phase3/includes/media/PNGMetadataExtractor.php
——@@ -66,7 +66,7 @@
6767 throw new Exception( __METHOD__ . ": File $filename does not exist" );
6868 }
6969
70 - $fh = fopen( $filename, 'r' );
 70+ $fh = fopen( $filename, 'rb' );
7171
7272 if ( !$fh ) {
7373 throw new Exception( __METHOD__ . ": Unable to open file $filename" );
——@@ -81,20 +81,24 @@
8282 // Read chunks
8383 while ( !feof( $fh ) ) {
8484 $buf = fread( $fh, 4 );
85 - if ( !$buf ) {
 85+ if ( !$buf || strlen( $buf ) < 4 ) {
8686 throw new Exception( __METHOD__ . ": Read error" );
8787 }
8888 $chunk_size = unpack( "N", $buf );
8989 $chunk_size = $chunk_size[1];
9090
 91+ if ( $chunk_size < 0 ) {
 92+ throw new Exception( __METHOD__ . ": Chunk size too big for unpack" );
 93+ }
 94+
9195 $chunk_type = fread( $fh, 4 );
92 - if ( !$chunk_type ) {
 96+ if ( !$chunk_type || strlen( $chunk_type ) < 4 ) {
9397 throw new Exception( __METHOD__ . ": Read error" );
9498 }
9599
96100 if ( $chunk_type == "IHDR" ) {
97101 $buf = self::read( $fh, $chunk_size );
98 - if ( !$buf ) {
 102+ if ( !$buf || strlen( $buf ) < $chunk_size ) {
99103 throw new Exception( __METHOD__ . ": Read error" );
100104 }
101105 $bitDepth = ord( substr( $buf, 8, 1 ) );
——@@ -122,7 +126,7 @@
123127 }
124128 } elseif ( $chunk_type == "acTL" ) {
125129 $buf = fread( $fh, $chunk_size );
126 - if( !$buf ) {
 130+ if( !$buf || strlen( $buf ) < $chunk_size || $chunk_size < 4 ) {
127131 throw new Exception( __METHOD__ . ": Read error" );
128132 }
129133
——@@ -131,10 +135,13 @@
132136 $loopCount = $actl['plays'];
133137 } elseif ( $chunk_type == "fcTL" ) {
134138 $buf = self::read( $fh, $chunk_size );
135 - if ( !$buf ) {
 139+ if ( !$buf || strlen( $buf ) < $chunk_size ) {
136140 throw new Exception( __METHOD__ . ": Read error" );
137141 }
138142 $buf = substr( $buf, 20 );
 143+ if ( strlen( $buf ) < 4 ) {
 144+ throw new Exception( __METHOD__ . ": Read error" );
 145+ }
139146
140147 $fctldur = unpack( "ndelay_num/ndelay_den", $buf );
141148 if ( $fctldur['delay_den'] == 0 ) {
——@@ -294,7 +301,7 @@
295302 throw new Exception( __METHOD__ . ": tIME wrong size" );
296303 }
297304 $buf = self::read( $fh, $chunk_size );
298 - if ( !$buf ) {
 305+ if ( !$buf || strlen( $buf ) < $chunk_size ) {
299306 throw new Exception( __METHOD__ . ": Read error" );
300307 }
301308
——@@ -317,20 +324,24 @@
318325 }
319326
320327 $buf = self::read( $fh, $chunk_size );
321 - if ( !$buf ) {
 328+ if ( !$buf || strlen( $buf ) < $chunk_size ) {
322329 throw new Exception( __METHOD__ . ": Read error" );
323330 }
324331
325332 $dim = unpack( "Nwidth/Nheight/Cunit", $buf );
326333 if ( $dim['unit'] == 1 ) {
327 - // unit is meters
328 - // (as opposed to 0 = undefined )
329 - $text['XResolution'] = $dim['width']
330 - . '/100';
331 - $text['YResolution'] = $dim['height']
332 - . '/100';
333 - $text['ResolutionUnit'] = 3;
334 - // 3 = dots per cm (from Exif).
 334+ // Need to check for negative because php
 335+ // doesn't deal with super-large unsigned 32-bit ints well
 336+ if ( $dim['width'] > 0 && $dim['height'] > 0 ) {
 337+ // unit is meters
 338+ // (as opposed to 0 = undefined )
 339+ $text['XResolution'] = $dim['width']
 340+ . '/100';
 341+ $text['YResolution'] = $dim['height']
 342+ . '/100';
 343+ $text['ResolutionUnit'] = 3;
 344+ // 3 = dots per cm (from Exif).
 345+ }
335346 }
336347
337348 } elseif ( $chunk_type == "IEND" ) {
Index: trunk/phase3/includes/media/JpegMetadataExtractor.php
——@@ -128,7 +128,7 @@
129129 continue;
130130 } else {
131131 // segment we don't care about, so skip
132 - $size = unpack( "nint", fread( $fh, 2 ) );
 132+ $size = wfUnpack( "nint", fread( $fh, 2 ), 2 );
133133 if ( $size['int'] <= 2 ) throw new MWException( "invalid marker size in jpeg" );
134134 fseek( $fh, $size['int'] - 2, SEEK_CUR );
135135 }
——@@ -144,9 +144,11 @@
145145 * @return data content of segment.
146146 */
147147 private static function jpegExtractMarker( &$fh ) {
148 - $size = unpack( "nint", fread( $fh, 2 ) );
 148+ $size = wfUnpack( "nint", fread( $fh, 2 ), 2 );
149149 if ( $size['int'] <= 2 ) throw new MWException( "invalid marker size in jpeg" );
150 - return fread( $fh, $size['int'] - 2 );
 150+ $segment = fread( $fh, $size['int'] - 2 );
 151+ if ( strlen( $segment ) !== $size['int'] - 2 ) throw new MWException( "Segment shorter than expected" );
 152+ return $segment;
151153 }
152154
153155 /**
——@@ -205,7 +207,12 @@
206208 $offset += $lenName;
207209
208210 // now length of data (unsigned long big endian)
209 - $lenData = unpack( 'Nlen', substr( $app13, $offset, 4 ) );
 211+ $lenData = wfUnpack( 'Nlen', substr( $app13, $offset, 4 ), 4 );
 212+ // PHP can take issue with very large unsigned ints and make them negative.
 213+ // Which should never ever happen, as this has to be inside a segment
 214+ // which is limited to a 16 bit number.
 215+ if ( $lenData['len'] < 0 ) throw new MWException( "Too big PSIR (" . $lenData['len'] . ')' );
 216+
210217 $offset += 4; // 4bytes length field;
211218
212219 // this should not happen, but check.

Follow-up revisions

RevisionCommit summaryAuthorDate
r100578MFT r100572: fixed JpegMetadataExtractor warningststarling04:03, 24 October 2011
r100750REL1_18 MFT r98997, r99118, r99370, r99700, r100239, r100242, r100347, r10051...reedy21:51, 25 October 2011

Status & tagging log