TYPO3 Core

I've attached a patch (made on trunk). Could you please test if it works for you?

It happens also with 4.1.13

Even if not using GM, the thumbs generation is broken. The difference is that IM doesn't hangs, instead it returns with no output.

I've tried the patch on 4.2.10 with GM and safe-mode = on but with no success.

Thanks for testing.

What the patch does:
- prevents double escaping if safe_mode is enabled
- makes sure that in all cases arguments are in single quotes

If the patch does not work (like you claim), then you are most like (additionally) affected by #0011993.

The frame indicator (being part of the filename and thus encapsulated in single quotes) is the problem. As temporary solution set
$TYPO3_CONF_VARS['GFX']['im_noFramePrepended'] = '1';
in your localconf.

Configuration description:
"If set, the [x] frame indicator is NOT prepended to filenames in stdgraphic. Some IM5+ version didn't work at all with the typical [0]-prefix, which allow multipage pdf's and animated gif's to be scaled only for the first frame/page and that seriously cuts down rendering time. Set this flag only if your ImageMagick version cannot find the files. Notice that changing this flag causes temporary filenames to change, thus the server will begin scaling images again which were previously cached."

-> This workaround will raise your server workload.

SUMMARY:
The problem IS the frame indicator (1) now being encapsulated by single quotes. Before the command injection vulnerability fix had been committed to the Core, file resources were encapsulated with double quotes only if the file contained spaces. So practically, file resources where seldom encapsulated by quotes. That's why safe_mode users haven't faced this problem.

Find attached version 3 of a patch that moves the frame indicator out of the single quoted file resources. With that patch, above mentioned workaround is obsolete.

(1) looks like this: /var/www/path/to/fileadmin/image.jpg⁰

You are right about the Frame Indicator being the most visible cause of the problem. With "clean" filenames it's the only source of the problem. But moving the frame indicator out of the escaping doesn't solve the problem correctly as files with "escaped" characters can also be valid filenames. So if a user uploads a file like "MyPhoto[2009-10-26].jpg" your patch will have no effect since the filename will not be "unescaped" by the shell correctly leading to the hanging processes again.

Just to show the different results of the wrapFilenames() function with safe_mode = On and Off here's the output of a filename including the frame indicator run thru that function in both situations:

safe_mode = Off:
clean-filename.jpg\[0\]

safe_mode = On:
clean-filename.jpg\\\[0\\\]

This is what the shell produces when it "unescapes" the parameters:

sh -c "echo clean-filename.jpg\[0\]" => clean-filename.jpg⁰
sh -c "echo clean-filename.jpg\\\[0\\\]" => clean-filename.jpg\[0\]

Using graphicsmagick from the commandline you can reproduce the hanging of the process using the \\\[0\\\] Version.

Your above (claimed) results of wrapFilenames() are wrong; the resulting string is independent from safe_mode settings. This method simply puts the file resources in single quotes and escapes single quotes in the string.

The problem is the later implicit call of escapeshellcmd() if safe_mode is enabled. This results in special characters (in the file resource) being escaped. As the file resource is encapsulated by single quotes [by wrapFilenames()], the shell is unable to "unescape" the special characters later on.

If escapeshellarg() would be disabled for safe_mode users, then there were other users with spaces in filenames who complain about non working graphics rendering.

A file similar to "myImageFile &Spaces[2009-10-26].jpg" would be a nice testcase for the most perfect solution on safe_mode systems.

Sorry, in my testfile I mistakenly used escapeshellcmd() instead of escapeshellarg() as a remains of some test combinations to get a valid filename.

Anyhow, your filename isn't a complete testcase as a lot more "problematic" shell meta characters are valid filename components. A more thorough testcase would be a filename like this:

mypic_ &testfile_2²⁰⁰⁹;allvalid"characters'".jpg

The problem is that php is unable to produce a valid shell command with such a filename in safe_mode environment due to only working with a single string as argument to exec.

Valid escapings in above case would be:

1) 'mypic_ &testfile_2²⁰⁰⁹;allvalid"characters'\''".jpg'
2) mypic_\ \&testfile_2\[2009\]\;allvalid\"characters\'\".jpg

Result #1 is being produced by escapeshellarg()

An reduced version of the GM command used by TYPO3 would be something like

gm convert 'mypic_ &file_2²⁰⁰⁹;allvalid"characters'\''".jpg' '/tmp/output.jpg'

Would this be sent unchanged to the shell everything would be just fine. But PHP uses escapeshellcmd() on above cmd and this results in

gm convert 'mypic_ \&testfile_2\[2009\]\;allvalid\"characters'\\''\".jpg\' '/tmp/output.jpg'

being passed to the shell. Mostly due to the single-quotes this will not get unescaped to a correct argument and the file will not be found.

Due to my testresults I would consider this to be more of a bug in PHP than in TYPO3 as there doesn't seem to be a way to make PHP produce a valid commandline.

