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.

Files: 
CommentFileSizeAuthor
#26 fix-image-access-1507988-25.patch3.46 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,349 pass(es).
[ View ]
#19 image-access-sa-tests-1507988-17-tests-only.patch4.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,248 pass(es).
[ View ]
#17 image-access-sa-tests-1507988-17.patch5.49 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,271 pass(es).
[ View ]
#17 image-access-sa-tests-1507988-17-tests-only.patch4.64 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,262 pass(es).
[ View ]
#12 15079880-12.patch3.76 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,127 pass(es).
[ View ]
#3 private-files-1507988-3.patch3.71 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 36,517 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#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

Comments

Xenza’s picture

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

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
StatusFileSize
new2.17 KB
new1.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

Status:Active» Needs review
StatusFileSize
new3.71 KB
FAILED: [[SimpleTest]]: [MySQL] 36,517 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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

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

Berdir’s picture

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

This is really wierd. If I do

<?php
     $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

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

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

chx’s picture

Assigned:Unassigned» chx
chx’s picture

Assigned:chx» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.76 KB
PASSED: [[SimpleTest]]: [MySQL] 40,127 pass(es).
[ View ]

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

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

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

Working on improving the tests.

Berdir’s picture

Assigned:Unassigned» Berdir
Berdir’s picture

StatusFileSize
new4.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,262 pass(es).
[ View ]
new5.49 KB
PASSED: [[SimpleTest]]: [MySQL] 40,271 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new4.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,248 pass(es).
[ View ]

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

Status:Needs work» Needs review
Issue tags:+Security improvements
Berdir’s picture

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

Status:Reviewed & tested by the community» Fixed

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

Berdir’s picture

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.

beejeebus’s picture

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

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new3.46 KB
PASSED: [[SimpleTest]]: [MySQL] 39,349 pass(es).
[ View ]

Ok, here are proper tests for 7.x.

chx’s picture

Status:Needs review» Reviewed & tested by the community

Same as D8, go ahead.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

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