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:
- Commit it as soon as possible, unless someone finds something really wrong with it.
- Release Drupal 7.21 on top of Drupal 7.20 (containing only this fix). Hopefully tomorrow.
- 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
Comment | File | Size | Author |
---|---|---|---|
#50 | allow_sites_using_the-1934568-50.patch | 5.6 KB | StryKaizer |
#50 | interdiff.txt | 3.67 KB | StryKaizer |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedAnd the patch.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedReroll 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
).Comment #3
pwolanin CreditAttribution: pwolanin commentedI still have some concerns about this patch - can you explain why this is protecting fully?
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)?
Comment #4
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedThanks!
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...
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedSince 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):
$conf['image_allow_insecure_derivatives'] = TRUE;
to your settings.php file.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 visitinghttp://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).Comment #8
jcisio CreditAttribution: jcisio commentedTested #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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedGreat, thanks.
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.
Comment #10
timaholt CreditAttribution: timaholt commentedThis 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?
Comment #11
jcisio CreditAttribution: jcisio commented@10 no, #1934482: Add an option to disable recursive imagecache preset path
Comment #12
timaholt CreditAttribution: timaholt commentedI 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.
Comment #13
jcisio CreditAttribution: jcisio commentedYes 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!
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedRight, 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.
Comment #15
pwolanin CreditAttribution: pwolanin commentedI think with the added tests this is rtbc
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.)
Comment #17
rogernyc CreditAttribution: rogernyc commentedJust 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...
Comment #18
star-szrIs the forward port for 8.x critical?
Comment #19
jcisio CreditAttribution: jcisio commentedI think. D7.21 is a security release. We can't ship D8 without this port.
Comment #20
attiks CreditAttribution: attiks commentedThere'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?
Comment #21
kifuzzy CreditAttribution: kifuzzy commented(subscribe)
Comment #22
rekano CreditAttribution: rekano commented#1: image-allow-insecure-derivatives-1934568-1.patch queued for re-testing.
Comment #22.0
rekano CreditAttribution: rekano commentedadd refs
Comment #23
martin107 CreditAttribution: martin107 commentedimage-allow-insecure-derivatives-1934568-6.patch no longer applies.
Comment #24
martin107 CreditAttribution: martin107 commentedComment #25
ofry CreditAttribution: ofry commentedComment #26
ofry CreditAttribution: ofry commentedTrying to reroll patch, but can't reroll test code :(
Comment #27
catchIt 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.
Comment #30
amitgoyal CreditAttribution: amitgoyal commentedReroll of #26.
Comment #32
martin107 CreditAttribution: martin107 commentedFixed one small aspect
Comment #33
jhedstromThe patch in #32 makes sense, but I'm worried if we commit without tests (as recommended in the issue summary), tests will never come.
Comment #34
DuaelFrTwo minor comments on that patch + I think we definitely need tests (two years later we are not anymore in a hurry).
This change leads to the duplication of the "site's vulnerability" term.
We do not need to escape the "/" character in
litrm()
.Comment #35
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe 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.
Comment #36
StryKaizerAttached you'll find the test ported from D7 (commit: http://cgit.drupalcode.org/drupal/commit/?id=33d91c4) and the test + patch from #32
Comment #37
StryKaizerComment #39
DuaelFr@StryKaizer thank you very much for that.
My #34 comment is still pertinent, though :)
Comment #40
borisson_#34
ImageStylesPathAndUrlTest
fails when changing that tostrpos(ltrim($target, '/'), 'styles/')
Setting to RTBC as I only changed one line in the docs.
Comment #41
DuaelFrThat test failure you're talking about is really strange but, anyway, it works as-si.
Thank you for the final fix :)
Comment #42
pwolanin CreditAttribution: pwolanin at Acquia commentedThis could possibly use some more explanation in comments about why the 'styles/' check protects against making derivatives of derivatives.
Comment #43
StryKaizerAbout this:
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.
Comment #44
borisson_The patch in #43 reverted the changes I did in #40.
Comment #45
stefan.r CreditAttribution: stefan.r commentedI 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:
Maybe "Stop supressing the security token in the URL"?
Instead of these can we assert that the URL has the expected format?
Let's test this with multiple slashes as well?
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.
Wouldn't this be redundant as we have the drupalGet() already?
Just for clarification, can we move this up, right under where the $nested_url is generated?
Not sure if this might generate unexpected characters so should we use randomMachineName here instead?
Comment #46
stefan.r CreditAttribution: stefan.r commentedUpdating 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.
Comment #47
stefan.r CreditAttribution: stefan.r commentedComment #48
DuaelFrAs 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
Comment #49
stefan.r CreditAttribution: stefan.r commentedIMO 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.
Comment #50
StryKaizerNew patch with remarks from #45 attached.
Comment #51
stefan.r CreditAttribution: stefan.r commentedLooks great now!
Comment #52
stefan.r CreditAttribution: stefan.r commentedComment #53
alexpott@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.
Comment #55
JeroenTRandom test failure: [PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full
Back to RTBC.
Comment #57
alexpottCommitted b37e84f and pushed to 8.0.x. Thanks!