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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

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

jcisio’s picture

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

jcisio’s picture

Title: Rollback the imagestyle 'itok' token, introduce a non-recursive option » Replace image style 'itok' token with non-intrusive options
KhaledBlah’s picture

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

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Critical
Issue tags: +Needs backport to D7

Moving to 8.x and bumping to critical - this affects 7.x-8.x upgrade as well as 7.x-7.x.

David_Rothstein’s picture

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

David_Rothstein’s picture

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

catch’s picture

Priority: Critical » Major

Hmm I may be getting the issues confused, definitely removing the tokens is not critical, some better fallback behaviour I think is.

jcisio’s picture

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

KhaledBlah’s picture

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

attiks’s picture

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

attiks’s picture

Version: 8.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
8.29 KB

A 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

attiks’s picture

Version: 7.x-dev » 8.x-dev

Back to 8

jcisio’s picture

Status: Needs review » Active

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

attiks’s picture

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

Dave Reid’s picture

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

jcisio’s picture

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

attiks’s picture

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

David_Rothstein’s picture

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

greggles’s picture

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

pwolanin’s picture

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

David_Rothstein’s picture

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.

Oh 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 :)

David_Rothstein’s picture

Ah, cross-posted with @pwolanin, but I guess that was a rationale too (fingerprinting).

jcisio’s picture

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

jcisio’s picture

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

attiks’s picture

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

jcisio’s picture

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

attiks’s picture

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

pwolanin’s picture

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

attiks’s picture

#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

pwolanin’s picture

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

attiks’s picture

Status: Active » Needs review

#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

attiks’s picture

Can any of the core comitters give feedback on the proposal above?

pwolanin’s picture

I 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

attiks’s picture

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

pwolanin’s picture

The core committers are participating in this issue, so I expect it's got their attention and consideration.

David_Rothstein’s picture

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

attiks’s picture

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

vandam-me’s picture

Issue tags: -Needs backport to D7

#12: i1934498-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, i1934498-12.patch, failed testing.

KhaledBlah’s picture

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

attiks’s picture

#41 I just started a project where I need to output static html as well, so I'll try to provide a patch ASAP.

David_Rothstein’s picture

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

Jelle_S’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
2.6 KB

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

Status: Needs review » Needs work

The last submitted patch, i1934498-44.patch, failed testing.

KhaledBlah’s picture

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

pwolanin’s picture

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

David_Rothstein’s picture

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

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

attiks’s picture

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

KhaledBlah’s picture

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

attiks’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
4.93 KB

First try, I added a new setting 'suppress_itok_output'

ufku’s picture

Is 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

greggles’s picture

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

ufku’s picture

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

greggles’s picture

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

attiks’s picture

Priority: Major » Critical

2 questions:

  1. can somebody give feedback on #51
  2. if #51 is good, any chance we can add it for the next release, http://groups.drupal.org/node/291593

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

neRok’s picture

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

KhaledBlah’s picture

Is this issue dead? Can I help somehow? Will something be done about the problems this change (itok) brings?

jcisio’s picture

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

pwolanin’s picture

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

attiks’s picture

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

attiks’s picture

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

Shyamala’s picture

Updated issue summary.
Looking to having this patch in core, the itok is interfering with our CDN integration.

Anonymous’s picture

@David_Rothstein If next release http://groups.drupal.org/node/298393 will be coming then is there any chance to fix this.

attiks’s picture

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

David_Rothstein’s picture

Title: Replace image style 'itok' token with non-intrusive options » Allow the image style 'itok' token to be suppressed in image derivative URLs
Priority: Critical » Major

