r65467 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r65466‎ | r65467 | r65468 >
Date:16:24, 23 April 2010
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
More rigorous fix for ImageMagick parameter interpretation (bug 23148 etc.) based on examination of the source code, updating r64932. The source was large and complex and I might have missed some bits, but this is my best guess for now.

Tested bug 23148 and wildcards in filenames (was broken) using a patched convert. With a manual unit test (no convert), tested all branches of the new functions except wfIsWindows() == true.
Modified paths:
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/Bitmap.php
@@ -62,7 +62,7 @@
6363 $physicalHeight = $params['physicalHeight'];
6464 $clientWidth = $params['width'];
6565 $clientHeight = $params['height'];
66 - $descriptionUrl = isset( $params['descriptionUrl'] ) ? "File source: ". $params['descriptionUrl'] : '';
 66+ $comment = isset( $params['descriptionUrl'] ) ? "File source: ". $params['descriptionUrl'] : '';
6767 $srcWidth = $image->getWidth();
6868 $srcHeight = $image->getHeight();
6969 $mimeType = $image->getMimeType();
@@ -113,7 +113,7 @@
114114
115115 $quality = '';
116116 $sharpen = '';
117 - $frame = '';
 117+ $scene = false;
118118 $animation = '';
119119 if ( $mimeType == 'image/jpeg' ) {
120120 $quality = "-quality 80"; // 80%
@@ -127,7 +127,7 @@
128128 if( $this->getImageArea( $image, $srcWidth, $srcHeight ) > $wgMaxAnimatedGifArea ) {
129129 // Extract initial frame only; we're so big it'll
130130 // be a total drag. :P
131 - $frame = '[0]';
 131+ $scene = 0;
132132 } else {
133133 // Coalesce is needed to scale animated GIFs properly (bug 1017).
134134 $animation = ' -coalesce ';
@@ -151,18 +151,16 @@
152152 $tempEnv .
153153 wfEscapeShellArg( $wgImageMagickConvertCommand ) .
154154 " {$quality} -background white -size {$physicalWidth} ".
155 - wfEscapeShellArg( $srcPath . $frame ) .
 155+ wfEscapeShellArg( $this->escapeMagickInput( $srcPath, $scene ) ) .
156156 $animation .
157157 // For the -resize option a "!" is needed to force exact size,
158158 // or ImageMagick may decide your ratio is wrong and slice off
159159 // a pixel.
160160 " -thumbnail " . wfEscapeShellArg( "{$physicalWidth}x{$physicalHeight}!" ) .
161 - // Add the source url as a comment to the thumb. A % is an
162 - // escape character in ImageMagick, so needs escaping (bug 23148)
163 - // http://www.imagemagick.org/script/escape.php?ImageMagick=i766ho1om315scce8eh75efjc6
164 - " -set comment " . wfEscapeShellArg( str_replace( '%', '%%', $descriptionUrl ) ) .
 161+ // Add the source url as a comment to the thumb.
 162+ " -set comment " . wfEscapeShellArg( $this->escapeMagickProperty( $comment ) ) .
165163 " -depth 8 $sharpen " .
166 - wfEscapeShellArg( $dstPath ) . " 2>&1";
 164+ wfEscapeShellArg( $this->escapeMagickOutput( $dstPath ) ) . " 2>&1";
167165 wfDebug( __METHOD__.": running ImageMagick: $cmd\n" );
168166 wfProfileIn( 'convert' );
169167 $err = wfShellExec( $cmd, $retval );
@@ -254,6 +252,90 @@
255253 }
256254 }
257255
 256+ /**
 257+ * Escape a string for ImageMagick's property input (e.g. -set -comment)
 258+ * See InterpretImageProperties() in magick/property.c
 259+ */
 260+ function escapeMagickProperty( $s ) {
 261+ // Double the backslashes
 262+ $s = str_replace( '\\', '\\\\', $s );
 263+ // Double the percents
 264+ $s = str_replace( '%', '%%', $s );
 265+ // Escape initial - or @
 266+ if ( strlen( $s ) > 0 && ( $s[0] === '-' || $s[0] === '@' ) ) {
 267+ $s = '\\' . $s;
 268+ }
 269+ return $s;
 270+ }
 271+
 272+ /**
 273+ * Escape a string for ImageMagick's input filenames. See ExpandFilenames()
 274+ * and GetPathComponent() in magick/utility.c.
 275+ *
 276+ * This won't work with an initial ~ or @, so input files should be prefixed
 277+ * with the directory name.
 278+ *
 279+ * Glob character unescaping is broken in ImageMagick before 6.6.1-5, but
 280+ * it's broken in a way that doesn't involve trying to convert every file
 281+ * in a directory, so we're better off escaping and waiting for the bugfix
 282+ * to filter down to users.
 283+ *
 284+ * @param $path string The file path
 285+ * @param $scene string The scene specification, or false if there is none
 286+ */
 287+ function escapeMagickInput( $path, $scene = false ) {
 288+ # Die on initial metacharacters (caller should prepend path)
 289+ $firstChar = substr( $path, 0, 1 );
 290+ if ( $firstChar === '~' || $firstChar === '@' ) {
 291+ throw new MWException( __METHOD__.': cannot escape this path name' );
 292+ }
 293+
 294+ # Escape glob chars
 295+ $path = preg_replace( '/[*?\[\]{}]/', '\\\\\0', $path );
 296+
 297+ return $this->escapeMagickPath( $path, $scene );
 298+ }
 299+
 300+ /**
 301+ * Escape a string for ImageMagick's output filename. See
 302+ * InterpretImageFilename() in magick/image.c.
 303+ */
 304+ function escapeMagickOutput( $path, $scene = false ) {
 305+ $path = str_replace( '%', '%%', $path );
 306+ return $this->escapeMagickPath( $path, $scene );
 307+ }
 308+
 309+ /**
 310+ * Armour a string against ImageMagick's GetPathComponent(). This is a
 311+ * helper function for escapeMagickInput() and escapeMagickOutput().
 312+ *
 313+ * @param $path string The file path
 314+ * @param $scene string The scene specification, or false if there is none
 315+ */
 316+ protected function escapeMagickPath( $path, $scene = false ) {
 317+ # Die on format specifiers (other than drive letters). The regex is
 318+ # meant to match all the formats you get from "convert -list format"
 319+ if ( preg_match( '/^([a-zA-Z0-9-]+):/', $path, $m ) ) {
 320+ if ( wfIsWindows() && is_dir( $m[0] ) ) {
 321+ // OK, it's a drive letter
 322+ // ImageMagick has a similar exception, see IsMagickConflict()
 323+ } else {
 324+ throw new MWException( __METHOD__.': unexpected colon character in path name' );
 325+ }
 326+ }
 327+
 328+ # If there are square brackets, add a do-nothing scene specification
 329+ # to force a literal interpretation
 330+ if ( $scene === false ) {
 331+ if ( strpos( $path, '[' ) !== false ) {
 332+ $path .= '[0--1]';
 333+ }
 334+ } else {
 335+ $path .= "[$scene]";
 336+ }
 337+ return $path;
 338+ }
 339+
258340 static function imageJpegWrapper( $dst_image, $thumbPath ) {
259341 imageinterlace( $dst_image );
260342 imagejpeg( $dst_image, $thumbPath, 95 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r65681MFT r65467 (and r64935, r64936, r64947 to sync the base): updated fix for bug...tstarling05:33, 30 April 2010
r65682MFT r65467 (and r64935, r64936, r64947 to sync the base): updated fix for bug...tstarling05:37, 30 April 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r64932(bug 23148) Escape % in ImageMagick comments. Patch by Derk-Jan Hartmancatrope12:38, 11 April 2010
r65401MFT r64932, bug 23148: ImageMagick comment escaping. Noted security implicati...tstarling15:35, 21 April 2010
r65402MFT r64932, bug 23148: ImageMagick comment escaping bugtstarling15:37, 21 April 2010

Comments

#Comment by Bryan (talk | contribs)   20:00, 9 January 2011

Can you check if this is related to bug 26233 ?

#Comment by Bryan (talk | contribs)   14:09, 10 January 2011

I've tried to read through the IM source code and I think this is ok. If nobody else is going to muddle through the IM source this can be marked ok.

Status & tagging log