Problem/Motivation

When the 'image_allow_insecure_derivatives' config setting is enabled, sites are still vulnerable to DoS attacks when a derivative of a style is requested.

This issue has been fixed in D7 and tests have been added but this has not happened in D8 yet, which means sites with this setting enabled are a lot less secure than in D7.

Proposed resolution

Disallow requesting derivatives of image styles, even with the 'image_allow_insecure_derivatives' config setting enabled. Drupal 7 tests were added in http://drupalcode.org/project/drupal.git/commit/33d91c4, forward-port those to Drupal 8.

Alternatively, consider removing the feature.

Remaining tasks

Review patch

User interface changes

No

API changes

Just like in D7 we always require a token when a derivative of an image style is requested

Data model changes

N/A

Original issue:

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Needs review
FileSize
1.85 KB

And the patch.

David_Rothstein’s picture

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

pwolanin’s picture

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

pwolanin’s picture

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.

David_Rothstein’s picture

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

David_Rothstein’s picture

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

jcisio’s picture

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.

David_Rothstein’s picture

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.

timaholt’s picture

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?

jcisio’s picture

timaholt’s picture

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.

jcisio’s picture

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!

David_Rothstein’s picture

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.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I think with the added tests this is rtbc

David_Rothstein’s picture

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

rogernyc’s picture

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

star-szr’s picture

Is the forward port for 8.x critical?

jcisio’s picture

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

attiks’s picture

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?

kifuzzy’s picture

(subscribe)

rekano’s picture

Status: Needs work » Needs review
Issue tags: -Security improvements
rekano’s picture

Issue summary: View changes

add refs

martin107’s picture

Issue tags: +Needs reroll

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

martin107’s picture

Status: Needs review » Needs work
ofry’s picture

Status: Needs work » Needs review
ofry’s picture

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

catch’s picture

Priority: Critical » Major

It made sense for this to be critical the day after the SA went out, but not a year and a month later. 8.x modules can deal with the API change and likely lots of 7.x modules have by now.

Downgrading to major.

Status: Needs review » Needs work

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

amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.34 KB

Reroll of #26.

Status: Needs review » Needs work

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

martin107’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
571 bytes

Fixed one small aspect

     // If the source file doesn't exist, return FALSE without creating folders.
     $image = \Drupal::service('image.factory')->get($original_uri);
