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.

Files: 
CommentFileSizeAuthor
#17 image-file-download-test-coverage-with-revert-1890754-17.patch1.49 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 39,984 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#17 image-file-download-test-coverage-1890754-17.patch823 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,955 pass(es).
[ View ]
#7 grrrrrr-file-download-1890754-6-test-only.patch1005 bytesBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,959 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 grrrrrr-file-download-1890754-6.patch1.72 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,421 pass(es).
[ View ]
#5 image-1890754-5.patch759 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,247 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#1 private-files-1890754-D7.patch739 bytespwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch private-files-1890754-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Patch (to be ported)
StatusFileSize
new739 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch private-files-1890754-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Priority:Major» Critical

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

Status:Patch (to be ported)» Needs work
StatusFileSize
new759 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,247 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 49,421 pass(es).
[ View ]

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.

StatusFileSize
new1005 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,959 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

#6: grrrrrr-file-download-1890754-6.patch queued for re-testing.

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

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.

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.

Status:Needs review» Reviewed & tested by the community

#6 is what went into D7 plus great tests.

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new823 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,955 pass(es).
[ View ]
new1.49 KB
FAILED: [[SimpleTest]]: [MySQL] 39,984 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Yay.

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

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.

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

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?

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

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.