This is a Drupal 8 followup for the security issue fixed here: http://drupal.org/SA-CORE-2013-002

Credit for the patch/fix: Damien Tournoud, pwolanin, David_Rothstein, Heine, Bèr Kessels

Drupal 7 patch is here: http://drupalcode.org/project/drupal.git/patch/3a24da1b40f5e05876ad77750...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Oh, if anyone is wondering about all the check_plain() changes in the tests, those were needed to get the tests to pass.

I believe the conclusion in the security team was that this was a preexisting bug in the tests, revealed now because the image derivative URLs (when clean URLs are off) started having a & in them. But it might be worth examining this a little further.

bfr’s picture

There's no public discussion of this anywhere, could someone describe HOW this vulnerability actually works?
Does it only occur when we have thousands of images with several versions allowed and all those are triggered at once, or can the attacker bypass predefined styles and generate versions that should not be possible to generate?

FiNeX’s picture

Did somebody analyzed the "token" solution implemented in 7.20 from a SEO point of view?

David_Rothstein’s picture

Cross-linking #1923554: New anti-DoS measure breaks for some file URIs (if that gets committed to Drupal 7 first it would need to be incorporated into the Drupal 8 patch here as well).

FiNeX’s picture

About the SEO related issue I'd like to cross-post this comment:

I'm thinking about a duplication/crawling problem of the image URL by Google.

Google would crawl two times the same URL and this is a waste of resources. Even if the crawler should be smart and group the content I don't like the idea. Anyway to it should be enough to exclude "itok" from the URL parameters on Webmaster Tools to be safe and let the crawler to get only the base URL.

What do you think about?

(source: https://drupal.org/node/1922812#comment-7099320)

David_Rothstein’s picture

I don't see what the issue would be? When a search engine crawls your site at some point it would re-fetch images anyway. Perhaps the new URLs make it happen sooner than it would otherwise, but either way crawlers should be pretty friendly about not overwhelming your site with requests.

Also, in most cases there will not be duplicate URLs for the same image, but rather all old links on the site will be replaced with the new one. (If there were hardcoded links to the old URLs e.g. embedded in content then both might co-exist after upgrading to Drupal 7.20. However, the two URLs point to the same exact content on the same exact site with the only difference being a query parameter, so I would think any search engine out there is smart enough to figure out it's the same item.)

FiNeX’s picture

I'm a bit more cautious so I suggest to document the new "itok" url parameter in order to explicitly inform about this (even if usually search engines are able to discover url parameters which doesn't alter the content).

Thanks for the attention :-)

Berdir’s picture

Status: Active » Needs review
FileSize
6.52 KB

Ported the patch to 8.x, did not yet apply the check_plains()'s, would like to see if this happens in 8.x as well.

The "problem" might not be a problem at all but expected behavior, because the generated URL does not encode the & whereas the generated HTML does. I'm not sure if check_plain() is the correct way to convert it or if just converts it correctly as a side effect. Maybe it would make more sense to convert the & more explicitly to &.

Damien Tournoud’s picture

@Berdir: check_plain() is correct, because we are looking for the raw HTML. Alternatively, we could use xpath to get the value of the attribute, which would be un-encoded in that case.

Berdir’s picture

Ok :)

Anyway, I actually think it won't be a problem anymore in 8.x because we no longer have ?q=path, we are doing index.php/path?

Status: Needs review » Needs work
Issue tags: -Security Advisory follow-up

The last submitted patch, image-style-dos-1922814-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Security Advisory follow-up

#8: image-style-dos-1922814-7.patch queued for re-testing.

Had the usualy missing cache table random failure. Passed everything else.

Berdir’s picture

Included the fix from #1923554: New anti-DoS measure breaks for some file URIs so that we can get this committed and either this issue backported or keep the the other one for 7.x

Anything left to do here?

Berdir’s picture

Oh, actually looks like there isn't an agreement yet in the other issue about the correct fix. Should we continue there or here, doesn't make sense to work on both issues at the same time I think?

Berdir’s picture

Ok, the other issue is RTBC now, I have the same fix in my patch, can I haz RTBC here too? ;)

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

You can haz.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

David_Rothstein’s picture

Removing the "7.21 release blocker" tag, since the only Drupal 7 followup here was handled in #1923554: New anti-DoS measure breaks for some file URIs.

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