Not using escapeshellarg() and manually escaping some characters like spaces manually won't work either as the (correctly) escape sequence will get double-escaped by safe_mode again.

As far as I can see, there will always be filenames which won't work in conjunction with safe-mode. I would consider checking for "problematic" filenames and using a informational image that signals the problem with the filename to the user instead.

so as conclusion all picture filenames should be cleaned before copying them to upload folder, then it should work in any case, right?

This will reduce the impact of this bug for sure. Extensions that do their "own" file-handling or work with files uploaded by FTP/not being copied to the uploads folder still might have problems with uncleaned filenames.

I think sanitizing all filenames uploaded with/copied by TYPO3 is the right step, but I would advocate an additional check for invalid characters in filenames and redirecting to a "safe" filename showing a warning to the user.

Knowingly allowing situations which could produce hanging processes/crashing servers is just too severe to not warrant such a check, especially since calling ImageMagick/GraphicsMagick is such a heavyweight operations that a few additional CPU cycles for a strpos/preg_match won't matter much.

If a user is allowed/able to circumvent any TYPO3 mechanism (creation of files via shell, upload via FTP/SFTP/SCP), you would still face this problem. Then such user is able to create filenames with special characters/whitespaces.

I think of following solution in pseudocode:

if (imageRenderingProcessRequest & safe_mode enabled) {
// check for existance of special characters
if (filename != escapeshellcmd(filename)) {
rename(filename,cleanFileName(fileName));
}
// process Request with the cleaned Filename
}

function cleanFileName($fileName) {
$fileName = escapeshellcmd($fileName)
search for positions of '\' in $fileName and foreach remove this and following position
return $fileName
}

But renaming files without user interaction might break other things. On the other hand, a warning only for problematic files won't be read by a user for every case a rendering process has been started.

Renaming files which are not under explicit TYPO3 control is a no-go.

In addition to the problem of modifying files under control of a different tool and breaking that there is the good chance that in such a case only read-access is available to a file. Then your proposed solution would break due to not being able to rename the file.

A saner way to sanitize a filename would be something like:

$inputName = preg_replace('#[^A-Za-z0-9\-_\.]+#','',$inputName);

if user don't use the TYPO3 API, we can't do something against. There are several ways to produce rubbish ...

So i would like to sanitize filenames in API for the upload.

@Marcus - i ahree that rename is a no-go. But we coud throw an exception if the file is "unescapable"

Morton, thanks for your note 31705, which pretty much sums up the real problem here. PHP is not able to create a escaped shell command in safe_mode=on.

So in my eyes we need a solution that:

a) always works in safe_mode=off (which is the case now already)

b) is security-aware in either safe_mode

c) doesn't rely on "renaming" files which we cannot know where they come from or even if we have the right to rename them.

d) the compromise: in safe_mode=on, works on most filenames which doesn't contain exotic characters in it. As far as I understood only the single-quote quoting of escapeshellarg() is doing the harm. Users with safe-mode should be made aware of that (there is no known solution to that in PHP in general, as Morton found out)

Agreed?

To sum up once again:

Either * leave it as it is as safe_mode is soon to be dropped from PHP packages and with chroot, suexec, fcgid, open_basedir there are methods available that provide equal or better security that safe_mode * we do some kind of sanitizing on file names shortly before image prcessing happens, by that accept any filename and make image processing working in any case or * drop escapeshellarg() for safe_mode users and accept that files with whitespaces won't work for image processing

We can safely drop escapeshellarg() for safe_mode users as the implicit call of escapsehllcmd() will make sure that no command injection will happen.

@Ernesto,@Steffen

I think sanitizing (new) files in the upload folder by way of the API is the way to go. Additionall we need to move the appending of the frame indicator after the filename has been run through wrapFilename, otherwise we will run into the problem due to [] showing up in the filename. Since we can't change existing content an additional safeguard for non-valid files (in safe_mode) should be considered.

My proposal would be perform a test like this
1) pass $inputfilename through escapeshellarg() like we do in 4.2.10
2) if safemode is enabled run $inputFilename thru escapeshellcmd() and check for a backslash character. If found consider the filename invalid and skip processing the file, producing a "soft" failure where no image gets rendered. Additionally an error could get thrown if debug mode is enabled.

Dear all,

