HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Security: Scale and cache images

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
securitycacheandimagesscale

Problem

Specification

A simple PHP script resizes images on-the-fly. The script is called by the web server's 404 handler to return a scaled version of the original. For example, if the original image is at:

http://localhost/images/filename.jpg


Then valid requests for scaled versions include:

http://localhost/images/filename-200x.jpg
http://localhost/images/filename-x200.jpg
http://localhost/images/filename-200x200.jpg


All requests maintain the aspect ratio. The first request scales to the image to 200px wide; the second scales the image to 200px high; the third request ensures neither the width nor height exceeds 200x200px.

This is similar to how Google Picasa works.

Assumptions

The code is called with the following known constraints:

  • The file names are hashed.



  • The images are valid.



  • The images are in a format recognized by the WideImage library.



  • Each image typically resides in its own folder, nested a few directories deep.



Questions

I am wondering:

  • What security holes are present?



  • How would you address them?



  • What optimizations are possible (e.g., use a faster library)?



Other non-security criticisms are also welcome.

Code

The code follows:

```

* filename-#x.ext
* filename-x#.ext
* filename-#x#.ext
*
*
* The examples specify: (1) width; (2) height; (3) width and height. The
* aspect ratio is maintained.
*
* For security purposes, only a specific set of dimensions are allowed.
*/

include 'WideImage/WideImage.php';

// Limit the number of possible cached images (i.e., do not allow arbitrary
// dimensions to create cached files).
$ACCEPT_DIMENSIONS = array( 200, 1024, 2048, 4096 );

// The "images" suffix comes from the URL.
$IMAGES_DIRECTORY = "/home/data";

$DIVIDER = "x";

/**
* Given an integer value, this will find its nearest value within an
* array of values.
*
* @param $needle The value whose nearest value is sought.
* @param $haystack The valid values.
*
* @return The value in $haystack that i

Solution

This code does not do what the comment says:

// Locate the position of the last hyphen in the filename.
$index = strpos( $filename, "-" );


Consider parsing the path using a regular expression.

For security, I've explicitly listed the allowed characters for the image filename.

# You probably want to analyze PATH_INFO rather than QUERY_STRING
$path = pathinfo($_SERVER['PATH_INFO']);
if (!preg_match('!^([A-Za-z0-9_]+)-(\d+)?x(\d+)?$!', $path['filename'], $matches)) {
    header('HTTP/1.0 404 Not Found');
    return;
}
$orig_image = sprintf('%s%s/%s.%s',
                      $IMAGES_DIRECTORY, $path['dirname'],
                      $matches[1], $path['extension']);
$width  = $matches[2];
$height = $matches[3];
if (!file_exists($orig_image)) {
    header('HTTP/1.0 404 Not Found');
    return;
}

# That's it for the URL parsing code!  Let's get on with the real work!
serve_image($orig_image, $width, $height);

Code Snippets

// Locate the position of the last hyphen in the filename.
$index = strpos( $filename, "-" );
# You probably want to analyze PATH_INFO rather than QUERY_STRING
$path = pathinfo($_SERVER['PATH_INFO']);
if (!preg_match('!^([A-Za-z0-9_]+)-(\d+)?x(\d+)?$!', $path['filename'], $matches)) {
    header('HTTP/1.0 404 Not Found');
    return;
}
$orig_image = sprintf('%s%s/%s.%s',
                      $IMAGES_DIRECTORY, $path['dirname'],
                      $matches[1], $path['extension']);
$width  = $matches[2];
$height = $matches[3];
if (!file_exists($orig_image)) {
    header('HTTP/1.0 404 Not Found');
    return;
}

# That's it for the URL parsing code!  Let's get on with the real work!
serve_image($orig_image, $width, $height);

Context

StackExchange Code Review Q#44025, answer score: 2

Revisions (0)

No revisions yet.