Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
4.35 KB

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.

jcisio’s picture

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.

scor’s picture

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

scor’s picture

+++ 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?

pwolanin’s picture

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.

FiNeX’s picture

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

greggles’s picture

Re #6 - I'm not aware of any potential SEO penalties from this. However, even if there are penalties, that should be pushed to a followup issue.

FiNeX’s picture

@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?

scor’s picture

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.

Dave Reid’s picture

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?

greggles’s picture

scor’s picture

@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.

Dave Reid’s picture

Status: Needs review » Needs work

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));
jcisio’s picture

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.

scor’s picture

Status: Needs work » Needs review
FileSize
981 bytes
4.14 KB

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

scor’s picture

oops, forgot to remove an hmac comment.

pwolanin’s picture

Status: Needs review » Needs work

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.

scor’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
pwolanin’s picture

Status: Needs review » Needs work

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)))));
scor’s picture

Status: Needs work » Needs review
FileSize
1007 bytes
4.68 KB

Thanks Peter.

scor’s picture

used the wrong global.

pwolanin’s picture

do we need to serialize?

+  $salt = sha1(serialize($db_url));
scor’s picture

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

greggles’s picture

In pressflow6 the db_url can be an array - https://pressflow.atlassian.net/wiki/display/PF/Using+database+replicati...

So, I think the serialize is worth keeping.

Dave Reid’s picture

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

pwolanin’s picture

Status: Needs review » Needs work

this needs to return the binary value:

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

should be:

$key = pack('H*', sha1($key));
pwolanin’s picture

FileSize
1.45 KB

Here's a quick test file for the corrected function

pwolanin’s picture

Also I'd rename these vars:

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

like $ikey and $okey?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.86 KB

updated plus added code comments.

pwolanin’s picture

Seems tests are not enabled for this project?

We probably need to update those too.

fizk’s picture

Should be enabled now.

pwolanin’s picture

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..
fizk’s picture

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

fizk’s picture

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

chellman’s picture

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

fizk’s picture

Status: Needs review » Needs work

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

a_c_m’s picture

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) ?

fizk’s picture

@a_c_m Does the patch work for you?

nickfitz’s picture

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...

fizk’s picture

@nickfitz, thanks for following up!

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

jcisio’s picture

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.