Thanks for the patch and issue summary. I'm updating the title and priority based on what the patch does. Here is a review:

  1. From the issue summary:

    The itok token introduced in 7.20 prevents working with CDNs, third party integrations that generate image presets on the fly.

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

  2. -  if (!config('image.settings')->get('allow_insecure_derivatives')) {
    +  if (!config('image.settings')->get('allow_insecure_derivatives') && !config('image.settings')->get('suppress_itok_output')) {
         $valid = $valid && isset($_GET[IMAGE_DERIVATIVE_TOKEN]) && $_GET[IMAGE_DERIVATIVE_TOKEN] === image_style_path_token($style->name, $scheme . '://' . $target);
       }
    

    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.

  3. +  elseif (!config('image.settings')->get('suppress_itok_output')) {
    +    $valid = $valid && strpos(ltrim($target, '\/'), 'styles/') === 0;
    +  }
    

    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?

  4.    /**
    +   * Tests image_style_url() without itok output.
    +   */
    +  function testImageStyleUrlWithoutItok() {
    +    config('image.settings')->set('suppress_itok_output', TRUE)->save();
    +    $this->_testImageStyleUrlAndPath('public', TRUE);
    +    config('image.settings')->set('suppress_itok_output', FALSE)->save();
    +  }
    

    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.

David_Rothstein’s picture

Status: Needs review » Needs work

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

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
3.8 KB
2.03 KB

Something like this, perhaps?

StevenPatz’s picture

That's the security release window. The bug fix release window is in June.

David_Rothstein’s picture

By the way:

FYI: 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"

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?

attiks’s picture

#70 it is indeed unrelated to this issue, so a new issue might be a good idea.

jcisio’s picture

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

David_Rothstein’s picture

So, the patches in #68 pass/fail as expected; I'd be happy with RTBC if others are.

David_Rothstein’s picture

@neRok (#57):

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

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.

timaholt’s picture

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

attiks’s picture

Status: Needs review » Reviewed & tested by the community

thanks

benjifisher’s picture

#68: itok-1934498-68.patch queued for re-testing.

adamdicarlo’s picture

Assigned: Unassigned » adamdicarlo

Going to re-roll this if necessary.

adamdicarlo’s picture

Going to re-roll this if necessary.

adamdicarlo’s picture

#68: itok-1934498-68.patch queued for re-testing.

adamdicarlo’s picture

Assigned: adamdicarlo » Unassigned

The patch still applies cleanly. It passed automated tests on May 14.

I tested it like this:

  1. Pull from git, apply patch, run installer
  2. Create new article node with an image
  3. Verify that when the image is displayed at /node and /node/1, it has an ?itok=.... parameter in its img src attribute.
  4. Run drush php-eval "config('image.settings')->set('suppress_itok_output', TRUE)->save();" to turn on the new option.
  5. Re-load /node, /node/1, verify that img src no longer contains ?itok parameter. (Image is still visible.)

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.

adamdicarlo’s picture

Yep, tests are still passing. This is RTBC!

Dries’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

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

effulgentsia’s picture

Status: Patch (to be ported) » Needs work

Before porting to D7, let's get the default value of the setting (currently, implicitly "false") into core/modules/image/config/image.settings.yml.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
363 bytes

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

tstoeckler’s picture

The config schema was missing for both the original config property and the new one added here.

The last submitted patch, image-suppress_itok_output-86.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

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

skyredwang’s picture

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

bebelg’s picture

Issue tags: -Needs backport to D7

#51: i1934498-51.patch queued for re-testing.

cancerian7’s picture

Issue tags: +Needs backport to D7

#12: i1934498-12.patch queued for re-testing.

cancerian7’s picture

Please backport it to drupal 7.

hanoii’s picture

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

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

RTBCing #86, unless someone wants to argument about Dries pointed naming issue in #83 that tstoeckler commented on #85.

catch’s picture

Assigned: Unassigned » David_Rothstein

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

jcisio’s picture

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

catch’s picture

Version: 8.x-dev » 7.x-dev
Assigned: David_Rothstein » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Oh good point! I zoned out that Dries had already committed the main patch. Committed/pushed to 8.x, then.

David_Rothstein’s picture

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

David_Rothstein’s picture

Issue summary: View changes

Updating Issue summary using Issue summary template, also updating details to make it more current.

julien_g’s picture

Issue summary: View changes
FileSize
619 bytes

On 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 :

function itok_fix_image_style_url_alter(&$file_url){

  if(strstr($file_url, '?itok=') != FALSE){
    $exploded_itok = explode('?itok=', $file_url);
    $file_url = $exploded_itok[0];
  }
}
julien_g’s picture

FileSize
629 bytes
tstoeckler’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D7
FileSize
3.31 KB

Here'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!

tstoeckler’s picture

+++ b/modules/image/image.test
@@ -272,6 +272,7 @@ class ImageStylesPathAndUrlTestCase extends DrupalWebTestCase {
+

Oops, that was unintentional. I'm assuming that can be fixed pre-commit, so not re-rolling just for that.

julien_g’s picture

@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

helmo’s picture

Here's a re-roll of #101 after the 7.26 release.

Still works a designed.

Status: Needs review » Needs work

The last submitted patch, 104: 1934498-104-d7-suppress-itok.patch, failed testing.

clemens.tolboom’s picture

Some notes

Adding more output to the failing test like

    $relative_path = file_uri_target($original_uri);
    $generate_url_from_relative_path = image_style_url($this->style_name, $relative_path);
    $this->assertEqual($generate_url, $generate_url_from_relative_path, 'Generated URL is the same regardless of whether it came from a relative path or a file URI.' . " $generate_url - $generate_url_from_relative_path - $relative_path - $original_uri");

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
  • image-test_0.png
  • public://image-test_0.png

[/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

function image_style_url($style_name, $path) {
  $uri = image_style_path($style_name, $path);

  // The passed-in $path variable can be either a relative path or a full URI.
  $original_uri = file_uri_scheme($path) ? file_stream_wrapper_uri_normalize($path) : file_build_uri($path);

.

This needs some more research.

Hadiseh’s picture

The last submitted patch, 101: 1934498-101-d7-suppress-itok.patch, failed testing.

KhaledBlah’s picture

FileSize
3.8 KB

EDIT: 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):

$conf['suppress_itok_output'] = TRUE;
KhaledBlah’s picture

Sorry, I added the wrong file in the comment before. This one is the correct one.

clemens.tolboom’s picture

@KhaledBlah your patch is missing the test #104

Please add the test back then set issue status on needs review.

shawnrosspeters’s picture

@KhaledBlah, Have you had a chance to test the patch so it can be added back for review?

Good work btw.

The last submitted patch, 101: 1934498-101-d7-suppress-itok.patch, failed testing.

KhaledBlah’s picture

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

mikeytown2’s picture

Just a heads up that if you want this functionality now the imageinfo_cache module can do this.

helmo’s picture

I can confirm that I no longer needed this core patch after starting to use imageinfo_cache module.

Thanks @mikeytown2

attiks’s picture

Status: Needs work » Needs review

Patch is still needed to suppress the output of itok, AFAIK imageinfo_cache module solves part of the problem

attiks’s picture

FileSize
2.84 KB

New patch, wording changed to reflect Drupal 7 and test added

jcisio’s picture

No we don't need this patch, at least for 99% the case, where the default theme_image() is used. This function does:

  $attributes['src'] = file_create_url($variables['path']);

and a bit after that:

  return '<img' . drupal_attributes($attributes) . ' />';

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.

attiks’s picture

The problem is that not all contrib modules (like picture) use file_create_url, picture calls image_style_url so it doesn't work.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Since I only rerolled the patch, RTBC

PS: This is the only way to get a performance grade of 100%

jcisio’s picture

In Picture module, you can do the same thing as the theme_image in core: instead of

image_style_url($style_name, $path);

you do:

file_create_url(image_style_url($style_name, $path));

It's hacky because file_create_url() is used twice, but at least it works instead of struggling with a D7 issue in core ;)

jcisio’s picture

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

attiks’s picture

#123 It is a struggle, but it is the only way to improve, contrib should not do hacky things to make this work

attiks’s picture

It is @David_Rothstein who has to commit it, let's hope he does

Wim Leers’s picture

The problem is that not all contrib modules (like picture) use file_create_url, picture calls image_style_url so it doesn't work.

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

attiks’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: i1934498-119.patch, failed testing.

attiks’s picture

#129 Test bot fluke, re ran test of #119

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

Testbot fluke, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: i1934498-119.patch, failed testing.

Status: Needs work » Needs review

mikeytown2 queued 119: i1934498-119.patch for re-testing.

mikeytown2’s picture

Status: Needs review » Closed (fixed)
mikeytown2’s picture

Status: Closed (fixed) » Reviewed & tested by the community

fix the fat finger

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: i1934498-119.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

That was a bogus test failure, so back to RTBC for now.

pounard’s picture

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

pounard’s picture

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

pounard’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: i1934498-119.patch, failed testing.

David_Rothstein’s picture

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

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

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

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 a better approach is to re-use the image_allow_insecure_derivatives variable instead.

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:

   // The token query is added even if the 'image_allow_insecure_derivatives'
   // variable is TRUE, so that the emitted links remain valid if it is changed
   // back to the default FALSE.

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.

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

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.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

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

KhaledBlah’s picture

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

/**
 * Implements hook_file_url_alter()
 *
 */
function my_module_file_url_alter(&$uri) {
  if (module_exists('image')) {
    module_load_include('module', 'image');
    $uri_parts = drupal_parse_url($uri);
    if (!empty($uri_parts)  && !empty($uri_parts['query'])
        && !empty($uri_parts['query'][IMAGE_DERIVATIVE_TOKEN])) {
      // the "itok" token is part of the query, now remove it
      unset($uri_parts['query'][IMAGE_DERIVATIVE_TOKEN]);

      // re-build the rest of the URI without the "itok" parameter
      $uri = $uri_parts['path'];
      if (!empty($uri_parts['query'])) {
        $uri .= '?' . drupal_http_build_query($uri_parts['query']);
      }   
    
      if (!empty($uri_parts['fragment'])) {
        $uri .= '#' . $uri_parts['fragment'];
      }   
    }   
  }
}
pounard’s picture

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

mikeytown2’s picture

Repeating #116. The https://www.drupal.org/project/imageinfo_cache module has this functionality built in as well.

Lets get this committed!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: i1934498-119.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Testbot fluke.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Alright, 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!

  • David_Rothstein committed eaa79a1 on 7.x
    Issue #1934498 by attiks, David_Rothstein, KhaledBlah, tstoeckler,...
David_Rothstein’s picture

Issue tags: +7.36 release notes

I added this to CHANGELOG.txt and the release notes too, since it introduces a new variable.

Status: Fixed » Closed (fixed)

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

atul_shin’s picture

Version: 7.x-dev » 8.2.x-dev
Issue tags: -7.36 release notes +8.2.0 release notes

I 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

David_Rothstein’s picture

Version: 8.2.x-dev » 7.x-dev
Issue tags: -8.2.0 release notes +7.36 release notes

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

atul_shin’s picture

Hi 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

cheans’s picture

@atul_shin #159
I hava same problem, did you solved it?

rovo’s picture

Running 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:

$conf['image_suppress_itok_output'] = TRUE;
$conf['image_allow_insecure_derivatives'] = TRUE;
nattyweb’s picture

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

MediaFormat’s picture

#161 works!

$conf['image_suppress_itok_output'] = TRUE;
$conf['image_allow_insecure_derivatives'] = TRUE;
JayKandari’s picture

D8 version of #161

$config['image.settings']['suppress_itok_output'] = TRUE;
$config['image.settings']['allow_insecure_derivatives'] = TRUE;