as the patch does not fix the problem on some systems (don't know why), I wrote a hotfix. But you can only use this hotfix if it is your webserver of if your hoster will do it for you.

The idea is to "import" the parameters into another script before starting e.g. combine (so that the other script will remove the backslash). I started trying it with a bash-script, but I failed in removing the unallowed characters from the parameters (and if you do not remove them, you could also disable the safemode as you are not longer limited to /usr/local/php/bin any more), so I wrote it in PHP.

1. Move to your servers php_safemode_exec_dir, e.g. /usr/local/php/bin

2. Rename the binary file "convert" to "convert_binary"

3. Create a new file "convert" with the script below and make it executable.

4. Have fun!

5. Warning: Use it as your own risk! Even if I substitute "unallowed" characters, nobody is perfect and this is a very hot fix.

6. If necessary, you can repeat steps 2 and 3 also for the other files (such as combine). With the first tests, it is not necessary, so you better keep them as they are.

Hope that helps,

Joern

#!/usr/bin/php5

/*
Patch for http://bugs.typo3.org/view.php?id=12341
"Unmasks" the parameters before sending it to the convert/combine-binary

(c) 2009 Joern Dost, D&T Internet GmbH
*/

// $sCommand will contain the command we have to execute
$sCommand = '';

// Use every parameter and...
foreach ($argv as $iCounter => $sArgument) {

// filter out the valid chars:
  preg_match('/[-.A-Za-z0-9_\[\]\/!+]*/', $sArgument, $aParameter);

if ($iCounter==0) {
      // Parameter 0 contains the call of this script. We change the
      // filename e.g. from /path/to/combine to /path/to/combine_binary
      $sCommand = $aParameter[0].'_binary';
  } else {
      $sCommand.=' '.$aParameter[0];
  }

}

// That's all. Now execute it:
echo shell_exec($sCommand);

?>

UPDATED Nov 12, 2009: You better should use "echo shell_exec..." instead of "shell_exec..."

Dear all,

now i'm just a little bit confused

There are three patches available, but the second includes the content of the first patch.
So does it really make scense, to show both patches (1+2) here?

@joern
to rename an existing binary is imho not a realy good sollution!
what happens if you use convert by another scripts?
what happens by an update of the imagemagick tools?

We try all patches on a typo3 installation, but there didn't fix the thumbnail Problem in the Backend.

Hi Michael:

to rename an existing binary is imho not a realy good sollution!

ACK. I never said it is a good solution. But it is a solution that works. And as I had the same problem like you - we tried all the patches, they didn't fix the problem - it is right now the only solution for me.

what happens if you use convert by another scripts?

Normally: Nothing special. My script just calls the binary with all the parameters -> so everything should be the same.

There was a small bug: I forgot the "echo" in the last row -> combine-calls to handle files work fine, but combine-calls e.g. to print out the combine-version will not make any output, so an old Typo3-version said "sorry, no ImageMagick installed". After fixing it, I do not know any problems handling request of other websited.

what happens by an update of the imagemagick tools?

I don't expect any updated for IM4 any more ;-)

As I said, it is just a hotfix until Typo3 fixes the problem itself. And I never said it is a very nice solution. But for now, it is the only solution that works (here), so decide yourself if you want to use it .

Best Regards,

Joern

Hi,

Moving the frameindicator out of wrapFilename() seems a sensible thing.
For spaces, etc in 4.2.8 it works and gm won't hang on others, just a file not found.
(wrapFileName() doesn't use escapeshellarg() in 2.4.8)

t3lib/thumbs.php #235
$parameters = '-sample '.$this->size.' '.$colors.' '.$this->wrapFileName($this->input) .'[0]'.' '.$this->wrapFileName($this->output);

Hi,

for me in 4.3.0 the patches dont't work in BE, the use of thumbs.php causes gm to hang again. Only way to fix it for me was to remove the '[0]' in thumbs.php in line 239

$parameters = '-sample '.$this->size.' '.$colors.' '.$this->wrapFileName($this->input.'[0]').' '.$this->wrapFileName($this->output);

im_noFramePrepended = 1 setting does not affect here.

For me, the patch v3 seems to fix the issue.
I haven't tested it thoroughly, but for a normal file (filename containing only alpha-numerical characters) it works!

I use the solution of Joern Dost, thanks a lot for it.
I only had one problem concerning thumnails of PDFs.

I got animated gifs with one image for each page. The reason was, that the command to produce such thumbnails was looking like this:
convert -sample 56x56 -colors 16 SOURCE_PATH/FacelVega.pdf\[0\] DEST_PATH/tmb_c171328cd1.gif

Because of the backslashes before the brackets, the resulting command would be:
convert -sample 56x56 -colors 16 SOURCE_PATH/FacelVega.pdf DEST_PATH/tmb_c171328cd1.gif

To handle this problem I had to modify the line, filtering out the valid chars to also accept the backslash.

So my version of this line is:
// filter out the valid chars:
preg_match('/[-.A-Za-z0-9_\\\[\]\/!+]*/', $sArgument, $aParameter);

Now everything is fine.

Manfred

Alexander, can you please test if v4 solves the issue for you as well?

Patch v4 works for me, at least for a normal file (filename containing only alpha-numerical characters)

Thanks to Marcus Krause, Helmut Hummel and Bernhard Kraft!

Commited to trunk (rev 6722)
Commited to TYPO3_4-3 (rev 6723)
Commited to TYPO3_4-2 (rev 6724)

Thanks to Marcus Krause, Helmut Hummel and Bernhard Kraft!

Commited to trunk (rev 6722)
Commited to TYPO3_4-3 (rev 6723)
Commited to TYPO3_4-2 (rev 6724)