Protection against DOS SA-CORE-2013-002 - Drupal core - Denial of service

Here's an initial backport.

For some reason i'm not getting image derivates generated, but that's true without the patch too, so I think it's an image toolkit issue.

Could we have a link to the issue that led to that change in Drupal core? I think the next step could be remove the token with some limited threshold in the number of concurrent image derivates generation. For example, flush an image preset (this function exists in Imagecache, but not in the D7 version of Drupal core) cause a similar DoS attack if there is no reverse proxy in front of Drupal.

same patch, only corrected a typo in the comment of the last hunk. This patch works on my localhost, views can generate thumbnail images.

+++ b/imagecache.module
@@ -435,9 +474,16 @@ function imagecache_cache_private() {
+  if (!$valid) {
+    // Send a 403 if the presented token isn't invalid.
+    header('HTTP/1.0 403 Forbidden');
     exit;

any reason why not doing

return drupal_access_denied();

here?

That does a much more expensive operation, including potentially writing to the DB via watchdog and theming a nice 403 page.

http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_a...

Given that the original module had this simple shortcut, I didn't see a reason to change.

Did someone analyzed the proposed solution from a SEO perspective? Thanks

@greggles: I'm thinking about a duplication/crawling problem of the image URL by Google.

Google would crawl two times the same URL and this is a waste of resources. Even if the crawler should be smart and group the content I don't like the idea. Anyway to it should be enough to exclude "itok" from the URL parameters on Webmaster Tools to be safe and let the crawler to get only the base URL.

What do you think about?

I don't think image SEO is as important as regular web pages URLs SEO. This might have an impact when searching for images on Google but I doubt it will affect SEO of regular web pages. these urls won't change. I'm pretty sure Google is smart enough to adjust if your images' urls change, especially if the change is a query parameter. I think having the same image in multiple derivative is possibly a larger concern, which is what imagecache does by design.

What is the version of the URL with itok did it's generation, and then redirected to the version of the image URL without the itok?

@Dave Reid wouldn't that lead to a redirect everytime you generate imagecache URLs via e.g. views. even when the image is already available?

Either way, this is good feedback, and the type of follow up that could be done in a separate issue.

Ignore #10.

A bigger problem may be this code assumes the hash PHP extension is enabled and available, but it's not a requirement for Drupal 6. This will cause fatal errors for any sites that do not have the extension enabled. It's only enabled by default as of PHP 5.1.2, which is not the minimum requirement for ImageCache nor core. The minimum PHP requirement for imageapi is 5.1.

+  $salt = hash('sha256', serialize($databases));

+  $hmac = base64_encode(hash_hmac('sha256', $data, $key, TRUE));

5.1.6 was released more than 6 years ago and has not been updated since then. According to http://w3techs.com/technologies/details/pl-php/5.1/all PHP < 5.1.6 has less than 0.15% market share of PHP. I don't think there are maintained D6 websites with PHP 5.1.

That said, simply replace hash/hash_hmac with md5 works if hash is not available.

Heine and Peter suggested to use sha1 to address #13. This patch also uses $db_url which is the D6 equivalent to $databases.

oops, forgot to remove an hmac comment.

change to sha1:

+  $salt = hash('sha256', serialize($db_url));

This port is wrong I think - you need to implement the HMAC using sha1 for imagecache_style_path_token(). Just calling sha1() is not enough.

ok, patch using HMAC-SHA1 now, inspired from http://drupalcode.org/project/acquia_search.git/blob/refs/heads/6.x-1.x:...

Still has this call to hash():

+  $salt = hash('sha256', serialize($db_url));

and this is wrong:

+  $hmac = base64_encode(sha1($opad . pack("H*", sha1($ipad . $data))));

it is missing a call to pack('H*'). Correct is:

$hmac = base64_encode(pack("H*", (sha1($opad . pack("H*", sha1($ipad . $data)))));

Thanks Peter.

used the wrong global.

do we need to serialize?

+  $salt = sha1(serialize($db_url));

we technically don't but I left it like that to remain closer to the D7 code logic.

It can be an array just with using Drupal 6 core, so agreed, serialize should stay.

this needs to return the binary value:

+  if (strlen($key) > 64) {
+    $key = sha1($key);
+  }

should be:

$key = pack('H*', sha1($key));

Here's a quick test file for the corrected function

Also I'd rename these vars:

  $ipad = $ipad ^ $key;
  $opad = $opad ^ $key;

like $ikey and $okey?

updated plus added code comments.

Seems tests are not enabled for this project?

We probably need to update those too.

Should be enabled now.

On the testing tab looking at the results it says:

FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: run-tests.sh reported no tests were found. See review log for details..

I renamed get_info() to getInfo() due to #237667: get_info to getInfo. Let's try this again.

Turns out there were a lot more things that needed updating. Should work now.

Just chiming in to say that #34 is working for me.

Now that the automated testing is working, some tests should be written. Could the D7/D8 tests be backported?

Given exploit code is now live : http://berk.es/2013/03/04/drupal-imagecache-security-vulnarability-with-...

Is there any further movement on this or advice for D6 users. Is 34 RTBC (minus tests) ?

@a_c_m Does the patch work for you?

Hey @Fizk:

I have tested and implemented this patch in a production environment on a large scale Drupal site. The patch works as intended and provides the additional security measures we needed to close the potential attack vector. Thank you for all of your work.

We ran into some hiccups with our own implementation of of the patch in instances where we were re-sizing already re-sized images that were made available to a node via node_embed and displayed within a view. These images were returning as invalid and displaying the 403 error. For others applying the patch in a similar environment, here are the issues we ran into and our solution:

Problem:

The original source image has been re-sized and is made available to a node via node_embed. The re-sized image will need to be re-sized again prior to being displayed within a view. After applying the patch, the source image path, which has already been re-sized, contains a token key. Because the source URL already contains a token key, Drupal's URL encode would return "?" and "=" as "%3F" and "%3D" respectively, causing the patch's validation to fail. This resulted in the image request returning a 403 error.

Original Source Path: http://www.example.com/sites/default/files/imagecache/imagepresetA/image...
Original Re-sized Path: http://www.example.com/sites/default/files/imagecache/imagepresetB/sites...

Solution:

We resolved this issue by removing the original key from the source image URL. Below is some example code:

// Remove the base URL from the returned file path.
$source_img = str_replace("http://" . $base_url  . "/", "", $source_img);

// Define everything prior to the original imagecache key.
$before_key = explode('?', $source_img);
$source_url = $before_key[0];

// Theme the newly re-sized image.
$resized_image = theme('imagecache', $preset, $source_url, $title, $title);

Adjusted Source Path: http://www.example.com/sites/default/files/imagecache/imagepresetA/image...
Adjusted Re-sized Path: http://www.example.com/sites/default/files/imagecache/imagepresetB/sites...

@nickfitz, thanks for following up!

@pwolanin, can you please look into nickfitz's report.

With what that is being discussed at #1934498: Allow the image style 'itok' token to be suppressed in image derivative URLs, why don't we try an easier solution? This issue is here for more than a month.

A 4-line patch #1934482: Add an option to disable recursive imagecache preset path.