See https://www.drupal.org/SA-CORE-2014-003

Private file access is based on access to the entity that references the file (where it's attached).

User's can insert a File ID in Drupal 8 and bypass the access.

In Drupal 8 we need to consider both REST and form widgets, so we need to add a constraint that applies to both those cases, rather than just a form API fix liek in Drupal 7.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Berdir’s picture

cilefen’s picture

Status: Active » Needs review

Correct me if wrong, but it seems D8 is already safe against this.

Drupal\system->FileDownloadController() seems to already respect the access check.

      foreach ($headers as $result) {
        if ($result == -1) {
          throw new AccessDeniedHttpException();
        }
      }

The main implementation, file_file_download(), returns -1 appropriately.

Status: Needs review » Needs work

The last submitted patch, sdo-62404-private-fid-56-D7.patch, failed testing.

Devin Carlson’s picture

Assigned: Unassigned » Devin Carlson
Status: Needs work » Needs review
FileSize
1.53 KB

Taking a look at this. From my testing it looks like there is still an issue, though it may have changed slightly as mentioned in #4.

Status: Needs review » Needs work

The last submitted patch, 6: private-file-access-bypass-2304969-6-tests-only.patch, failed testing.

catch’s picture

Issue tags: +D8 upgrade path, +beta target

Tagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.

pwolanin’s picture

So, the user should not be able to see the file even though the admin attached it?

pwolanin’s picture

Discussing with berdir - we don't even understand why it works (or if it does) in Drupal 7, but we need to make changes to 8 to flag each entity reference item as having access or not.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.02 KB

This was fun.

We found yet another bug in file_get_file_references(). It was incorrectly assuming that it does not have to check if an entity/field actually has the file referenced when we do not check by reference. But then we also don't skip fields that don't have the file.

So we were returning both fields, and then check access on both, and it was granted on the second, so we were granting access. That was obviously wrong, but the test at some point was completely broken and when it got fixed, worked around this problem by switching to a different user that probably doesn't have access to the node.

This gets us to the point where we can actually fix this problem.

pwolanin’s picture

After discussion with fago, berdir, and klausi about how to handle this for REST as well as form APi, we need to add the fix to ValidReferenceConstraintValidator not just to the form like we did in Drupal 7.

Looking at that class we have:

public function validate($value, Constraint $constraint)

$value is a field item.

$value->getEntity() is the current entity (the one in the process of being saved)

So we need to load the original one. Find all NEW reference values and for new values, validate that the current user has access to view the newly referenced entities.

See: EntityChangedConstraintValidator for example of loading the original entity.

pwolanin’s picture

FileSize
1.53 KB
4.3 KB

Here's a small test fix on top to prep for real fixes.

berdir noticed that the view mode was not set up for the file field, so it was never rendered.

Status: Needs review » Needs work

The last submitted patch, 13: 2304969-13.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 13: 2304969-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2304969-13.patch, failed testing.

Status: Needs work » Needs review

Berdir queued 13: 2304969-13.patch for re-testing.

pwolanin’s picture

FileSize
1.33 KB
5.11 KB

Adds new test coverage - this demonstrates the bug that we need to fix and expect 2 fails.

Status: Needs review » Needs work

The last submitted patch, 18: 2304969-18.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
11.57 KB
14.54 KB

ok, this adds the constraint hopefully the access check logic is right.

Status: Needs review » Needs work

The last submitted patch, 20: 2304969-20.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.66 KB
15.7 KB

Ok, let's make this more focused and just apply the access constraint to the file item.

pwolanin’s picture

FileSize
1.14 KB
15.7 KB

oops - forgot to fix the private file test

Fabianx’s picture

Needs an issue summary update

pwolanin’s picture

Assigned: Devin Carlson » Unassigned
Issue summary: View changes
pwolanin’s picture

pwolanin’s picture

FileSize
15.7 KB

re-posting patch - the above is green, but not reporting back.

Berdir’s picture

That won't help, there are some communication issues between drupal.org and qa.drupal.org. So it won't send the new patch. But it will probably catch up again at some point.

The last submitted patch, 22: 2304969-22.patch, failed testing.

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReferenceAccessConstraintValidator.php
    @@ -0,0 +1,56 @@
    +           if (isset($referenced_entities[$ref->id()])) {
    +             debug($ref->id());
    +             $check_permission = FALSE;
    

    As I am not familiar with that function, I assume debug is not an oversight here, but intentional.

  2. +++ b/core/modules/file/file.module
    @@ -1852,22 +1847,18 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
    +          // Iterate over the field items to find the referenced file and field
    +          // name. This will fail if the usage checked is in a/ non-current
    +          // revision because field items are from the current
    

    nit - a/

  3. +++ b/core/modules/file/src/Tests/FilePrivateTest.php
    @@ -38,15 +41,14 @@ function testPrivateFile() {
    +    \Drupal::entityManager()->getStorage('node')->resetCache(array($nid));
    +    $node = Node::load($nid);;
    

    Should this not use ->loadUncached() as in the above code - that was added.

Besides those questions and nits, this is RTBC.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Novice
  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReferenceAccessConstraintValidator.php
    @@ -0,0 +1,56 @@
    +    $id = $value->get('target_id')->getValue();
    

    Can be simplified to $value->target_id.

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReferenceAccessConstraintValidator.php
    @@ -0,0 +1,56 @@
    +    $referenced_entity = $value->get('entity')->getValue();
    

    Same, $value->entity.

    Does not make a different for computed properties like entity here, but it is optimized for actual properties like target_id to not create a typed data object just to get the value out again.

  3. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
    @@ -40,7 +40,8 @@
    + *   constraints = {"ValidReference" = {}}
    

    ImageItem should have the access check constraint too, those are files too and you could reproduce this with an image field too.

#30.1: No it is not, should be removed.
#30.3: Doesn't really matter, loadUnchanged() is a shortcut for the same code, both work. Tests are full of this pattern.

Tagging novice for re-roll and addressing those minors things.

cilefen’s picture

cilefen’s picture

Sorry, missed the point of the novice tag on this one.

Status: Needs review » Needs work

The last submitted patch, 32: port_private_files-2304969-32.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
15.65 KB
Berdir’s picture

Issue tags: -Novice

Oh, don't worry, I only added that because it usually helps to add that tag so that someone picks up the issue. I don't care if it is actually a novice or not :) Thanks for the updates.

Changes look good to me. A lot of the code is based on my input, so I would prefer if someone else RTBC's it, but I'm +1 on doing so.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch and everything looks good. The logic is not easy to follow in a couple of places, but the variable names are reasonably verbose to understand what is going on.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8f3f79a and pushed to 8.0.x. Thanks!

diff --git a/core/modules/file/src/Tests/FilePrivateTest.php b/core/modules/file/src/Tests/FilePrivateTest.php
index f9b8413..3e69fe4 100644
--- a/core/modules/file/src/Tests/FilePrivateTest.php
+++ b/core/modules/file/src/Tests/FilePrivateTest.php
@@ -7,10 +7,10 @@
 
 namespace Drupal\file\Tests;
 
+use Drupal\Core\Entity\Plugin\Validation\Constraint\ReferenceAccessConstraint;
 use Drupal\Component\Utility\String;
 use Drupal\file\Entity\File;
 use Drupal\node\Entity\Node;
-use \Drupal\Core\Entity\Plugin\Validation\Constraint\ReferenceAccessConstraint;
 
 /**
  * Uploads a test to a private node and checks access.

Small amount of clean up on commit.

  • alexpott committed 8f3f79a on 8.0.x
    Issue #2304969 by pwolanin, cilefen, Berdir, Devin Carlson, klausi:...

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture