Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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...
Comment | File | Size | Author |
---|---|---|---|
#13 | image-style-dos-1922814-13-test-only.patch | 7.6 KB | Berdir |
#13 | image-style-dos-1922814-13.patch | 7.63 KB | Berdir |
#13 | image-style-dos-1922814-13-interdiff.txt | 2.67 KB | Berdir |
#8 | image-style-dos-1922814-7.patch | 6.52 KB | Berdir |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedOh, 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.Comment #2
bfr CreditAttribution: bfr commentedThere'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?
Comment #3
FiNeX CreditAttribution: FiNeX commentedDid somebody analyzed the "token" solution implemented in 7.20 from a SEO point of view?
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedCross-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).
Comment #5
FiNeX CreditAttribution: FiNeX commentedAbout the SEO related issue I'd like to cross-post this comment:
(source: https://drupal.org/node/1922812#comment-7099320)
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.)
Comment #7
FiNeX CreditAttribution: FiNeX commentedI'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 :-)
Comment #8
BerdirPorted 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 &.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.Comment #10
BerdirOk :)
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?
Comment #12
Berdir#8: image-style-dos-1922814-7.patch queued for re-testing.
Had the usualy missing cache table random failure. Passed everything else.
Comment #13
BerdirIncluded 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?
Comment #14
BerdirOh, 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?
Comment #15
BerdirOk, the other issue is RTBC now, I have the same fix in my patch, can I haz RTBC here too? ;)
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedYou can haz.
Comment #17
webchickCommitted and pushed to 8.x. Thanks!
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedRemoving 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.