Special chars in filenames not handled for PDF thumbnails for Imagemagick

steveatsilver

Hi Karl

Hope you're having a great weekend. We've had lots of fun playing with files.gallery so far, and using the new custom click handler.

However we have a potential bug report for you, relating to the generation of PDF thumbnails (jpg from pdf). Generally speaking, this works well if you have Imagemagick and GhostScript installed.

However we have found an issue that only happens when the filename of the PDF contains certain special characters. We have tried to debug it and found it seems to be how these special characters are being passed into the command line via the php exec() function for imagemagick (i.e. magick or convert)

Here are some examples to help you reproduce the issue:

Example HTTP POST - Error 500

Request URL:
/Media/index.php?action=file&file=CROWN HUNTER TRAIL 2020 Programme (2).pdf&resize=pdf&1748772137.1119234
Response Body:
<h3>Error</h3>Error generating pdf preview image ($result_code 1).

Example filenames with special characters which fail to generate a PDF thumbnail. We have tried these with the exact same PDF file, but with different filenames to prove that it's not the actual PDF that's the problem, just the filename.

Example 1

Fails:
/Media/CROWN HUNTER TRAIL 2020 Programme (2).pdf
Works:
/Media/CROWN HUNTER TRAIL 2020 Programme.pdf
Reason/Issue:
The rounded brackets () are causing the problem when passed to the command line for ImageMagick

Example 2:

Fails:
/Media/Area 5 HT Qual & UnaffPC 13th June 2021.pdf
Works:
/Media/Area 5 HT Qual UnaffPC 13th June 2021.pdf
Reason/Issue:
The ampersand character (&) is causing the problem when passed to the command line for ImageMagick

Some characters seem fine such as spaces and hyphens, but we think some are messing up the command.

Are you able to take a look at this when you get chance? Thanks in advance as always 🙂

Karl

steveatsilver I checked and I can confirm the issue with these characters. Here's the deal: A few months ago, somebody reported a vulnerability in Files Gallery, related to the script not using escapeshellarg():
github.com/beune/CVE-2024-53615

This was fixed, so now we are using escapeshellarg(). Current code in index.php:

// path to ffmpeg in command-line is normally just 'ffmpeg', but escapeshellarg() in case using absolute path
$path = escapeshellarg(Config::get($app_path));
// this will work for all characters, but is apparently unsafe:
//$path = '"' . str_replace('"', '\"', Config::get($app_path)) . '"';

The problem with escapeshellarg(), is that it seemingly tries to escape certain "insecure" characters that you don't want escaped. So basically, we can revert to a version that doesn't escape certain characters (like in your examples), but then It's no longer secure. For the record, it's only insecure if you don't trust the users who have upload access, but nevertheless insecure, and we can't officially revert to the previous code (commented out in the above example). You could do though.

I'm really not sure what a better solution could be here. It might be possible to search-and-replace certain characters to make them work, but we must stick with escapeshellarg() to sanitize the string.

Karl

I think you might want to re-check to be entirely sure. I did test the vulnerability after it was reported, and I can confirm that it was able to trigger commands from the file name, even if the file name was within double-quotes. Please see an extended report here (I'm not making the link clickable because I don't want to promote it):
beune.dev/posts/my-first-cve/

I can't remember exactly how I tested, but it was something like below. Create a file that poses as mp4 (using magic bytes) and names it as a file command:

$ echo "AAAAIGZ0eXBpc29tAAACAGlzb21pc28yYXZjMW1wNDEAAQv7bW9vdgAAAGxtdmhk" | base64 -d > '$(ping localhost).mp4'

To block this behavior, escapeshellcmd() was required.

Karl This was fixed, so now we are using escapeshellarg(). Current code in index.php:

Sorry, I was highlighting wrong code in my previous reply. As you seem to have figured out, the affected code would be the one that triggers escapeshellcmd(), not escapeshellarg().

steveatsilver

Thanks for the reply

We've taken a deeper look at this and on windows, it escapes with a caret/circumflex character: ^ whilst on Unix based systems it escapes with a backslash character: \

But by the time the command line executes, the filesystem basically cannot find those files, so it's definitely not quite the right implementation at present, as it leads to some missing thumbnails for PDFs and presumably videos would have a similar problem when ffmpeg is used.

As you've suggested, we have temporarily hacked the PHP code on line 1559 to remove the escapeshellcmd() as we feel it's unncessary here, because what it escapes is already contained within double quotes so the special characters wont be having any effect, they are simply part of the actual filename until the end double quotes. I suppose the 'gotcha' is if someone added a double quote in a filename (however this is already forbidden by the Windows operation system), then potentially it could break out of the filename string and special characters would start doing funky stuff again. So maybe the best solution would be to simply escape double quotes with a backslash instead of using escapeshellcmd()

So currently our hacked code is now:

// get exec command string
    $cmd = str_replace(
      ['%APP_PATH%', '%PATH%', '%CACHE%', '%RESIZE%'],
	  [$app_path, $this->path, $cachepath, $resize],
      self::${"preview_cmd_$type"});

We've tested on both windows and linux and this now handles both the examples of special characters correctly with the PDF thumbnails being generated perfectly. Love the lazy-loading by the way, it feels very efficient too 🙂

It might be worth double checking where you wrap the various paths in double quotes, but we think this is already sufficient to not need to escape the $this->path using escapeshellcmd()

Ps. We've had a successful weekend of integrating files.gallery into our CMS and it's working pretty well, so we're very happy! May have a couple of other minor bits of feedback and little things for the wishlist, so will post these soon

Karl

steveatsilver This issue has now been solved in Files Gallery 0.15.0.

steveatsilver we have temporarily hacked the PHP code on line 1559 to remove the escapeshellcmd()

There is no longer any need to remove the escape functions. Special characters in file names will work correctly, also when escaped, which should remain for security reasons. One of the issues previously was related to duplicate quotes in commands after escaping.

steveatsilver

Ok thanks, apologies, we are ensuring authenticated / known users so less of an issue for our implementation but good to know!

steveatsilver

Karl Thanks thats great!