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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Patch (to be ported)
FileSize
739 bytes

Attached is that patch that was committed to Drupal 7. This patch was authored by Heine.

pwolanin’s picture

Issue tags: +Security Advisory follow-up, +security.drupal.org

tagging

catch’s picture

Priority: Major » Critical
David_Rothstein’s picture

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

tim.plunkett’s picture

Status: Patch (to be ported) » Needs work
FileSize
759 bytes

Still needs tests, but at least here's a patch that applies to D8.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.72 KB

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

Berdir’s picture

Forgot the test only patch.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Security Advisory follow-up, -security.drupal.org, -Needs backport to D7

The last submitted patch, grrrrrr-file-download-1890754-6-test-only.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Security Advisory follow-up, +security.drupal.org, +Needs backport to D7
Berdir’s picture

#7 failed as expected, #6 passed that test but failed with a random upgrade error. Re-testing.

Berdir’s picture

Issue tags: -Needs tests, -Security Advisory follow-up, -security.drupal.org, -Needs backport to D7
webchick’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Security Advisory follow-up, +security.drupal.org, +Needs backport to D7

The last submitted patch, grrrrrr-file-download-1890754-6-test-only.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

I get what's happening. :P I was re-testing the tests only patch. :P

Well it's failing. Hooray! ;) Back to needs review.

chx’s picture

Status: Needs review » Reviewed & tested by the community

#6 is what went into D7 plus great tests.

webchick’s picture

Title: Private Images visible by url » [Tests] Private Images visible by url
Version: 8.x-dev » 7.x-dev
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Patch (to be ported)

Lovely.

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.

Berdir’s picture

Yay.

Here's a backport including a patch that reverts the fix to prove that the test works.

David_Rothstein’s picture

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

David_Rothstein’s picture

David_Rothstein’s picture

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

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

yannickoo’s picture

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

David_Rothstein’s picture

Title: [Tests] Private Images visible by url » Private Images visible by url
Category: task » bug
Priority: Normal » Critical
Status: Reviewed & tested by the community » Fixed
David_Rothstein’s picture

Issue tags: +7.22 release blocker

Fixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release.

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