3
\$\begingroup\$

On web pages, obviously we cannot serve large photos (e.g. 4000x4000px) and just let CSS do the resizing: it would be too slow for end users and too bandwidth-consuming for the server.

So it's important to rescale photos to the desired size that will be actually used on the website. For this reason I usually resized the image with a tool like Photoshop to, say 800px width. But then later you always have the case "I would finally have preferred 10000px instead! and then ... you have to export the JPG again, re-upload, etc. Annoying!

Instead here is an automated solution in which you upload the top-quality version (never served to end users because it would be too big), and the tool automatically resizes on-demand.

Here is code that:

  • serves myphoto.jpg if present in current folder
  • serves myphoto!500.jpg if present in current folder
  • creates myphoto!500.jpg by rescaling myphoto.jpg with a width of 500px if it is not done yet. So as it will be saved/cached to a file, next requests will be fast because the server will just serve the pre-cooked file

.htaccess

RewriteEngine On
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^(.*)$ index.php [QSA,L]

index.php

<?php
$req = urldecode(basename($_SERVER['REQUEST_URI']));
list($f, $size) = explode('!', $req);
list($size, $ext) = explode('.', $size);
$size = intval($size);
if (($size > 2000) || ($size % 50 != 0))
    die();
$f .= '.' . $ext;
list($width, $height) = getimagesize($f);
$src = imagecreatefromstring(file_get_contents($f));
$dst = imagescale($src, $size);
imagejpeg($dst, $req);
?>

Question for code review: is there something to improve in terms of security?

Note: To avoid the scenario that someone requests myimage!1.jpg, myimage!2.jpg, ..., myimage!1999.jpg, myimage!2000.jpg in loop, filling the disk for nothing, the code only accepts size as integer multiples of 50. At most 50 100 150 ... 1900 1950 2000 will be written, i.e. 40 versions.

\$\endgroup\$
2
  • \$\begingroup\$ I added the .htaccess tag, because without understanding what that does you cannot understand the PHP code. \$\endgroup\$ Dec 2, 2021 at 19:25
  • 2
    \$\begingroup\$ Am I incorrect in reading that you would like help adding features to this script? I mean, it is not currently filtering/sanitizing/validating files the way you desire. We are not meant to bake new functionality in your supplied code as far as I know. When you know how many elements your explode should return, a limit parameter is a good idea. My mind went to sscanf() when I saw that you wanted an int val from the middle of the string. \$\endgroup\$ Dec 3, 2021 at 5:44

2 Answers 2

1
\$\begingroup\$

Analysis

The code looks okay - not difficult to read. The description does not state which image formats should be supported however the code appears to output resized images as JPEG images. It may be wise to keep the image format - e.g. GIF, PNG, JPG, etc.

Suggestions

Pattern matching

A regular expression could be used to both ensure the request URI matches the expected format and also parse out the components. While it may be slightly slower the difference should be negligible. One could use preg_match() or if Unicode string support is needed then mb_eregi() could be used. A pattern could be crafted to ensure:

  • there is not more than one exclamation mark
  • The number that follows the exclamation mark is positive and has a number of digits within a certain range (e.g. 1-5)
  • The extension is within the acceptable list of extensions

Doing this should allow removal of the multiple calls to split the string using explode(). For example:

preg_match('#^([^!\./]+)!(\d+)\.(png|jpg|gif)#i', $req, $matches);
if (!$matches) {
    die();
}
[$uri, $name, $size, $ext] = $matches;
  • ^ asserts position at start of the string
  • 1st Capturing Group ([^!\./]+) Match a single character not present in the list below [^!\./] + matches the previous token between one and unlimited times, as many times as possible, giving back as needed (greedy)
    • Could be changed to ([a-z0-9]+) to only allow alphanumeric characters or (\w+) to allow [A-Za-z0-9_]
  • ! matches the character ! with index 3310 (2116 or 418) literally (case insensitive)
  • 2nd Capturing Group (\d+)
    • \d matches a digit (equivalent to [0-9])
    • + matches the previous token between one and unlimited times, as many times as possible, giving back as needed (greedy)
  • \. matches the character . with index 4610 (2E16 or 568) literally (case insensitive)
  • 3rd Capturing Group (png|jpg|gif)
    • 1st Alternative png png matches the characters png literally (case insensitive)
    • 2nd Alternative jpg jpg matches the characters jpg literally (case insensitive)
    • 3rd Alternative gif gif matches the characters gif literally (case insensitive)
  • Global pattern flags
    • i modifier: insensitive. Case insensitive match (ignores case of [a-zA-Z])

See the regular expression pattern explained here.

Named sub-patterns can be named to allow the array of matches to have keys that are easier to work with.

For example:

