SECURITY: Sanitize shell command args

Add validation and sanitization to several code paths.

Bug: 60339
Change-Id: Id124281d21ec730a2e0bbace843dd97194a712b4

diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php
index 7f108c8..a608657 100644 (file)
--- a/includes/media/Bitmap.php
+++ b/includes/media/Bitmap.php
@@ -307,27 +307,27 @@ class BitmapHandler extends ImageHandler {
                global $wgSharpenReductionThreshold, $wgSharpenParameter, $wgMaxAnimatedGifArea,
                        $wgImageMagickTempDir, $wgImageMagickConvertCommand;
 
-               $quality = '';
-               $sharpen = '';
+               $quality = array();
+               $sharpen = array();
                $scene = false;
-               $animation_pre = '';
-               $animation_post = '';
-               $decoderHint = '';
+               $animation_pre = array();
+               $animation_post = array();
+               $decoderHint = array();
                if ( $params['mimeType'] == 'image/jpeg' ) {
-                       $quality = "-quality 80"; // 80%
+                       $quality = array( '-quality', '80' ); // 80%
                        # Sharpening, see bug 6193
                        if ( ( $params['physicalWidth'] + $params['physicalHeight'] )
                                / ( $params['srcWidth'] + $params['srcHeight'] )
                                < $wgSharpenReductionThreshold
                        ) {
-                               $sharpen = "-sharpen " . wfEscapeShellArg( $wgSharpenParameter );
+                               $sharpen = array( '-sharpen', $wgSharpenParameter );
                        }
                        if ( version_compare( $this->getMagickVersion(), "6.5.6" ) >= 0 ) {
                                // JPEG decoder hint to reduce memory, available since IM 6.5.6-2
-                               $decoderHint = "-define jpeg:size={$params['physicalDimensions']}";
+                               $decoderHint = array( '-define', "jpeg:size={$params['physicalDimensions']}" );
                        }
                } elseif ( $params['mimeType'] == 'image/png' ) {
-                       $quality = "-quality 95"; // zlib 9, adaptive filtering
+                       $quality = array( '-quality', '95' ); // zlib 9, adaptive filtering
 
                } elseif ( $params['mimeType'] == 'image/gif' ) {
                        if ( $this->getImageArea( $image ) > $wgMaxAnimatedGifArea ) {
@@ -336,15 +336,15 @@ class BitmapHandler extends ImageHandler {
                                $scene = 0;
                        } elseif ( $this->isAnimatedImage( $image ) ) {
                                // Coalesce is needed to scale animated GIFs properly (bug 1017).
-                               $animation_pre = '-coalesce';
+                               $animation_pre = array( '-coalesce' );
                                // We optimize the output, but -optimize is broken,
                                // use optimizeTransparency instead (bug 11822)
                                if ( version_compare( $this->getMagickVersion(), "6.3.5" ) >= 0 ) {
-                                       $animation_post = '-fuzz 5% -layers optimizeTransparency';
+                                       $animation_post = array( '-fuzz', '5%', '-layers', 'optimizeTransparency' );
                                }
                        }
                } elseif ( $params['mimeType'] == 'image/x-xcf' ) {
-                       $animation_post = '-layers merge';
+                       $animation_post = array( '-layers', 'merge' );
                }
 
                // Use one thread only, to avoid deadlock bugs on OOM
@@ -356,26 +356,28 @@ class BitmapHandler extends ImageHandler {
                $rotation = $this->getRotation( $image );
                list( $width, $height ) = $this->extractPreRotationDimensions( $params, $rotation );
 
-               $cmd =
-                       wfEscapeShellArg( $wgImageMagickConvertCommand ) .
+               $cmd = call_user_func_array( 'wfEscapeShellArg', array_merge(
+                       array( $wgImageMagickConvertCommand ),
+                       $quality,
                        // Specify white background color, will be used for transparent images
                        // in Internet Explorer/Windows instead of default black.
-                       " {$quality} -background white" .
-                       " {$decoderHint} " .
-                       wfEscapeShellArg( $this->escapeMagickInput( $params['srcPath'], $scene ) ) .
-                       " {$animation_pre}" .
+                       array( '-background', 'white' ),
+                       $decoderHint,
+                       array( $this->escapeMagickInput( $params['srcPath'], $scene ) ),
+                       $animation_pre,
                        // For the -thumbnail option a "!" is needed to force exact size,
                        // or ImageMagick may decide your ratio is wrong and slice off
                        // a pixel.
-                       " -thumbnail " . wfEscapeShellArg( "{$width}x{$height}!" ) .
+                       array( '-thumbnail', "{$width}x{$height}!" ),
                        // Add the source url as a comment to the thumb, but don't add the flag if there's no comment
                        ( $params['comment'] !== ''
-                               ? " -set comment " . wfEscapeShellArg( $this->escapeMagickProperty( $params['comment'] ) )
-                               : '' ) .
-                       " -depth 8 $sharpen " .
-                       " -rotate -$rotation " .
-                       " {$animation_post} " .
-                       wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) );
+                               ? array( '-set', 'comment', $this->escapeMagickProperty( $params['comment'] ) )
+                               : array() ),
+                       array( '-depth', 8 ),
+                       $sharpen,
+                       array( '-rotate', "-$rotation" ),
+                       $animation_post,
+                       array( $this->escapeMagickOutput( $params['dstPath'] ) ) ) );
 
                wfDebug( __METHOD__ . ": running ImageMagick: $cmd\n" );
                wfProfileIn( 'convert' );
