List of steps to reproduce (step by step, including full links if applicable):
- Set in LocalSettings.php:
$wgUseImageMagick = false; $wgCustomConvertCommand = "gm convert %s -resize %wx%h %d";
The $wgCustomConvertCommand command is the example from https://www.mediawiki.org/wiki/Manual:$wgCustomConvertCommand.
- Upload an image file big enough to require thumbnailing (I used 600×600). Give it the destination filename Test%w.png.
What happens?:
See error message in the Thumbnail column:
Error creating thumbnail: gm convert: Unable to open file (/var/www/mediawiki/w/images/8/84/Test120.png) [No such file or directory].
See in $wgDebugLogFile:
[thumbnail] thumbnail failed on <hostname>: error 1 "gm convert: Unable to open file (/var/www/mediawiki/w/images/8/84/Test120.png) [No such file or directory]." from "gm convert '/var/www/mediawiki/w/images/8/84/Test'120'.png' -resize '120'x'120' '/tmp/transform_4f4696c1492a.png'"
What should have happened instead?:
A thumbnail should have been created in /var/www/mediawiki/w/images/thumb/8/84/Test%w.png/120px-Test%w.png.
Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc.:
MediaWiki 1.37.2
$wgCustomConvertCommand is supposed to have the magic strings %d, %s, %w, and %h replaced by the source path, destination path, width, and height, respectively. What's going wrong is that BitmapHandler::transformCustom does the replacements sequentially, not simultaneously. %s gets replaced with a source path that contains %w. Then, that newly introduced %w is replaced with the width (in addition to the %w that was already present in $wgCustomConvertCommand).
Something like this may work better:
$cmd = $customConvertCommand;
$src = Shell::escape( $params['srcPath'] );
$dst = Shell::escape( $params['dstPath'] );
$w = Shell::escape( $params['physicalWidth'] );
$h = Shell::escape( $params['physicalHeight'] );
$cmd = preg_replace_callback('/%[dswh]/',
function ($m) use ($dst, $src, $w, $h) {
return [
'%d' => $dst,
'%s' => $src,
'%w' => $w,
'%h' => $h,
][$m[0]];
},
$cmd
);I did not find any security vulnerabilities caused by this bug. The only substitution an attacker can control flexibly is %s, and the %w and %h substitutions that happen later cannot contain shell metacharacters because they are decimal integers. But the pattern of sequential substitution is fragile (in the debug log above, observe how the 120 is outside the shell quoting), and could give rise to shell injection vulnerabilities if other code were to change. For example, if File::makeTransformTmpFile were to use the basename of the source file in its temporary destination filename, then uploading a file with a name like %s;echo hacked;echo .png would result in shell execution. The same could happen if, in the future, another substitution string other than the four already supported were added.