Problem/Motivation
The itok token introduced in 7.20 prevents working with CDNs, third party integrations that generate image presets on the fly. To understand the Image style token, itok, read Drupal 7.20 release notes at http://drupal.org/drupal-7.20-release-notes.
Proposed resolution
Allow the drupal user/administrative user of the site an option to decide if they need itok or not. The patch provided at #51 adds a new setting 'suppress_itok_output' The argument for making itok optional is that DDoS or DoS should be better solved at the infrastructure or server level. More over this attack is also possible views with a pager or exposed filters.
Remaining tasks
Needs review by Senior Core contributor
User interface changes
Not applicable
API changes
Related Issues
#1934482: Add an option to disable recursive imagecache preset path
Original report by [jcisio]
Linked issue #1934482: Add an option to disable recursive imagecache preset path because even the fix was published two weeks ago, I can't see any discussion on that issue.
The itok token introduced in 7.20 prevents many sites from upgrading and causes many problem. Why not eliminate it and replace with two things:
- A no recursive option: I think it is much better than the 'image_allow_insecure_derivatives' variable because 1/ we care security 2/ no reason to have urls like example.com/sites/default/files/styles/thumbnail/public/styles/thumbnail/public/image.jpg
- A threshold to limit the concurrent image derivate generation request.
The drawback is you can have image derivates generated by hacker that you'll never use. But given that they are limited, who cares?
Comment | File | Size | Author |
---|---|---|---|
#119 | i1934498-119.patch | 2.84 KB | attiks |
#110 | image.module-drupal-7.27.patch | 1.56 KB | KhaledBlah |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedFor a "no recursive" option, see #1934568: Allow sites using the 'image_allow_insecure_derivatives' variable to have partial protection from the Drupal 7.20 security issue which is related.
As for a threshold, there's some discussion in #1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 7.20 if image styles have been re-saved of using that as a backup method (i.e., still have tokens, but for requests that don't have a token allow a certain number up to the threshold). The problem with using it to replace tokens entirely is that you have to make the threshold large enough so that no one will hit it legitimately, but small enough to effectively protect against a denial-of-service attack. Especially after an image style is flushed, there will be many new requests for image derivatives afterwards (which are legitimate and should not be denied), so it might be hard to choose the right threshold.
Comment #2
jcisio CreditAttribution: jcisio commentedI think the linked issue #1934568: Allow sites using the 'image_allow_insecure_derivatives' variable to have partial protection from the Drupal 7.20 security issue addresses 100% what "no recursive" want to do. About the threshold, I think we don't really care, when I want to flusha in image style, I'd set it to 0 (unlimited), otherwise something like 10 is more than enough, because you get an 403 when the threshold is over, but after an F5 you'll get the image generated (if load decreases). It's similar a to poorman cron: 0,01% of the audience may have a bad experience.
Comment #3
jcisio CreditAttribution: jcisio commentedComment #4
KhaledBlah CreditAttribution: KhaledBlah commentedI need to be able completely prevent the "itok=..." token from any image style.
We use drupal 7 in our internal network and "export" a static HTML version of that site on our public website. For us the itok token does not do any good but it only does harm. Even with the "image_allow_insecure_derivatives" variable set to TRUE the image styles STILL have that token.
Maybe I misunderstand the purpose of that variable but I'd like to completely disable the itok token without hacking core (right now I have to edit image_style_url() which would usually be a big NONO but without our site doesn't work anymore). Is there a way to completely disable the itok token? If not, is there one in the works?
Comment #5
catchMoving to 8.x and bumping to critical - this affects 7.x-8.x upgrade as well as 7.x-7.x.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commented@KhaledBlah, can you explain how merely having the token in the URL causes problems on your site?
@jcisio, when you say you'd switch the threshold, you're envisioning a user interface for this or something that would switch automatically based on certain conditions?
So in general (and pending the question above) I don't see any reason for getting rid of the tokens. When a token is present, it's guaranteed to be secure and guaranteed to be reliable, so why not use it? The only issue we have is that some image links won't have a token and what to do about them. So to me it would make sense to merge this discussion into #1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 7.20 if image styles have been re-saved... unless someone can articulate a good reason to kill the tokens after all.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedFor that reason I'm not sure this should be critical either (we now have two critical issues open for a similar thing) but leaving it that way for now while this is discussed.
Comment #8
catchHmm I may be getting the issues confused, definitely removing the tokens is not critical, some better fallback behaviour I think is.
Comment #9
jcisio CreditAttribution: jcisio commented#6 I haven't seen any website with token in image urls (if not for caching). They might want to add token for access control in every request, but adding a token for an image that is present in 99.99% the case is simply ugly.
About the threshold, I'm open. It could be something such as: x10 when an image style is flushed, then returns back one hour after. Users can customize it (like they can customize the cron period). But I thought again, it's not perfect. If an attacker continously send requests until all possible image derivates are generated, those for new images are not generated, and it could also consider as a DoS attack (in a small amount of time, if you have a powerful server and a lot of disk space).
Another solution could be look at the page cache, if possible, to verify if an image style request is "authoritave". Usually, when a request is made, it should be mentionned in a page cache less than a few seconds before that. The problem is that we can not always search in page cache if it is not in database. However should there be something like "cache for page generated in the last 3 seconds" stored in database? It might even be more effective than token (compare between 50 itok generations and 1 db_insert every page request).
Comment #10
KhaledBlah CreditAttribution: KhaledBlah commented@David_Rothstein, we use wget to create a "static" copy of our drupal 7 website and we have created a JS based feature which does a blend-in/blend-out of certain images. These images are created using image styles. Now, wget downloads these images as static files and attaches the itok token. The JS code, however, assumes the image files to be available using the image style and their filename. Thus, the itok token breaks this feature. I could rewrite the feature, sure, but I'd rather see an option to disable the itok on user's will. After all not everyone is affected by the underlying security issue.
Plus, the way I understand the HTTP RFC GET queries with a ? are not meant to be used in this fashion. I might be wrong and this a minor concern but it still bugs me just the "cachebuster feature" does.
Comment #11
attiks CreditAttribution: attiks commented@David_Rothstein I'm also in favor of removing the itok, I think we better only handle the recursive problem, and allow people to override it, preferably using a callback/hook so they can limit the number of levels, if that's too much work, maybe using a variable specifying how many levels are allowed (0 meaning no re-cursing).
We sometimes manipulate images using javascript, and - as @KhaledBlah said - it complicates our code, without giving us any benefits.
Last thing: the introduction of the itok breaks resp_img module, so the only 'solution' right now is to advise everybody to disable the itok checking.
Comment #12
attiks CreditAttribution: attiks commentedA patch against D7 to demonstrate my comment in #11, if this is an acceptable solution, I'm more than happy to roll this against D8 as well
Setting to D7 for testbot
Comment #13
attiks CreditAttribution: attiks commentedBack to 8
Comment #14
jcisio CreditAttribution: jcisio commentedThat's basically Drupal 7.21 without security patch in 7.20, so I don't think it will be accepted, as there is no new approach introduced here.
Comment #15
attiks CreditAttribution: attiks commentedIn the current D7 head it isn't possible to allow recursive styles without itok tokens, that's something that was possible before 7.20, so it sounds like a regression.
I'm getting more and more the feeling that we're trying to solve something that better can be solved on the server/infrastructure level (DDOS), if you don't anonymous users to generate image styles, you can use hook_menu_alter to specify the required permissions, and/or use a custom callback to allow anonymous users to create image styles for only a particular subset of image styles.
Comment #16
Dave ReidI'm also confused why we need the token at all and can't actually check for recursion and adding flood protection with a default reasonable limit. I would never expect to be able to create an image style derivative of an already generated image style image. I should have spoke up sooner when this was being discussed in the security team.
Comment #17
jcisio CreditAttribution: jcisio commentedAs we expect the non recursive usage of image style cover 99.99% use cases, could we simply add the token when there are more than one image style levels?
Other things, like a threshold, are also nice to have. The image derivative generation is sometimes the most expensive request (when there comes face detection, more advanced actions etc.).
Comment #18
attiks CreditAttribution: attiks commentedI think it safe to ship with the recursive option disabled, if people need it I assume they know what they are doing, and they can enable it.
Threshold can be nice, but it will complicate things, if it gets implemented please tie it to the IP address as well, like flood does now, the other problem will be to set a 'good' threshold value, we will probably get some new issues of people not seeing images (or being blocked altogether) after flushing all image styles ;-)
What else is needed to make a decision on this, the longer we wait, the more people will move to 7.20/7.21 and the more complex this will be?
Is it an option to solve this in D7 first and forward port it to D8?
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedI will post a message in the security team thread seeing if people who were involved in this want to comment here.
I agree there is a fine line between denial-of-service attacks that Drupal can protect against and those that it can't. However, keep in mind that this is about more than just denial-of-service; it's also about unauthorized image generation filling up a hard disk which can cause serious problems too.
Drupal core (before Drupal 7.20) allowed any site visitor armed with a little information (information which is not that hard to find) to generate arbitrary images on the server simply by making a request. Preventing recursive image generation fixes a significant part of that, but with all the top-level combinations of uploaded files and image styles there are still a huge number remaining. The token is what allows Drupal to verify that the image being requested is one that was intended to be generated.
#10 and #11: These seem to me like cases where you would have to fix the code. It was an API-breaking change, no question about that. But once the code is fixed there shouldn't be any issues having the token in the URL?
Comment #20
gregglesFlood protection tied to IP address is only effective as a solution against DOS, not DDOS. It also seems likely to get accidentally triggered if you flush your image style. For example, a site I work on has several pages with 50 image derivatives of the same style. If we flush that style then flood protection would likely freak out about 50 images getting regenerated when someone hits that page so people would get broken pages.
I don't expect Drupal to be able to prevent every kind of DOS/DDOS, of course, but I'm with David's comment in #19 that once a fix is in place I don't see how this is a problem on an ongoing basis.
Comment #21
pwolanin CreditAttribution: pwolanin commentedA justification for always emitting the query string was so that it was not trivial to fingerprint sites updated to 7.20+ that had enabled the bypass variable.
I am not wedded to that, but it seemed better to start with a more secure posture.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedOh wait, you're saying the JS code contains a regular expression or something that fails when a token is present? If so, yeah, the 'image_allow_insecure_derivatives' variable won't help with that. I think the rationale of always adding the token was that when you turn that variable off again you want any previous links (e.g. that were cached somewhere) to start working immediately. I guess an option could be added to turn that off even too. Although at this point, maybe it's simpler just to fix the code... then you get the security benefit too :)
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedAh, cross-posted with @pwolanin, but I guess that was a rationale too (fingerprinting).
Comment #24
jcisio CreditAttribution: jcisio commented#20 greggles: it is suggested that we could implement a dynamic threshold. Also, those unavailable images are very temporary and should go away after one or two page refresh. Also, for website with heavy media/traffic, there is usually a caching reverse proxy that reduce this kind of problem.
Problem:
- People do not like the image style token
- We need to prevent unauthorized image derivative generation
Solution:
- Store in database a pagecache-like of recently (less than a few seconds ago) generated pages to see if a request is legitimate.
Comment #25
jcisio CreditAttribution: jcisio commentedThis issue is only major. But once it is solved, one critical issue #1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 7.20 if image styles have been re-saved is automatically won't fix.
Comment #26
attiks CreditAttribution: attiks commentedSome remarks
#19
"about unauthorized image generation"
If you want authorization you can use hook_menu_alter to add an access callback.
"Preventing recursive image generation fixes a significant part of that, but with all the top-level combinations of uploaded files and image styles there are still a huge number remaining" I assume 'huge' is pretty relative, for the average site this is like a couple of thousand images, most site will not have more than a couple hundred of images.
#20
"Flood protection tied to IP address is only effective as a solution against DOS, not DDOS" You're right, so maybe we need both, but this is also true for any site using views with a pager or exposed filters, so this is not only related to images.
"flood protection would likely freak out about 50 images getting regenerated" Unless the threshold is set to 60, this is about what value is a good default, but if people can change this on a site per site basis, that shouldn't be a problem.
#22
"maybe it's simpler just to fix the code" we have drupal site that's used to drive the content on in store displays (120 stores), the store software is written by a third party and they get the images in json (filename only) and they construct the URI on the fly using 5 image styles, changing this will be pretty costly and it will be hard to explain to the client. In this case the site will not update to D7.20, it's running on a private network, but I can see similar problems with apps connecting to a Drupal 7 site in the wild, that would rather update their site to get other security fixes.
Reading through #19 - #23 it looks like this (itok) will stay?
Comment #27
jcisio CreditAttribution: jcisio commentedUnless I'm wrong, the #19 - #23 means that the introduction of itok was reasonable from a security point of view. If we could suggest something similar and less intrusive, then itok would go away.
Comment #28
attiks CreditAttribution: attiks commentedI understand it needed a solution, but the solution from #12 solves the problem for +80% without having any impact on module developers and/or themers.
DOS/DDOS is IMHO better handled on server or infrastructure level, not on the PHP level, and if we decide to handle it on PHP level, we need to do it for all possible attack points, not only images.
Comment #29
pwolanin CreditAttribution: pwolanin commented@David - yes, making sure existing linked work when the variable is toggled is another importnat (maybe more important) reason too.
So, i think the patch in #12 is a start, but not complete and we need to think about the possible combinations of settings that people could want.
In roughly declining order of security, we can imagine these settings:
A) derivatives protected by a token, nested derivatives disabled
B) all derivates protected by a token, nested derivates enabled
C) no derivatives protected by a token, but nested derivates disabled
D) non-nested derivates not protected by a token, nested derivates protected with token
E) please DoS me
7.21 basically offers you the choice of B and D, though in D still emitting the token.
Given that it's an opaque setting (no admin form) maybe a patch we could put is would be to not emit the token on non-nested derivates when bypass variable is enabled? Otherwise, we could easily add another variable to toggle this?
Comment #30
attiks CreditAttribution: attiks commented#29 I like the idea of multiple options, all 5 of them, but I would add an option to allow a specific number of recursion. I also think it would be wise to add an admin form, so 'morals' can configure this as well. Since some of these options will disable the output of itok, we would need some decent help text explaining the consequences of toggling these settings.
Variables needed: use_itok (*All*, None, Nested only), number_of_recursions_allowed (*0* .. n)
A) use_itok = All, number_of_recursions_allowed = 0
B) use_itok = All, number_of_recursions_allowed = 9
C) use_itok = Nested only, number_of_recursions_allowed = 0
D) use_itok = Nested only, number_of_recursions_allowed = 9
D2) use_itok = None, number_of_recursions_allowed = 0 (is patch in #12)
E) use_itok = None, number_of_recursions_allowed = 9
Maybe we need an extra variable to control the output of the itok as well, so people can keep outputting the itok even if they want to disable the checking (C, D2, E), but if they want to they can disable the outputting as well for these cases. Option A, B, C and D imply that the token will be generated, so basically they can only suppress to output of the token if use_itok = None
Alternatively we can use the following options for use_itok: *All*, None, Nested only, None but output token anyway
Comment #31
pwolanin CreditAttribution: pwolanin commentedI don't think we have a reliable way to detect the degree of recursion?
Also, these setting are not for mortals - basically we want most people stick with the default secure setting and they can vset via drush if they are in a special situation.
Comment #32
attiks CreditAttribution: attiks commented#31 As long as you don't store your images in a folder called 'styles' it can be detected, if you call your folder styles each level will count for 2.
So no settings form, ok for me. Any other feedback on #30?
Going to mark this NR to get some more attention, so this can be solved
Comment #33
attiks CreditAttribution: attiks commentedCan any of the core comitters give feedback on the proposal above?
Comment #34
pwolanin CreditAttribution: pwolanin commentedI don't think the patch is complete, so still needs work - can you post a new patch?
Also, I honestly think we are working too hard to support a feature no one on the issue has said they are using? Is there anyone using manually generated nested styles except maybe KhaledBlah?
I guess I can see from this thread the possible need for 2 additional variables in addition to what's in 7.21.
1) a variable that disables the token from being emitted when it's not needed for access
2) a variable that provides the 7.20 behavior of allowing any derivative without a token
Comment #35
attiks CreditAttribution: attiks commentedI'm willing to write the patch, but I would love to know if there's a change it gets committed, otherwise it's a waste of time.
#34 using two variables is also fine by me.
Comment #36
pwolanin CreditAttribution: pwolanin commentedThe core committers are participating in this issue, so I expect it's got their attention and consideration.
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedI have the same question as @pwolanin - it's still not clear to me what specific use case we're trying to address here that isn't already possible with Drupal 7.21?
If there's a good reason to let people stop the token from being emitted in URLs, I don't see a problem with adding a variable for that. (#1936176: image.module uses file_create_url() incorrectly might be a more robust method for allowing that though, and seems like it would have other benefits too.)
Comment #38
attiks CreditAttribution: attiks commented#37Use case is to be able to output image url's without any tokens, I started working on a project where Drupal will be used as a site generator, the visible site will be static HTML, so there's no need for any tokens on image URL's, and most derivatives of images will not exist to moment the static version is generated, so we need to disable the checking of itok (possible) and suppress the output of itok (possible if we add a variable to control this).
Using hook_file_url_alter() might be a solution (#1936176: image.module uses file_create_url() incorrectly), but I guess a variable will be easier for most users. hook_file_url_alter is probably also more expensive than checking a variable.
I'll try working on a patch (later today) that adds a variable to suppress to output of itok, and allows infinitive depth of recursion.
Comment #39
vandam-me CreditAttribution: vandam-me commented#12: i1934498-12.patch queued for re-testing.
Comment #41
KhaledBlah CreditAttribution: KhaledBlah commentedSo I guess any use case that isn't covered in the "top 90%" is not a valid use case. Nice. I think my use case is very valid and I also believe we could come up with core code that prevents this token to be generated by people who chose not to have it.
Comment #42
attiks CreditAttribution: attiks commented#41 I just started a project where I need to output static html as well, so I'll try to provide a patch ASAP.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commented@KhaledBlah, it's still not 100% clear to me from your above comments why exactly 'image_allow_insecure_derivatives' doesn't work for you (and why you need the tokens to be additionally removed from the links).... whether it's because you have JavaScript which is looking for a particular URL pattern that is now no longer there, or something else.
I think everyone above has agreed that a separate variable to prevent the token from being emitted is a reasonable addition (even if not an ideal one).
Comment #44
Jelle_SFollowing patch tries to achieve what is described in #34 in Drupal 7.
As said in #12: If such a patch is accepted I'll roll this agains D8 as well.
Setting to D7 for testbot.
Comment #46
KhaledBlah CreditAttribution: KhaledBlah commented#43, I use JavaScript to blend-in/blend-out small images and I use a "pre-compiled" list of (partial) URIs for that purpose. So this code expects these images to be available at a URL that doesn't have the token.
Comment #47
pwolanin CreditAttribution: pwolanin commentedI'm not sure why the patch include the allowed levels of nesting - is that a use case we want to support?
Maybe rather than trying to think of every combination of variables, we should have an alter hook for this? I guess that would be a little slower, but feels like there is a matrix of possible settings that are hard to explain.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedSo why doesn't the existing 'image_allow_insecure_derivatives' variable work for you then? With that variable set, the images will be accessible at a URL without the token.
***
The patch looks like a decent start, but I agree with @pwolanin - I don't understand the point of the nesting level variable. Does anyone out there have an actual use case for that one? (Note that for extreme edge case situations, hook_menu_alter() will already work.)
Also, I noticed that the code which checks 'image_style_url_add_token' fails to run for non-clean URLs.
Comment #49
attiks CreditAttribution: attiks commented#48 the images will be accessible with 'image_allow_insecure_derivatives', but altering the src for the images will be 'harder' and the code doesn't look as nice without the itok.
I'll try a reroll with the nesting level variable, so with only a variable to disable the creation of itok, and a variable to allow any kind of nesting or not. (default = not)
Comment #50
KhaledBlah CreditAttribution: KhaledBlah commented@David_Rothstein, that seems like a waste of time to me. Once @attiks patch is availabe I will disable the itok patch and be done with it. It would take me a few hours to change (and test) my code because of a change that is too intrusive IMO.
Comment #51
attiks CreditAttribution: attiks commentedFirst try, I added a new setting 'suppress_itok_output'
Comment #52
ufku CreditAttribution: ufku commentedIs there any specific reason for not allowing derivatives per role by using permission definitions just like in D6 imagecache?
Also see #1955554: Enable insecure image derivatives by role
Comment #53
greggles@ufku - that seems vulnerable to csrf, right? Admittedly it's trickier to DOS via CSRF, but if you can get someone logged in on a site to visit a page with a bunch of
<img width=0 height=0 src=blah>
(or hidden in a 1 pixel iframe or...) then it seems nearly as effective as the problem the original fix was trying to address.Comment #54
ufku CreditAttribution: ufku commented@greggles: as you mentioned in your img example any drupal path is vulnerable to csrf-ddos. It comes to the question "Should drupal protect against DDOS?"
Comment #55
gregglesAny path is vulnerable to that, but image derivatives have the ability to fill disk or consume cpu much faster than a typical url. That's why this issue earned an SA.
Comment #56
attiks CreditAttribution: attiks commented2 questions:
Bumping this to critical, since it will provide a solution for #1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 7.20 if image styles have been re-saved
Comment #57
neRok CreditAttribution: neRok commentedNot sure if this is the right place to post this, but itok seems like a band-aid solution that needs more band-aids as per the patch you are making now. Im just wondering why image styles arent selected per field, rather than 'on the fly'? It seems this would solve the flooding problem. For example, I know my node will only need scaled-thumbnails linking to the full size image. So when I create the image field, I can tick on which styles I will enable for this image field. Image derivatives could then be created upon node save, not at other times. If I decided I wanted to use a different style, say square-thumbnails, I would have to go back to the field and change this setting. This is not really a burden in any way, as I would have to go change the nodes display settings or views etc to achieve the change with the current drupal way.
Comment #58
KhaledBlah CreditAttribution: KhaledBlah commentedIs this issue dead? Can I help somehow? Will something be done about the problems this change (itok) brings?
Comment #59
jcisio CreditAttribution: jcisio commentedPatch looks good, but I'll leave a more senior contributor to review it. BTW, I think we should default suppress_itok_output to TRUE, as saying in this issue title.
Comment #60
pwolanin CreditAttribution: pwolanin commentedIs Drupal 8 fully in sync with 7.21 now?
So, it looks like this patch allows you to omit the token always, and we will still never allow more than 1 level of image derivative without a token?
Comment #61
attiks CreditAttribution: attiks commented#60 AFAIK they are in sync and your description of the patch is correct. We just need someone to RTBC this (or provide feedback to improve)
Comment #62
attiks CreditAttribution: attiks commentedFYI: While testing the patch of #1279226-134: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page, google PageSpeed gave the following remark
"Resources with a "?" in the URL are not cached by some proxy caching servers. Remove the query string and encode the parameters into the URL for the following resources"
https://developers.google.com/speed/pagespeed/insights#url=http_3A_2F_2F...
Comment #63
Shyamala CreditAttribution: Shyamala commentedUpdated issue summary.
Looking to having this patch in core, the itok is interfering with our CDN integration.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commented@David_Rothstein If next release http://groups.drupal.org/node/298393 will be coming then is there any chance to fix this.
Comment #65
attiks CreditAttribution: attiks commented#64 We need at least one person to mark this RTBC so it can get fixed in Drupal 8, only after it is submitted it can be backported to Drupal 7.
Comment #66
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the patch and issue summary. I'm updating the title and priority based on what the patch does. Here is a review:
The CDN module fixed this a long time ago (#1926884: CDN module is not compatible with security fix in Drupal core update 7.20) although I guess it's possible the patch here would help with some other particular CDN setups. As for third party integrations, again, if they haven't already been updated (many/most have) then the existing "image_allow_insecure_derivatives" variable should really work around almost all of them on its own; I'm still not sure anyone has come up with a specific piece of code that the patch here will fix though it's possible some exists.
So, this isn't a critical issue (and I'm not sure it's major either but leaving it at that).
I don't see the point of this; I think we can just document that if you set the second variable you need to set the first too? Especially if the word "security" doesn't appear in the second variable name I don't like that it has security implications.
That looks like it's part of #1934568: Allow sites using the 'image_allow_insecure_derivatives' variable to have partial protection from the Drupal 7.20 security issue rather than this issue... And when it comes time to integrate that with the code here (e.g., in the Drupal 7 patch) I don't think there's any reason to allow sites to bypass that check. It's very dangerous to do so, and in practice I don't believe anyone has ever said they need it for legitimate use cases?
I think it would be better to have the tests run in all cases (e.g., public and private). That will conflict a bit with the tests being added in #1934568: Allow sites using the 'image_allow_insecure_derivatives' variable to have partial protection from the Drupal 7.20 security issue but I don't think that can be helped.
I'll see if I can do a quick reroll to address those.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedMoving to "needs work" until I post a new patch.
Also, @naveenvalecha, the release window on Wednesday is for a security release only so it won't include this patch.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedSomething like this, perhaps?
Comment #69
StevenPatzThat's the security release window. The bug fix release window is in June.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedBy the way:
That's interesting.... but doesn't Drupal also add a "?" in the URL to CSS and JS files in some cases? (At least ones that aren't aggregated.) Maybe there needs to be an issue to discuss that?
Comment #71
attiks CreditAttribution: attiks commented#70 it is indeed unrelated to this issue, so a new issue might be a good idea.
Comment #72
jcisio CreditAttribution: jcisio commented#70 #71 with Assetic in core, I think we can wait a bit more so see if those querystring go away alone. Also, it is not a big problem for a few CSS/JS files, not like images.
Comment #73
David_Rothstein CreditAttribution: David_Rothstein commentedSo, the patches in #68 pass/fail as expected; I'd be happy with RTBC if others are.
Comment #74
David_Rothstein CreditAttribution: David_Rothstein commented@neRok (#57):
Yes, it should be another issue - I'm not sure if an issue exists already, but you're basically proposing what was proposed in http://berk.es/2013/03/04/drupal-imagecache-security-vulnarability-with-... as a long term solution.
However, that would definitely be Drupal 8+ only, since it's a massive change that would affect the image system in a lot of ways. It would also be necessary to make sure that it works in a way such that admin users don't accidentally DDOS themselves :) For example, if you have an existing image style with thousands of derivatives and then re-save the image style configuration itself (for example, change the size or scaling properties or anything else), those thousands of images all need to get updated in response and it can't be all at once.
Comment #75
timaholt CreditAttribution: timaholt commented@David_Rothstein (#74) and @neRok (#57):
What if a simpler approach would be to do what neRok is suggesting, except not generate the images on node_save? Basically then you would select image styles per field, and when the image url is hit it would look to see if that particular style is allowed. This way it doesn't cause issues with something like feeds imports of large #s of nodes or when changing styles, but it effectively allows you to "whitelist" which image derivatives can be generated for a particular field.
Comment #76
attiks CreditAttribution: attiks commentedthanks
Comment #77
benjifisher#68: itok-1934498-68.patch queued for re-testing.
Comment #78
adamdicarlo CreditAttribution: adamdicarlo commentedGoing to re-roll this if necessary.
Comment #79
adamdicarlo CreditAttribution: adamdicarlo commentedGoing to re-roll this if necessary.
Comment #80
adamdicarlo CreditAttribution: adamdicarlo commented#68: itok-1934498-68.patch queued for re-testing.
Comment #81
adamdicarlo CreditAttribution: adamdicarlo commentedThe patch still applies cleanly. It passed automated tests on May 14.
I tested it like this:
drush php-eval "config('image.settings')->set('suppress_itok_output', TRUE)->save();"
to turn on the new option.Don't be alarmed by
itok-1934498-68-TESTS-ONLY.patch
failing tests -- that's expected, as the fix is not included in that patch. Without the fix, the new tests should fail.Queued for retesting since presumably a ton of commits have gone in since the patch was written.
If it passes again, then it's ready to be committed.
Comment #82
adamdicarlo CreditAttribution: adamdicarlo commentedYep, tests are still passing. This is RTBC!
Comment #83
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks. I believe this needs to be back-ported to 7.x.
We could argue what the default value should be for this setting. Instead of 'surpress_xxx = false' it could simply be called 'enable_xxx = true'. The double negation is a bit confusing.
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedBefore porting to D7, let's get the default value of the setting (currently, implicitly "false") into core/modules/image/config/image.settings.yml.
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedThis implements #84, but not #83. I actually prefer the suppress name, given the way in which it only works in conjunction with allow_insecure_derivatives: i.e., it only make sense to be true when allow_insecure_derivatives is true.
Comment #86
tstoecklerThe config schema was missing for both the original config property and the new one added here.
Comment #88
tstoeckler#86: image-suppress_itok_output-86.patch queued for re-testing.
EDIT: I assume the breakage was due to (temporarily) broken HEAD from #1798654: Clean faulty HTML in help description of field widgets.
Comment #89
skyredwangI am late to this issue.
Can we add two more permissions in Image module:
1. Skip simple imagecache itok check (Default: ANONYMOUS off, AUTHENTICATED off, ADMINISTRATOR on)
2. Skip recursive imagecache itok check (Default: ANONYMOUS off, AUTHENTICATED off, ADMINISTRATOR on)
This way roles with sufficient access can bypass the screening process, which makes the security fix a little bit more robust.
Comment #90
bebelg CreditAttribution: bebelg commented#51: i1934498-51.patch queued for re-testing.
Comment #91
cancerian7 CreditAttribution: cancerian7 commented#12: i1934498-12.patch queued for re-testing.
Comment #92
cancerian7 CreditAttribution: cancerian7 commentedPlease backport it to drupal 7.
Comment #93
hanoiiSorry to request info that's probably around, I read this issue, the SA as well as the release notes but I can't really understand what the derivative token does, can someone be kindly enough to explain it to me? I get what it is, not sure how it prevents the DDOS attack? I was asked what it was and when it went to figure it out myself somehow I am not getting it.EDIT: I found a great post http://berk.es/2013/03/04/drupal-imagecache-security-vulnarability-with-... that explains most of it.
Comment #94
penyaskitoRTBCing #86, unless someone wants to argument about Dries pointed naming issue in #83 that tstoeckler commented on #85.
Comment #95
catchThis isn't my favourite patch but it does make the output nicer if you already have insecure derivatives, and we have at least two other issues open for alternative fixes.
Moving over to David Rothstein to make sure he's happy with this going into 7.x as a backport. David if you're happy please unassign and I'll commit to 8.x.
Comment #96
jcisio CreditAttribution: jcisio commentedI think this is a task. The feature was committed in #83 and two configs were missing so it needs to be committed in D8 anyway, doesn't it?
Comment #97
catchOh good point! I zoned out that Dries had already committed the main patch. Committed/pushed to 8.x, then.
Comment #98
David_Rothstein CreditAttribution: David_Rothstein commentedYup, I'm happy with the general approach for 7.x as well. Still not fully understanding why this matters to people, but I'm OK with committing something like it to 7.x.
Comment #98.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdating Issue summary using Issue summary template, also updating details to make it more current.
Comment #99
julien_g CreditAttribution: julien_g commentedOn Drupal 7, I have added a drupal_alter on the image_style_url file in order to be able to remove itoks without modifying the original behavior. This was the solution choosen for my need.
With that call of Drupal alter, I juste have to create a module that catch my new hook and remove the itok :
Comment #100
julien_g CreditAttribution: julien_g commentedComment #101
tstoecklerHere's a straight backport from D8. Because there are already additional tests in D7 vs. D8 I had to adapt those a little bit. If they pass though, it should be good, I only deleted a bunch of lines that were already present.
@julien_acti: I don't necessarily find the idea of a hook_image_style_url_alter() bad, but let's discuss that in a separate issue. This was already hard enough to get in on its own. If you link the newly created issue here, people can follow-up over there directly. Thanks!
Comment #102
tstoecklerOops, that was unintentional. I'm assuming that can be fixed pre-commit, so not re-rolling just for that.
Comment #103
julien_g CreditAttribution: julien_g commented@tstoeckler : ok to separate the issue to discuss separatly my idea to remove itok
Here is the issue that propose an other way to offer the possibility to remove the itok : https://drupal.org/node/2138279
Comment #104
helmo CreditAttribution: helmo commentedHere's a re-roll of #101 after the 7.26 release.
Still works a designed.
Comment #106
clemens.tolboomSome notes
Adding more output to the failing test like
shows the token is regenerated on each call (doh).
Values are
[edit]
http://drupal.d7/sites/default/files/simpletest/180755/styles/style_foo/public/image-test_0.png?itok=ybHcR_GC
http://drupal.d7/sites/default/files/simpletest/180755/styles/style_foo/public/image-test_0.png?itok=mCEF2wse
[/edit]
The failing test is added through #1955378: Return same derivative token with path or URI. That issue introduced a non used variable $original_uri in
.
This needs some more research.
Comment #107
Hadiseh CreditAttribution: Hadiseh commented101: 1934498-101-d7-suppress-itok.patch queued for re-testing.
Comment #109
KhaledBlah CreditAttribution: KhaledBlah commentedEDIT: Please note the mentioned patch is attached to #110.
Since I still have the same issue that I explained above I created a patch based on the patch in #101 (which is meant for d8 appearantly). I used drupal 7.27 and I am publishing the patch here for those who have the same issue.
After applying the patch (please note that the patch has to be re-applied after a drupal upgrade because it modifies a core file in modules/image/image.module) you have to set the variable "suppress_itok_output" to TRUE. You can do so by adding this line to your settings.php (there are other ways obviously):
Comment #110
KhaledBlah CreditAttribution: KhaledBlah commentedSorry, I added the wrong file in the comment before. This one is the correct one.
Comment #111
clemens.tolboom@KhaledBlah your patch is missing the test #104
Please add the test back then set issue status on needs review.
Comment #112
shawnrosspeters CreditAttribution: shawnrosspeters commented@KhaledBlah, Have you had a chance to test the patch so it can be added back for review?
Good work btw.
Comment #115
KhaledBlah CreditAttribution: KhaledBlah commentedSorry for the (very) late reply!
@clemens.tolboom: I am sorry I must admit I have no idea how the patch testing system works (yet)! I will have to read up on it.
@shawnrosspeters: the patch in #110 works for me. I have been using it for a while now. As far as I am concerned it is ready for review.
Comment #116
mikeytown2 CreditAttribution: mikeytown2 commentedJust a heads up that if you want this functionality now the imageinfo_cache module can do this.
Comment #117
helmo CreditAttribution: helmo commentedI can confirm that I no longer needed this core patch after starting to use imageinfo_cache module.
Thanks @mikeytown2
Comment #118
attiks CreditAttribution: attiks commentedPatch is still needed to suppress the output of itok, AFAIK imageinfo_cache module solves part of the problem
Comment #119
attiks CreditAttribution: attiks commentedNew patch, wording changed to reflect Drupal 7 and test added
Comment #120
jcisio CreditAttribution: jcisio commentedNo we don't need this patch, at least for 99% the case, where the default theme_image() is used. This function does:
and a bit after that:
Thus the itok token can be suppressed u using hook_file_url_alter(). AFAICT, this has been changed recently so that the CDN module could work.
Of course, if you use image_style_url() directly in your code then it won't work without this patch.
Comment #121
attiks CreditAttribution: attiks commentedThe problem is that not all contrib modules (like picture) use file_create_url, picture calls image_style_url so it doesn't work.
Comment #122
attiks CreditAttribution: attiks commentedSince I only rerolled the patch, RTBC
PS: This is the only way to get a performance grade of 100%
Comment #123
jcisio CreditAttribution: jcisio commentedIn Picture module, you can do the same thing as the theme_image in core: instead of
you do:
It's hacky because file_create_url() is used twice, but at least it works instead of struggling with a D7 issue in core ;)
Comment #124
jcisio CreditAttribution: jcisio commentedPS: I read the issue again. I didn't see/forgot that David_Rothstein was ok (a year ago!) with the direction of the patch. But as it is fixed in D8, I didn't hope for D7 btw.
Comment #125
attiks CreditAttribution: attiks commented#123 It is a struggle, but it is the only way to improve, contrib should not do hacky things to make this work
Comment #126
attiks CreditAttribution: attiks commentedIt is @David_Rothstein who has to commit it, let's hope he does
Comment #127
Wim LeersThat'd be a bug: all file URLs MUST be created using
file_create_url()
, otherwise they would not be alterable, and hence be impossible to serve from a CDN.Comment #128
attiks CreditAttribution: attiks commented#127 image_style_url calls file_create_url
Comment #130
attiks CreditAttribution: attiks commented#129 Test bot fluke, re ran test of #119
Comment #131
David_Rothstein CreditAttribution: David_Rothstein commentedTestbot fluke, back to RTBC.
Comment #134
mikeytown2 CreditAttribution: mikeytown2 commentedComment #135
mikeytown2 CreditAttribution: mikeytown2 commentedfix the fat finger
Comment #138
David_Rothstein CreditAttribution: David_Rothstein commentedThat was a bogus test failure, so back to RTBC for now.
Comment #139
pounardHum, this should be commited, it seems that the 'itok' protection does not even prevent the DDoS it tries to prevent, it only makes it a bit less probable to catch (even thought an attacker that knows how to read PHP code will still be able to do it quite easily).
Comment #140
pounardAm I reading this wrong or the patch will make the Drupal site unstable if the
image_allow_insecure_derivatives
is not set? Drupal will generate images without the token, but the callback will try to check for the token, no images at all will ever be generated.I think a better approach is to re-use the
image_allow_insecure_derivatives
variable instead.Comment #141
pounardHum or maybe you should add at the place where:
if (variable_get('image_allow_insecure_derivatives')) {
this
if (variable_get('image_allow_insecure_derivatives') || variable_get('image_suppress_itok_output')) {
in order to be sure that setting the new variable will induce the right behavior everywhere.
Comment #143
David_Rothstein CreditAttribution: David_Rothstein commentedI'm not sure what you mean by this, but if there's a security issue remaining here in Drupal 7, please report details to the security team privately.
You're reading it right. That is why the code comment basically only recommends using the new variable if 'image_allow_insecure_derivatives' is also being used. (Do you think the comment could be worded differently to make that more clear?)
I think the reason for not doing that is explained in the code comment visible in the context at the top of the patch file:
Basically we want people who are just using 'image_allow_insecure_derivatives' to be able to turn it off whenever they're ready to, without fear of breaking any links.
This could be done, but I think the main reason we didn't want to was that the name "image_suppress_itok_output" doesn't imply that setting the variable on its own could have security implications. (It could be given a different name of course, but the Drupal 8 patch already went in with the equivalent name.) Also, having each variable do one specific thing just seemed cleaner.
I think this is really going to be a very rarely-used variable... we can hopefully assume that anyone using it will know enough of what they are doing to understand that both variables need to be set.
Comment #145
David_Rothstein CreditAttribution: David_Rothstein commentedThat was a bogus testbot failure, so moving back to RTBC for now.
Will leave for a few more days before committing though, in case there's further feedback.
Comment #146
KhaledBlah CreditAttribution: KhaledBlah commentedI am not sure if this is the appropriate place but I joined this debate because I wanted to completely remove the "itok" parameter from image URIs because it breaks my use case. We use drupal in an environment where only a limited number of people have access to the system so removing the itok parameter is not such a risk but having it breaks some other code which is more difficult to fix. Hence, I developed a fix based on hook_file_url_alter() which I would like to contribute here in case anybody needs it and maybe someone can point something I missed.
This code removes the "itok" parameter from image URIs without having to patch core code.
Comment #147
pounardKhaledBlah Nice, that's a good move. I already did patched core for a few projects for the next one I'll certainly try to do it this way.
Comment #148
mikeytown2 CreditAttribution: mikeytown2 commentedRepeating #116. The https://www.drupal.org/project/imageinfo_cache module has this functionality built in as well.
Lets get this committed!
Comment #151
David_Rothstein CreditAttribution: David_Rothstein commentedTestbot fluke.
Comment #152
David_Rothstein CreditAttribution: David_Rothstein commentedAlright, no followup objections any time recently, so let's do this :)
I'm not 100% sure the tests are the correct backport from Drupal 8, but there's enough other discrepancies in these tests between Drupal 7 and Drupal 8 already and another issue open to try to reconcile them. I think the test here is good enough for this feature for now.
Committed to 7.x - thanks!
Comment #154
David_Rothstein CreditAttribution: David_Rothstein commentedI added this to CHANGELOG.txt and the release notes too, since it introduces a new variable.
Comment #157
atul_shin CreditAttribution: atul_shin commentedI am using Drupal 8.2.0-dev.
I need some help with token suppressing for images and I have tried some settings in settings.php but none works to suppress the token in image paths.
Or is there way so that images are not broken with tokens in path ?
Thanks
Comment #158
David_Rothstein CreditAttribution: David_Rothstein commentedPlease create a separate issue for that.
The particular configuration setting added to Drupal 8 in this issue can be enabled via
$config['image.settings']['suppress_itok_output'] = TRUE
in settings.php followed by a cache clear. However, images in Drupal should not be broken just because there are tokens in the path.... So instead of suppressing the tokens and lowering security, it would make more sense to debug what is breaking them.Comment #159
atul_shin CreditAttribution: atul_shin commentedHi David,
That did help me in achieving what I want. When images are uploaded on site some of them do not appear due to permissions but a lot (not all) do not show up. So when I remove the "itok " from the code using Developer tools, the images do show up. Its something that I could not figure out as why only newly uploaded images do not show up.
But thanks a lot for introducing to new settings. Is there a way a list of all settings which are allowed in settings.php available ?
Thanks
Atul
Comment #160
cheans CreditAttribution: cheans as a volunteer and commented@atul_shin #159
I hava same problem, did you solved it?
Comment #161
rovoRunning 7.50, and the iTok generation was becoming an issue for me. After reading this thread, it took me a little time to figure out how and which flags needed to be set in my settings.php.
Incase this helps anyone else, this is what worked for me:
Comment #162
nattyweb CreditAttribution: nattyweb commented#161 additions to settings.php worked for me with 7.51. I had tried every fix I could find when this problem appeared in 7.50 and nothing was working, save switching off clean urls. My problem was introduced when I updated to 7.50 but I didn't notice it immediately and so didn't relate it to the update. Tried everything I could and was about to switch to another host to try that out. Thanks @rovo - you saved me a lot of time.
Comment #163
MediaFormat CreditAttribution: MediaFormat commented#161 works!
Comment #164
JayKandariD8 version of #161