-    if (!$image->isExisting()) {
+    if (!$image->isValid()) {
       return FALSE;
     }
 
jhedstrom’s picture

The patch in #32 makes sense, but I'm worried if we commit without tests (as recommended in the issue summary), tests will never come.

DuaelFr’s picture

Status: Needs review » Needs work
Issue tags: +HappyDays1506, +Needs tests

Two minor comments on that patch + I think we definitely need tests (two years later we are not anymore in a hurry).

  1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
    @@ -102,9 +102,11 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
         // bypass the latter check, but this will increase the site's vulnerability
    -    // to denial-of-service attacks.
    +    // site's vulnerability to denial-of-service attacks. To prevent this
    +    // variable from leaving the site vulnerable to the most serious attacks, a
    +    // token is always required when a derivative of a derivative is requested.
    

    This change leads to the duplication of the "site's vulnerability" term.

  2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
    @@ -102,9 +102,11 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
    +    if (!$this->config('image.settings')->get('allow_insecure_derivatives') || strpos(ltrim($target, '\/'), 'styles/') === 0) {
    

    We do not need to escape the "/" character in litrm().

David_Rothstein’s picture

The patch in #32 makes sense, but I'm worried if we commit without tests (as recommended in the issue summary), tests will never come.

The issue summary is actually very out of date. We only considered committing this without tests in order to get a quick fix out for Drupal 7 since it was a very serious issue. But Drupal 7 tests were added a long time ago (see the commit in #16) so we just need to forward-port those to Drupal 8.

I'm also not sure why this wouldn't be critical for Drupal 8 like it was for Drupal 7. (In theory the feature could be considered for removal for Drupal 8, making this issue obsolete... but if it hasn't yet it isn't likely to). And as long as it exists I think it should not be way less secure than the Drupal 7 version. But leaving this at "major" for now.

StryKaizer’s picture

Attached you'll find the test ported from D7 (commit: http://cgit.drupalcode.org/drupal/commit/?id=33d91c4) and the test + patch from #32

StryKaizer’s picture

Issue tags: -Needs tests

The last submitted patch, 36: test_only-allow_sites_using_the-1934568-36.patch, failed testing.

DuaelFr’s picture

@StryKaizer thank you very much for that.
My #34 comment is still pertinent, though :)

borisson_’s picture

DuaelFr’s picture

That test failure you're talking about is really strange but, anyway, it works as-si.
Thank you for the final fix :)

pwolanin’s picture

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -102,9 +102,11 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+    if (!$this->config('image.settings')->get('allow_insecure_derivatives') || strpos(ltrim($target, '\/'), 'styles/') === 0) {

This could possibly use some more explanation in comments about why the 'styles/' check protects against making derivatives of derivatives.

StryKaizer’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.36 KB

About this:

strpos(ltrim($target, '\/'), 'styles/') 

I couldnt reproduce an url abusing this issue, which uses a forward slash. (the backwardslash url ofcourse is an issue for sure).
I asked David_Rothstein and ircmaxell about the forward slash which is trimmed from the url.
Basicly, I'm still not sure why the forward slash is there, being trimmed. There might be a webserver/os/browser which allows forward slashes. Then again, as ircmaxell said, better be safe then sorry, so I suggest leaving it as is unless somebody can ensure the forward slash is a mistake, which got committed to D7 anyway.

Attached you'll find a patch which has some more explanation as suggested in #42

#41: the failure is only there because of the test being wrong if the forward slash is not required for security reasons. The assertion on line 244 should be set to 404 instead of 403, or even better, be removed as in that case it makes no sense to test that url.

borisson_’s picture

The patch in #43 reverted the changes I did in #40.

stefan.r’s picture

I have compared this with the D7 commit at http://cgit.drupalcode.org/drupal/commit/?id=33d91c4 and overall this looks great.

I wonder if we should consider deprecating and removing the suppress_itok_output and allow_insecure_derivatives settings though, for D7 those may have made sense but not sure about D8?

Just a few nits:

  1. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    // Enable the security token again
    

    Maybe "Stop supressing the security token in the URL"?

  2. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    $this->pass($nested_url);
    +    $this->verbose($nested_url);
    

    Instead of these can we assert that the URL has the expected format?

  3. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    $this->drupalGet(substr_replace($nested_url_with_wrong_token, '//styles/', strrpos($nested_url_with_wrong_token, '/styles/'), strlen('/styles/')));
    

    Let's test this with multiple slashes as well?

  4. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    $this->drupalGet(substr_replace($nested_url_with_wrong_token, '/\styles/', strrpos($nested_url_with_wrong_token, '/styles/'), strlen('/styles/')));
    

    I don't think the backslash is testable on our testbot, it's not in the D7 patch either so we may be able to leave it out.

  5. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    $this->verbose(substr_replace($nested_url_with_wrong_token, '/\styles/', strrpos($nested_url_with_wrong_token, '/styles/'), strlen('/styles/')));
    

    Wouldn't this be redundant as we have the drupalGet() already?

  6. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    // Make sure the image can still be generated if a correct token is used.
    +    $this->drupalGet($nested_url);
    +    $this->assertResponse(200, 'Image was accessible when a correct token was provided in the URL.');
    

    Just for clarification, can we move this up, right under where the $nested_url is generated?

  7. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
    @@ -224,6 +224,33 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
    +    $directory = $scheme . '://styles/' .  $this->style->id() . '/' . $scheme . '/' . $this->randomString();
    +    $this->drupalGet(file_create_url($directory . '/' . $this->randomString()));
    

    Not sure if this might generate unexpected characters so should we use randomMachineName here instead?

stefan.r’s picture

Issue summary: View changes

Updating the IS, and just reiterating the comment in #35 that this same issue for D7 was considered critical so arguably this one should be as well, as D8 sites with this setting enabled are still vulnerable to the derivative attack and so they are currently less secure.

stefan.r’s picture

Issue tags: +DUGBE0609
DuaelFr’s picture

As far I remember, take setting was introduced to avoid breaking sites that relied on old contrib or custom modules. These websites are not likely to exist on D8 so I agree with @stefan.r about removing it in 8.0.x

stefan.r’s picture

IMO we can RTBC this as soon as the nits are addressed as currently the feature still exists and is less secure than in D7. That way a core committer can have a look at this and decide whether to remove the feature instead.

StryKaizer’s picture

New patch with remarks from #45 attached.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now!

stefan.r’s picture

Category: Task » Bug report
alexpott’s picture

@stefan.r raises a good point - I don't think these settings have any place in D8 - I've opened #2568517: Deprecate allow_insecure_derivatives and suppress_itok_output in image system to discuss this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: allow_sites_using_the-1934568-50.patch, failed testing.

JeroenT’s picture

Status: Needs work » Reviewed & tested by the community

Random test failure: [PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b37e84f and pushed to 8.0.x. Thanks!

  • alexpott committed b37e84f on 8.0.x
    Issue #1934568 by David_Rothstein, StryKaizer, borisson_, martin107,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.