File system set to private (and default to private).
Remove rights to view published content from all bar admin
Original image by "system" URL displays access denied. Great!
BUT the thumbnail, medium, large styles are all accessible via "system" URL to anonymous user....

Any help would be much appreciated.

CommentFileSizeAuthor
#26 fix-image-access-1507988-25.patch3.46 KBBerdir
#19 image-access-sa-tests-1507988-17-tests-only.patch4.64 KBchx
#17 image-access-sa-tests-1507988-17.patch5.49 KBBerdir
#17 image-access-sa-tests-1507988-17-tests-only.patch4.64 KBBerdir
#12 15079880-12.patch3.76 KBchx
#3 private-files-1507988-3.patch3.71 KBBerdir
#2 66538_28_no_cache_private_images-D7-do-not-test.patch1.99 KBwebchick
#2 62969-24_test_drupal_exit-D7-do-not-test.patch2.17 KBwebchick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JamesDevware’s picture

JamesDevware CreditAttribution: JamesDevware commented

It appears as if there is a problem within the image.module
function image_file_download($uri)

if i add this code

$pattern = '/(styles\/)(.*?)(\/private\/)/';
        $original_image_uri = preg_replace($pattern, '', $uri);
        if (!file_file_download($original_image_uri, 'image')) {
            exit;
        }

after this 