@@ -485,8 +487,8 @@ class BitmapHandler extends ImageHandler {
                $dst = wfEscapeShellArg( $params['dstPath'] );
                $cmd = $wgCustomConvertCommand;
                $cmd = str_replace( '%s', $src, str_replace( '%d', $dst, $cmd ) ); # Filenames
-               $cmd = str_replace( '%h', $params['physicalHeight'],
-                       str_replace( '%w', $params['physicalWidth'], $cmd ) ); # Size
+               $cmd = str_replace( '%h', wfEscapeShellArg( $params['physicalHeight'] ),
+                       str_replace( '%w', wfEscapeShellArg( $params['physicalWidth'] ), $cmd ) ); # Size
                wfDebug( __METHOD__ . ": Running custom convert command $cmd\n" );
                wfProfileIn( 'convert' );
                $retval = 0;
@@ -790,7 +792,7 @@ class BitmapHandler extends ImageHandler {
                        case 'im':
                                $cmd = wfEscapeShellArg( $wgImageMagickConvertCommand ) . " " .
                                        wfEscapeShellArg( $this->escapeMagickInput( $params['srcPath'], $scene ) ) .
-                                       " -rotate -$rotation " .
+                                       " -rotate " . wfEscapeShellArg( "-$rotation" ) . " " .
                                        wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) );
                                wfDebug( __METHOD__ . ": running ImageMagick: $cmd\n" );
                                wfProfileIn( 'convert' );

diff --git a/includes/media/DjVu.php b/includes/media/DjVu.php
index 7da5a4c..1202c9a 100644 (file)
--- a/includes/media/DjVu.php
+++ b/includes/media/DjVu.php
@@ -180,9 +180,12 @@ class DjVuHandler extends ImageHandler {
                $srcPath = $image->getLocalRefPath();
                # Use a subshell (brackets) to aggregate stderr from both pipeline commands
                # before redirecting it to the overall stdout. This works in both Linux and Windows XP.
-               $cmd = '(' . wfEscapeShellArg( $wgDjvuRenderer ) . " -format=ppm -page={$page}" .
-                       " -size={$params['physicalWidth']}x{$params['physicalHeight']} " .
-                       wfEscapeShellArg( $srcPath );
+               $cmd = '(' . wfEscapeShellArg(
+                       $wgDjvuRenderer,
+                       "-format=ppm",
+                       "-page={$page}",
+                       "-size={$params['physicalWidth']}x{$params['physicalHeight']}",
+                       $srcPath );
                if ( $wgDjvuPostProcessor ) {
                        $cmd .= " | {$wgDjvuPostProcessor}";
                }

diff --git a/includes/media/ImageHandler.php b/includes/media/ImageHandler.php
index 4dd79a8..8a12e7e 100644 (file)
--- a/includes/media/ImageHandler.php
+++ b/includes/media/ImageHandler.php
@@ -93,6 +93,7 @@ abstract class ImageHandler extends MediaHandler {
                if ( !isset( $params['page'] ) ) {
                        $params['page'] = 1;
                } else {
+                       $params['page'] = intval( $params['page'] );
                        if ( $params['page'] > $image->pageCount() ) {
                                $params['page'] = $image->pageCount();
                        }