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?

Files: 
CommentFileSizeAuthor
#104 1934498-104-d7-suppress-itok.patch2.9 KBhelmo
FAILED: [[SimpleTest]]: [MySQL] 40,820 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#101 1934498-101-d7-suppress-itok.patch3.31 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934498-101-d7-suppress-itok.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#100 image-alter-image-url_1934498_99.patch629 bytesjulien_acti
#86 image-suppress_itok_output-86.patch944 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 56,012 pass(es).
[ View ]
#85 image-suppress_itok_output-85.patch363 byteseffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,895 pass(es).
[ View ]
#68 itok-1934498-68-TESTS-ONLY.patch2.03 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 55,747 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#68 itok-1934498-68.patch3.8 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 55,728 pass(es).
[ View ]
#51 i1934498-51.patch4.93 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1934498-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 i1934498-44.patch2.6 KBJelle_S
FAILED: [[SimpleTest]]: [MySQL] 40,034 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#12 i1934498-12.patch8.29 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1934498-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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

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.

Title:Rollback the imagestyle 'itok' token, introduce a non-recursive optionReplace image style 'itok' token with non-intrusive options

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?

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.

@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 to Drupal 7.20 if image styles have been re-saved... unless someone can articulate a good reason to kill the tokens after all.

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.

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.

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

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

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

Version:8.x-dev» 7.x-dev
Status:Active» Needs review
StatusFileSize
new8.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1934498-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Back to 8

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.

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.

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.

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

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?

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?

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.

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.

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

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

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

This issue is only major. But once it is solved, one critical issue #1923814: Existing hardcoded images can break after updating to Drupal 7.20 if image styles have been re-saved is automatically won't fix.

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?

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.

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.

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

#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

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.

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

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

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

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.

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

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

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

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.

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.

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

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

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new2.6 KB
FAILED: [[SimpleTest]]: [MySQL] 40,034 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

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.

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

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.

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

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

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

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new4.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1934498-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

First try, I added a new setting 'suppress_itok_output'

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

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

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

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.

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 to Drupal 7.20 if image styles have been re-saved

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.

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

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.

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?

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

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

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

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

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

Title:Replace image style 'itok' token with non-intrusive optionsAllow 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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 55,728 pass(es).
[ View ]
new2.03 KB
FAILED: [[SimpleTest]]: [MySQL] 55,747 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Something like this, perhaps?

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

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?

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

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

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

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

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

Status:Needs review» Reviewed & tested by the community

thanks

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

Assigned:Unassigned» adamdicarlo

Going to re-roll this if necessary.

Going to re-roll this if necessary.

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

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.

Yep, tests are still passing. This is RTBC!

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.

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.

Status:Needs work» Needs review
StatusFileSize
new363 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,895 pass(es).
[ View ]

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.

StatusFileSize
new944 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,012 pass(es).
[ View ]

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.

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.

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.

Issue tags:-needs backport to D7

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

Issue tags:+needs backport to D7

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

Please backport it to drupal 7.

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.

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.

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.

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?

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.

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.

Issue summary:View changes

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

Issue summary:View changes
StatusFileSize
new619 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];
  }
}

StatusFileSize
new629 bytes

Status:Patch (to be ported)» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new3.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934498-101-d7-suppress-itok.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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!

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

@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

Issue tags:-needs backport to D7
StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] 40,820 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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.

Some notes

Adding more output to the failing test like

<?php
    $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

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

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