preg_match('#^(?P<name>[^!\./]+)!(?P<size>\d+)\.(?P<ext>png|jpg|gif)#i', $req, $matches);

Here are the parts of the pattern explained:

  • opening delimiter: #

  • Start of line anchor: ^

  • match one or more chars for file name- anything that isn’t an exclamation mark, dot or slash: (?P<name>[^!\./]+)

    Could be changed to (?P<name>[a-z0-9]+) to only allow those characters

  • the exclamation mark: !

  • The size - digits only (?P<size>\d+)

  • Match a literal dot \.

  • The extension (?P<ext>png|jpg|gif)

  • closing delimiter: #

  • Case insensitive modifier flag i

If the pattern is matched then the array $matches would look like this:

array ( 
    0 => 'myphoto!500.jpg', 
    'name' => 'myphoto', 
    1 => 'myphoto', 
    'size' => '500', 
    2 => '500', 
    'extension' => 'jpg', 
    3 => 'jpg', 
)

Destructuring arrays

Like was used in the previous section, As of PHP 7.1 array assignment can be used to destructure arrays1. Instead of using list() when exploding strings like the request into two parts, a simple array can be used. This may not save much processing time since list() is just a language construct2 but it is simpler to type.

So instead of these lines:

list($f, $size) = explode('!', $req);
list($size, $ext) = explode('.', $size);

Those could be simplified to:

[$f, $size] = explode('!', $req);
[$size, $ext] = explode('.', $size);

Unused variables

The variables declared here:

list($width, $height) = getimagesize($f);

appear to be unused. Those can be eliminated.

Updated code

I created a repository with a docker file and container file so I could test it on http://localhost:8082. I also updated the code to utilize the suggestions above and output the image to the browser.

<?php
$req = urldecode(basename($_SERVER['REQUEST_URI']));
preg_match('#^(\w+)!(\d+)\.(png|jpg|gif)#i', $req, $matches);
if (!$matches) {
    die();
}
[, $name, $size, $ext] = $matches;
$size = intval($size);
if (($size > 2000) || ($size % 50 != 0))
    die();
$src = imagecreatefromstring(file_get_contents($name . '.' . $ext));
$dst = imagescale($src, $size);
$function = 'image' . ($ext == 'jpg' ? 'jpeg' : strtolower($ext));
header('Content-Type: image/' . strtolower($ext));
$function($dst, $req);
$function($dst);    //send the image to the browser
\$\endgroup\$
6
  • \$\begingroup\$ Thank you very much for your answer. Can you add details about what the regex is catching: in particular, what characters are allowed in the filename before the !? Is this regex safe if the filename uses things like ../../<strange_utf8_char>hello.jpg, or are there similar tricks that can be malicious, as a URL? \$\endgroup\$
    – Basj
    Dec 19, 2021 at 9:24
  • \$\begingroup\$ Hey sorry it took so long- I’ve been traveling the past few days. I updated the answer to explain the pattern and offer possible changes to only allow certain characters for the name \$\endgroup\$ Dec 22, 2021 at 15:41
  • \$\begingroup\$ Thanks @SᴀᴍOnᴇᴌᴀ! Would you use empty($matches) here to test if there are matches or not? \$\endgroup\$
    – Basj
    Dec 22, 2021 at 15:46
  • \$\begingroup\$ yes- I was considering suggesting that if (count($matches) < 4) { then it have a die() but yes checking empty($matches) should also suffice \$\endgroup\$ Dec 22, 2021 at 18:18
  • 1
    \$\begingroup\$ You're welcome! I updated my answer to include the code I used for testing, as well as docker files for a container to run it locally in a sandbox environment. \$\endgroup\$ Feb 28 at 17:46
3
\$\begingroup\$

If this were my code I'd be totally distrustful of the URI presented by my user. In other words I would assume that the results of the explode operations $f, $size, and $ext contained malicious garbage.

I would wrap the whole thing in a try{} block. I would check the values of $size and $ext against explicit allowlists. I would examine the path in $f to rigorously reject stuff like ../ and other file system relative navigation operations. I would restrict the character set allowed in each level of the path to, probably to numbers and ASCII letters, excluding anything else.

I would check files before opening them, for existence, for having a byte size less than some arbitrary threshold (no 70MiB jpg files, thank you very much). I'd also check whether the dimensions were reasonable (no 10k x 50k pngs, please).

If any of these checks failed, I would respond "sorry" to the (untrusted) user without giving the slightest detail about which check failed.

You're not paranoid when you believe highly motivated cybercriminals are plotting against you. They are plotting against you.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your answer! How would you for example sanitize $f to reject things like ../? Is there a PHP built-in way to do this? If you have some code example for the tests you're mentioning, you're welcome to include it in the answer. Thanks! \$\endgroup\$
    – Basj
    Dec 12, 2021 at 15:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.