D8 follow-up for http://drupal.org/SA-CORE-2013-001
reported by kressin
"the problem is related to image-styles or imagecache actions. Drupal seems to block the original images but once they get altered by an image style module the security of the private file system no longer is given."
The fix for D7 is to modify hook_file_download() to confirm there's at least one module granting access and none denying access.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | image-file-download-test-coverage-with-revert-1890754-17.patch | 1.49 KB | berdir |
| #17 | image-file-download-test-coverage-1890754-17.patch | 823 bytes | berdir |
| #7 | grrrrrr-file-download-1890754-6-test-only.patch | 1005 bytes | berdir |
| #6 | grrrrrr-file-download-1890754-6.patch | 1.72 KB | berdir |
| #5 | image-1890754-5.patch | 759 bytes | tim.plunkett |
Comments
Comment #1
pwolanin commentedAttached is that patch that was committed to Drupal 7. This patch was authored by Heine.
Comment #2
pwolanin commentedtagging
Comment #3
catchComment #4
David_Rothstein commentedThis one needs tests as well, which can then be backported to Drupal 7.
(The security team just doesn't have the resources right now to write tests for all issues before the fix goes out.)
Comment #5
tim.plunkettStill needs tests, but at least here's a patch that applies to D8.
Comment #6
berdirI'm starting to take these hook_file_download() security issues personal.
So this only happens for existing private style images, because we already fixed it for generating new ones including test coverage (which I partially wrote :p). That at least means adding test coverage is easy.
We already noticed in the previous issue that access checks for new and existing image styles are completely different and opened an issue about that. This again demonstrates nicely why that is a problem but we haven't really found a solution for it yet.
Comment #7
berdirForgot the test only patch.
Comment #9
berdir#6: grrrrrr-file-download-1890754-6.patch queued for re-testing.
Comment #10
berdir#7 failed as expected, #6 passed that test but failed with a random upgrade error. Re-testing.
Comment #11
berdir#6: grrrrrr-file-download-1890754-6.patch queued for re-testing.
Comment #12
webchick#7: grrrrrr-file-download-1890754-6-test-only.patch queued for re-testing.
Comment #14
webchickI get what's happening. :P I was re-testing the tests only patch. :P
Well it's failing. Hooray! ;) Back to needs review.
Comment #15
chx commented#6 is what went into D7 plus great tests.
Comment #16
webchickLovely.
Committed and pushed to 8.x. Now moving back to 7.x for the test coverage, and reducing to just a normal task for that.
Comment #17
berdirYay.
Here's a backport including a patch that reverts the fix to prove that the test works.
Comment #18
David_Rothstein commentedYeah, I guess it's normal priority, but still a D7 release blocker :) Moving to 7.21 because Drupal 7.20 was a security release only.
I think this looks good though, probably ready to go.
Comment #19
David_Rothstein commented#17: image-file-download-test-coverage-with-revert-1890754-17.patch queued for re-testing.
Comment #20
David_Rothstein commented#17: image-file-download-test-coverage-1890754-17.patch queued for re-testing.
Comment #21
David_Rothstein commentedI read over the surrounding test code and think this is RTBC provided it still passes/fails as expected after all the fun image style changes that happened recently in Drupal 7.20...
Comment #22
yannickooDavid can you tell me when #17 will be committed because this is the last release blocker? Or is there a date for 7.21 release?
Comment #23
David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/0a9cd35
@yannickoo, see http://groups.drupal.org/node/286418
Comment #24
David_Rothstein commentedFixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release.