function image_file_download($uri) {
    $path = file_uri_target($uri);

    // Private file access for image style derivatives.
    if (strpos($path, 'styles/') === 0) {

Then i can make it produce a blank screen when anonymous user tries to access images by uri but authorised users can still access them as normal...

It appears as if something is wrong in the way that $original_uri is being formed so when file_file_download() is called it verifies the request for anyone.

I am relatively new to Drupal and unfortunately cannot figure out exactly what is wrong with it but hopefully what I have done can help somewhat.

Regards

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Private Image Styles are accessible by anon » SA-CORE-2012-002 - Access bypass - private images
Version: 7.12 » 8.x-dev
Issue tags: +Security improvements
FileSize
62969-24_test_drupal_exit-D7-do-not-test.patch2.17 KB
66538_28_no_cache_private_images-D7-do-not-test.patch1.99 KB

Ok, now that SA-CORE-2012-002 is out, we can re-open this, because we need to get the fix into D8 as well.

Here were the patches used by the sec. team for D7.

Commit credit should include netiva jak, frega, greggles, Xenza, chx, and Damien Tournoud.

Private tracker: #66538, #62969

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
private-files-1507988-3.patch3.71 KB

Ok, applied both patches to 8.x, he're a re-roll.

However, the tests confuse me a bit. There is a drupalLogout() in there but that doesn't make any sense because we never log in anywhere in that test? And I don't see where we'd do that in 7.x either.

So, the tests are currently failing. The new assertions however pass, which confuses me even more.

Status: Needs review » Needs work

The last submitted patch, private-files-1507988-3.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I diffed the image.test files 8.x and 7.x didn't notice anything obvious.

Is it possible that drupalLogout() in 7.x doesn't fail if you're not logged in and in 8.x it does?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Yes, that's totally the case. See how http://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dru... asserts the 200 response and http://api.drupal.org/api/drupal/modules%21simpletest%21drupal_web_test_... doesn't.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes, I also confirmed that with frega, the logout is wrong.

However, given that the logout there is wrong, I totally do not understand how these tests pass? That makes no sense. How can the first request be OK, then we do a fake logout, check again and now we're supposed to see an access denied?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

This is really wierd. If I do 

     $this->drupalGet($generate_url);
     $this->drupalGet($generate_url);

directly, one after the other, the first is a 200 and the second is a 403.

Radouch’s picture

Radouch CreditAttribution: Radouch commented

I am not sure if this is the right place to report it, but it seems this bug is not fixed in Drupal 7.14. I cannot test it in 7.13 but I have found this problem in version 7.14, see http://drupal.org/node/1594162

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

#1545930: drupalLogout() should only do one request, not two might be related? It wasn't backported to D7.

chx’s picture

chx CreditAttribution: chx commented
Assigned: Unassigned » chx
chx’s picture

chx CreditAttribution: chx commented
Assigned: chx » Unassigned
Status: Needs work » Needs review
FileSize
15079880-12.patch3.76 KB

I rerolled this. The testing needed small changes: I removed the logout call because there was no matching login and that caused a fail. The Cache-Control header is just no-cache, private, the test wanted it to be no-cache, must-revalidate, post-check=0, pre-check=0.StreamedResponse::prepare sets no-cache and file_get_content_headers() sets the private.

In general I do not understand anything that goes on with the test. How can the first $this->drupalGet($generate_url); return 200 while a second one at the same URL return a 403? Anyways, here's the reroll so someone else (or me later) can debug this further.

chx’s picture

chx CreditAttribution: chx commented

OK, so I debugged further, although #12 itself contains some of the answers: StreamedResponse::prepare sets no-cache. The call is $this->headers->set('Cache-Control', 'no-cache');. If you take a look at HeaderBag::set public function set($key, $values, $replace = true) this replaces. So this is why we have a no-cache and this where the Drupal headers are gone. Previously, you could see Drupal headers because we were sending them in a terminate subscriber, past everything but since the Symfony upgrade exposed the bugginess of that, we no longer do that so prepare() is now later than Drupal.

So: unless we have something against just private, no-cache Cache-Control headers, we are good.

As for the repeated drupalGet changing status that's because the test claims ownership only of the original file and image_style_deliver checks the access control of the derivative uri if it exists and the original if it doesn't.

With all this said , #12 might actually be correct.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The actual header sounds fine to me.

So the tests pass because on the first request, we generate the file and therefore have access and on the second request, it's already there and access to that is not granted to anonymous users? That's not exactly a behavior I'd want to build a test upon...

Can we make this more explicit somehow, by having two files and access to one is allowed and the other not? Took me a while to understand this and it's relatively fragile...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Working on improving the tests.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » Berdir
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
image-access-sa-tests-1507988-17-tests-only.patch4.64 KB
image-access-sa-tests-1507988-17.patch5.49 KB

Ok, I think this is better.

There are two changes in this patch.

1. I removed the explicit access denied from the image_module_test_file_download() implementation. The problem with this is that it prevents image.module from granting access to the derivate through the original file as it overrides it. Then the second request actually works just like the first one, as expected.

2. Now, to actually test that this is working as expected, I'm setting up a second image derivate and try to access that. This fails as expected.

Also, it looks like the symfony changes already resolved the actual bug here, the tests-only patches seem to pass as well for me.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The tests-only patch actually contains a few lines which aren't removed but it still passes without that as well. Not actually sure why, I guess because it gets overwritten somewhere?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community
FileSize
image-access-sa-tests-1507988-17-tests-only.patch4.64 KB

This is an unchanged patch from #17 which is ready. What we have concluded is that unlike drupal_access_denied() which returned to the caller in Drupal 7, the throw new AccessDeniedHttpException can not return to the place where it was thrown and so it's completely safe because on 403 the function is just over.

The fixed test is just nice because it actually tests a different file for access denied.

I am just reposting the patch to avoid confusing of which of the two in #17 is ready.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Security improvements

The last submitted patch, image-access-sa-tests-1507988-17-tests-only.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: +Security improvements

#19: image-access-sa-tests-1507988-17-tests-only.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Setting back to RTBC as per #19, not sure why the test failed the first time.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

YAY! Thanks a bunch. Committed and pushed to 8.x.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Active

I think we need to backport the test fixes to 7.x, working on it.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

created a follow up for the inconsistent access checks #1744136: Consistent and flexible derivative access check.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
fix-image-access-1507988-25.patch3.46 KB

Ok, here are proper tests for 7.x.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Same as D8, go ahead.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Yay! Committed and pushed to 7.x. Thanks!

spacereactor’s picture

spacereactor CreditAttribution: spacereactor commented
Status: Fixed » Needs work

i apply #26 patch and my private image styles is still able to be view by anonymous user.
I setup a private og group and private og content. Attach private image to the private og content.

Anonymous user unable to view this.
http://localhost/drupal7/system/files/image/photo1.jpg

But below image styles is being view by anonymous user.
http://localhost/drupal7/system/files/styles/thumbnail/private/image/pho...
http://localhost/drupal7/system/files/styles/medium/private/image/photo1...
http://localhost/drupal7/system/files/styles/large/private/image/photo1.jpg

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