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

Files: 
CommentFileSizeAuthor
#13 image-style-dos-1922814-13-test-only.patch7.6 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,383 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#13 image-style-dos-1922814-13.patch7.63 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,376 pass(es).
[ View ]
#13 image-style-dos-1922814-13-interdiff.txt2.67 KBBerdir
#8 image-style-dos-1922814-7.patch6.52 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,274 pass(es).
[ View ]

Comments

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.

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?

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

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

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)

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

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

Status:Active» Needs review
StatusFileSize
new6.52 KB
PASSED: [[SimpleTest]]: [MySQL] 52,274 pass(es).
[ View ]

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

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

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.

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.

StatusFileSize
new2.67 KB
new7.63 KB
PASSED: [[SimpleTest]]: [MySQL] 52,376 pass(es).
[ View ]
new7.6 KB
FAILED: [[SimpleTest]]: [MySQL] 52,383 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

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?

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?

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

Status:Needs review» Reviewed & tested by the community

You can haz.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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.