For background, see http://drupal.org/drupal-7.20-release-notes.

Because Drupal 7.20 fixed a security issue that was related to fundamental functionality in the Drupal core Image module, it broke a fair number of contributed modules and custom code/workflows. Because of that, many sites which have tried to upgrade to Drupal 7.20 have been unable to, or have upgraded but have set the 'image_allow_insecure_derivatives' variable to TRUE. In either case, these sites are completely unprotected from the denial-of-service issues that Drupal 7.20 was designed to fix.

This patch is designed to provide some level of protection even to sites that use 'image_allow_insecure_derivatives'. They will still be vulnerable to some kinds of denial-of-service attacks using image styles. However, the most damaging and easiest-to-inflict attacks (involving the generation of derivatives-of-derivatives, as well as the unauthorized creation of empty image style directories) will be protected against. Thus, although the variable is still considered insecure and is not recommended, it will allow sites using it to upgrade to Drupal 7.21 and at least have some security protection, while they wait for the other fixes they need to be able to stop using it altogether.

Some details of the above attacks have not been publicly disclosed yet (or have been partially disclosed elsewhere on the Internet), but the Drupal security team has cleared this for public discussion because it's not really fixing any new security issues; they were all already fixed in Drupal 7.20 for sites not using the bypass variable, and the bypass variable was already documented as insecure.

My plan for this patch:

  1. Commit it as soon as possible, unless someone finds something really wrong with it.
  2. Release Drupal 7.21 on top of Drupal 7.20 (containing only this fix). Hopefully tomorrow.
  3. Move the issue to Drupal 8 and forward-port it (as if it were a security release). Tests would be added at that point (and could then be backported to Drupal 7). Tests should likely involve checking that (a) even if 'image_allow_insecure_derivatives' is set to TRUE, it is still impossible to visit a URL which creates an image derivative based off an another image derivative (unless the URL contains the security token), and (b) even if 'image_allow_insecure_derivatives' is set to TRUE, it is impossible to visit a URL which creates an arbitrary empty directory in the site's image styles directory.

Follow-up could be #1934498: Allow the image style 'itok' token to be suppressed in image derivative URLs

Files: 
CommentFileSizeAuthor
#26 image-allow-insecure-derivatives-1934568-26.patch2.44 KBofry
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,449 pass(es).
[ View ]
#6 image-allow-insecure-derivatives-1934568-6-TESTS-ONLY.patch3.57 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 40,070 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
#6 image-allow-insecure-derivatives-1934568-6-INCOMPLETE-FIX.patch5.42 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 40,088 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#6 image-allow-insecure-derivatives-1934568-6.patch5.43 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,041 pass(es).
[ View ]
#4 image-allow-insecure-derivatives-1934568-4.patch3.94 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 39,875 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 image-allow-insecure-derivatives-1934568-2.patch1.88 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,902 pass(es).
[ View ]
#1 image-allow-insecure-derivatives-1934568-1.patch1.85 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-allow-insecure-derivatives-1934568-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-allow-insecure-derivatives-1934568-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And the patch.

StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 39,902 pass(es).
[ View ]

Reroll to prevent bypassing this with a URL containing extra slashes (e.g., http://example.com/sites/default/files/styles/medium/public///////////////styles/thumbnail/public/field/image/example.png).

I still have some concerns about this patch - can you explain why this is protecting fully?

strpos(file_uri_target($scheme . '://' . $target), 'styles/') === 0

Also, why are we calling file_uri_target() if we already have the target? That's just to trim slashes?

I'd also think at least a simple test case should be included? Also, by repurposing the variable this way, it makes it impossible for anyone who really wants to make combined derivatives by hand (crazy as that amy be)?

StatusFileSize
new3.94 KB
FAILED: [[SimpleTest]]: [MySQL] 39,875 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This adds some tests of the variable behavior. Taking away the patch on image.module gives 2 fails.

As above, I'm not sure this will be good enough in the end, since this means it's impossible to create a combination derivative.

Status:Needs review» Needs work

The last submitted patch, image-allow-insecure-derivatives-1934568-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.43 KB
PASSED: [[SimpleTest]]: [MySQL] 40,041 pass(es).
[ View ]
new5.42 KB
FAILED: [[SimpleTest]]: [MySQL] 40,088 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new3.57 KB
FAILED: [[SimpleTest]]: [MySQL] 40,070 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

Thanks!

I think the unrelated test failure comes from reassigning the trimmed value back to $target. (That makes sense in theory, but it's going to have side-consequences not related to this issue, so I removed it in the attached patch.) In the earlier patch I did use file_uri_target() to accomplish the trimming (so as to wind up with the same code as image_file_download()) but ltrim() is probably good enough, so sure, let's just do that.

As for tests, I agree, after discovering the issue with extra slashes, getting tests in now is a good idea :) The attached makes some additions to the tests so they run for private files too, test the forward/backward slash issue, and also test the creation of empty directories. All seem to pass/fail as expected for me locally, but let's let the testbot try them out.

Repurposing the variable is because many sites have used it already (and are therefore unprotected) and we should make them safer. In theory, we could introduce a second variable ('image_allow_insecure_recursive_derivatives'?) which would reproduce the behavior of the original one. It wouldn't be hard to add, but adding it means it's something we have to support going forward. I am not convinced there is anyone out there building combined derivatives in the first place, let alone building them by hand (!). If I'm wrong, they can stay on Drupal 7.20 for the time being (the patch in this issue would not be able to help them compared to Drupal 7.20 anyway, right?) and file a bug report. But I really don't think it will happen...

Since I'm going to link to this from a couple other issues, here are instructions for testing this patch manually (assuming you experienced problems with Drupal 7.20):

  1. Apply the patch.
  2. Add $conf['image_allow_insecure_derivatives'] = TRUE; to your settings.php file.
  3. Confirm that your problem is gone.

A brief overview: For sites not using the 'image_allow_insecure_derivatives' variable, this patch shouldn't change anything (tokens will still be required for all image derivatives). For sites using the variable, tokens will be optional for most image derivatives (as they were before), but required in the unlikely case of derivatives of derivatives (for example, if you first generate a thumbnail image by visiting http://example.com/sites/default/files/styles/thumbnail/public/field/image/example.png, and then want to take that thumbnail image and generate a "medium" image based off of it by visiting http://example.com/sites/default/files/styles/medium/public/styles/thumbnail/public/field/image/example.png, the second case will require a token in the URL in order to work).

Tested #6 and it works as expected on Ubuntu 12.10 with the default LAMP stack. Functions like ltrim, strpos have the exact behaviors in all PHP 5 version, so I think it's good to go. Unless someone else points out a way to bypass the "styles/" check.

I'll let a more experienced developer set the RTBC status anyway.

Great, thanks.

Unless someone else points out a way to bypass the "styles/" check.

Since this code uses the same logic as image_file_download() for determining if a file is an image derivative, if anyone does find a way to bypass this, there's a good chance you've discovered an existing security hole in Drupal too. So, perhaps better to report it to the Drupal Security Team rather than here.

I have done some testing of this myself, though, and haven't come up with anything except the forward slashes and backslashes which the ltrim() call protects against.

This looks great, but as an alternative suggestion would a simple rate limiting mechanism be sufficient to help stem a ddos attack when image_allow_insecure_derivatives is TRUE?

I get that issue is specific to fixing recursion, but the original issue here was to offer partial protection. Combine disabling recursion along with a rate limit and you'd potentially have an effective combo to provide that. Just wondering if that option was ever discussed as an option.

Yes it's what is discussed at #1934498: Allow the image style 'itok' token to be suppressed in image derivative URLs. This issue is meant for a quick commit for 7.21 because many many Drupal sites are in danger!

Right, a rate limit could definitely be discussed on top of this, but it's a fair amount more complicated (and protects against a lower-risk attack) so I think it's better to keep it in a separate issue.

Status:Needs review» Reviewed & tested by the community

I think with the added tests this is rtbc

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

This is committed to 7.x and will be released as Drupal 7.21 (coming out shortly): http://drupalcode.org/project/drupal.git/commit/33d91c4

Thanks everyone for your help!

Moving to Drupal 8 to forward-port the patch. (To be honest, there's a question to be asked about whether Drupal 8 should even continue supporting this variable at all going forward. However, the tests need to be ported either way.)

Just in case it helps anyone, I had the same issues - images not appearing after core/module updates (though only ones inserted via an image field - site graphics and images inserted in the main body field were there). Eventually noticed that something during the update process had overwritten the setting for "Public file system path" in configuration>>media>>file system. Restoring the correct path fixed it for us. What the heck...

Is the forward port for 8.x critical?

I think. D7.21 is a security release. We can't ship D8 without this port.

There's a similar patch in #1934498-51: Allow the image style 'itok' token to be suppressed in image derivative URLs for Drupal 8, so can we move the discussion to that issue?

(subscribe)

Status:Needs work» Needs review
Issue tags:-Security improvements

Issue summary:View changes

add refs

Issue tags:+Needs reroll

image-allow-insecure-derivatives-1934568-6.patch no longer applies.

Status:Needs review» Needs work

Status:Needs work» Needs review

StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,449 pass(es).
[ View ]

Trying to reroll patch, but can't reroll test code :(