I found this because of #845838: File/Original image attached to private message does not display when using private files..

There seem to be two different issues...

First, it seems that the following line does not return anything for files/images that are attached to a private message:

$references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);

This might be related to #353458: hook_file_references() was not designed for a highly flexible field storage.

And if that would return something, http://api.drupal.org/api/function/file_file_download/7 only implements custom access checks if the entity this is attached to is either a node or a user. How are other entities (like private messages) supposed to handle this? Maybe some sort of callback so that the entity can tell if the user can view that piece of content?

Or are they supposed to implement hook_file_download themself? then file_file_download() is pretty useless and should be moved to node_file_download() and user_file_download() but that would lead a lot of duplicated code I guess. file.module should imho not have a hard dependency on node.module and user.module even if they are both required.

Files: 
CommentFileSizeAuthor
#69 file_get_file_references.patch550 bytesnonzero
PASSED: [[SimpleTest]]: [MySQL] 31,558 pass(es). View
#68 file.patch852 bytesneoglez
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es). View
#66 file.patch838 bytesneoglez
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_32.patch. View
#58 846296_taxonomy_files.patch667 bytesIsland Usurper
PASSED: [[SimpleTest]]: [MySQL] 31,553 pass(es). View
#46 file_download_access7.patch11.98 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,294 pass(es). View
#44 846296-file-download2.patch12.79 KBagentrickard
FAILED: [[SimpleTest]]: [MySQL] 23,242 pass(es), 2 fail(s), and 0 exception(es). View
#43 846296-file-download.patch10.35 KBagentrickard
FAILED: [[SimpleTest]]: [MySQL] 23,244 pass(es), 2 fail(s), and 0 exception(es). View
#41 file_download_access6.patch11.16 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,301 pass(es). View
#36 file_download_access5.patch6.6 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,957 pass(es). View
#33 file_download_access4.patch6.54 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,930 pass(es). View
#31 file_download_access3.patch6.71 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,959 pass(es). View
#28 file_download_access2.patch5.59 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,941 pass(es). View
#24 file_download_access.patch4.66 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 22,888 pass(es). View
#6 fix_bad_drupal_static_call_846296_6.patch784 bytesridgerunner
PASSED: [[SimpleTest]]: [MySQL] 22,141 pass(es). View
#7 fix_bad_drupal_static_call_846296_07.patch784 bytesridgerunner
PASSED: [[SimpleTest]]: [MySQL] 22,137 pass(es). View

Comments

moshe weitzman’s picture

I haven't researched this, but it sounds like field_access('view') should tell you for any entity if the file is viewable.

Berdir’s picture

Hm. In theory, I guess so. Thing is, field_access() just collects data from hook_field_access() and there are no implementations of that in core.

BenK’s picture

Just need to track this issue since I filed the original Private Message bug report....

tstoeckler’s picture

Shouldn't node.module pass on permissions from the node type to the attached fields via a node_field_access()?
And shouldn't user.module do the same, ie grant users with the 'view user profiles' permissions the field-level 'view' permission?
I actually have no clue, but I read the PHPDoc of the related functions, and, going from there, that would IMO make the most sense conceptually.

catch’s picture

Using field_access() then having hook_field_access() implementations would get around the hard coding of node and user modules, but it's going to add a fair bit of PHP overhead for every single field - 20 instances on a node, 10 nodes in a list, now you're running node_field_access() 200 times where previously nothing, and that overhead is going to be on sites without private files too.

ridgerunner’s picture

FileSize
784 bytes
PASSED: [[SimpleTest]]: [MySQL] 22,141 pass(es). View

I found what is probably a pretty nasty bug in file_get_file_references() where it is improperly calling drupal_static(). It is calling it like so...
$references = drupal_static(__FUNCTION__, array());
but it is supposed to call it with the =& reference operator like so...
$references = &drupal_static(__FUNCTION__, array());
I'm not familiar with the file end of the drupal code base here so I have no idea what the ramifications of this fix might be, but this may resolve this issue. Attached is a patch that corrects this error. Let's see if this fix passes automatic testing...

ridgerunner’s picture

FileSize
784 bytes
PASSED: [[SimpleTest]]: [MySQL] 22,137 pass(es). View

Not sure why the patch from the previous post did not get submitted for testing. Trying again with a different filename...

ridgerunner’s picture

Ok. Could someone please explain why the above two (identical except for filename) patches were ignored?

ridgerunner’s picture

Status: Active » Needs review

Ok, found the problem. Looks like the status needs to be "Needs Review" to trigger the bot...

Berdir’s picture

While that is a bug, it shouldn't lead to any actual bugs. It simply means that references are not statically cached but re-calculated every time they are being used.

So, while that is a correct fix, it is imho nothing that belongs to this issue.

maxikey’s picture

Status: Needs review » Closed (fixed)
sun’s picture

Status: Closed (fixed) » Closed (won't fix)
Berdir’s picture

Status: Closed (won't fix) » Active

Erm, what?

Why has this been won't fixed? In #10, I just said that the proposed patch doesn't have to do with the issue reported here.

Are modules that add fieldable entities supposed to define hook_file_download()? If so, then that needs to be documented and it needs to be done for comments and terms. Also, these modules would have to copy/paste 90% of file_file_download().

ridgerunner’s picture

Sorry for the interruption. Carry on.

aaronbauman’s picture

Here are (at least some of) the options then:

  1. Make the switch to hook_field_access(), node_field_access(), and user_field_access(); eat the performance hit
  2. Force developers to implement hook_file_download()
  3. adding: Create hook_file_download_access(); implement node_file_download_access() and user_file_download_access()
  4. and: Make "public/private download" control a field-level setting.
Damien Tournoud’s picture

As noted in the original post, file and image downloads just don't work currently. See #867928: Files and Images don't work in private filesystem mode.

Damien Tournoud’s picture

Making node and user implement hook_field_access() sounds like the way to go to me.

This said, it will have a performance hit, which proves that field access control is not performed at the correct place. It is currently processed field-by-field into field_default_view(), while it should probably be a multiple hook processed in field_attach_view(). One additional argument to make this change is that field_view_field() currently calls field_default_view(), so that fields displayed by other modules (for example: Views) will *also* be subjected to access control.

catch’s picture

Having node_field_access() and user_field_access() introduces that performance hit even for sites not using private files - that's a regression from D6 for all sites which don't use private files (which is still likely to be the majority even with the separation). Making it a multiple hook is a good idea to ameliorate this somewhat but that's a proper API change, whereas hook_file_download_access() wouldn't have any performance penalty, and is only an API addition. Either multiple hook_field_access() if it worked out alright or hook_file_download_access() I can live with though.

Berdir’s picture

I would vote for hook_file_download_access() too...

Two more questions:
- Should access default to TRUE or FALSE (in case there is no hook implementation for $entity_type)?
- Where should it be documented that this hook needs to be implemented for new entities? It took me a while until I figured out this based on the original bug report....

moshe weitzman’s picture

Since we have a module_implements cache, I don't really see a significant performance hit for adding a 100-200 (for the sake of argument) no-op function calls. I would prefer that to adding a new hook which is special for files. Not a huge deal either day.

moshe weitzman’s picture

I guess I prefer whatever patch anyone comes up with to close the critical. Keep existing hook or make a new one.

moshe weitzman’s picture

Re: #19

1. default to true (allow access)
2. best i can find is http://api.drupal.org/api/function/hook_entity_info/7

At #851878: serve image derivatives from the same url they are generated from, justin notes that image derivitives of private files are not currently access controlled. Might be fixable here.

Gábor Hojtsy’s picture

This also looks like THE place/function to solve 1/4th of #881578: Solve SA-CORE-2010-002 issues, i.e., the "File download access bypass". Looks like the files list returned could be multiples and the upper/lower case trick also applies. I'll submit a patch over there, just cross-referencing.

Berdir’s picture

Status: Active » Needs review
FileSize
4.66 KB
PASSED: [[SimpleTest]]: [MySQL] 22,888 pass(es). View

Ok, attaching first patch. This is not perfect yet, but works...

file_file_download() currently checks field_access() and hook_file_download_access(). If for all entities, where a file is used, either of those returns FALSE, access is denied.

I had a lot of ideas/questions while writing this:
- Still not sure if hook_file_download_access() is necessary or not. Because of the current implementation, we could also simply change the hook implementations in user.module and node.module to hook_field_access() and remove the hook_file_download_access() and it would work as well. But that would probably require rewriting that hook to support multiple fields for performance reasons. I don't know if I can do that.

- If we keep the hook, we could need a better name...

- If we keep the hook, we need to think about the arguments passed to it (currently $field, $entity_type and $entity)

- Another idea I had to solve this, but this is most probably D8 stuff: Add access permission/callback to hook_entity_info() and with that, allow a generic entity_access($entity). Because all implementations of hook_file_download_access() only check access to $entity, they don't care about $field (that's what hook_field_access is for, after all). But I think that would introduce too many changes. That would also be self-documenting, not like hook_file_download_access(). And that's als why I'm not sure about the name and arguments for hook_file_download_access().

- I haven't even considered writing documentation for hook_file_download_access(), as it might not survive the next patch :)

BTW, I verified that this also works for privatemsg.module, I've implemented and tested the following hook implementation (Only allows access if you have read all permission or are a recipient of the message the file belongs to):


/**
 * Implements hook_file_download_acces().
 */
function privatemsg_file_download_access($field, $entity_type, $entity) {
  global $user;

  if ($entity_type == 'privatemsg_message') {
    // Users with read all private messages permission can view all files too.
    if (user_access('read all private messages')) {
      return TRUE;
    }

    // Check if user is a recipient of this message.
    return (bool)db_query_range("SELECT 1 FROM {pm_index} WHERE recipient = :uid AND type IN ('user', 'hidden') AND mid = :mid", 0, 1, array(':uid' => $user->uid, ':mid' => $entity->mid))->fetchField();
  }
}
moshe weitzman’s picture

Code and approach are sound IMO.

- If you have implement multiple in hook_field_access, thats ideal. But I am happy to go with this patch approach as well.

- current hook name is fine

- those args look good to me

- agree, d8 stuff

- thanks for including privatemsg example code. it is helpful when considering the patch.

Berdir’s picture

- Ill try to have a look at the multiple stuff.

Another question, what about other core entities? Do they need a hook implementation too?
- comments: "user_access('access comments') && $comment->status || user_access('administer comments')"?
- terms: Doesn't look like there's a view permission defined in http://api.drupal.org/api/function/taxonomy_permission/7?

moshe weitzman’s picture

I do think that comment logic is right and belongs in this hook. As you said, there is no protection currently for term viewing.

Berdir’s picture

FileSize
5.59 KB
PASSED: [[SimpleTest]]: [MySQL] 22,941 pass(es). View

In the sense of http://cyrve.com/node/23, here is a new patch...

Changes
- Implemented comment_file_download_access() and did some very basic tests
- Tried to improve the comments in file_file_download()
- If file_file_download() fails to load $entity or $field, it denies access (for that reference only). This actually shouldn't happen because a valid reference to that was returned.

I thought about changing hook_field_access() but I'm not even sure how that would work then (should it return an array with access definitions, or should we pass the fields by reference and define #access, ... ) and changing field api to be able to do that is certainly even more complex :) I agree that it would be the perfect solution but I also agree with moshe that it is too late for finding the perfect solution :)

If someone can come up with a working patch, great, but I don't have the time nor knowledge to do that...

And an additional question:
- A file is used in two entities. One does deny access and the second does not define any access checks? Currently, that would mean that access is denied, because $denied has been set to TRUE. What should happen in this case? Allow download? Not sure how to implement that.... I guess we would need to reverse the logic and make it like this "Allow access unless all entities deny access"...

Still todo: hook_file_download_access() documentation

moshe weitzman’s picture

About your 'additional question': I agree with the behavior you have coded.

Code looks ready save for the the doxygen you mentioned.

No tests in this patch. You mentioned those in #28

moshe weitzman’s picture

Perhaps the comment module hunk needs minor change since #881578: Solve SA-CORE-2010-002 issues was committed.

Berdir’s picture

FileSize
6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 22,959 pass(es). View

- Added documentation for hook_file_download_access().
- Made the status check for comment access a bit more explicit by comparing it to COMMENT_PUBLISHED instead of just a non-zero integer but other than that, the security issue is imho not really related as this already had a status check and the other issue was about edit access...

As you said, no tests yet. We do have a very basic test of a private file on a node, but that's it... Maybe I'll find the time to write some tests for this at the core summit :) What does need to be tested here? A deny/allow test for each entity? Combinations with multiple references would be nice I guess, but I'm not sure how to create that.. And obviously no references but again, I'm not sure how to create that... any pointers?

moshe weitzman’s picture

Status: Needs review » Needs work

I'm satisfied with the existing test on this. We're just down to doxygen tweaks

+ * Note that hook_field_access() should be implemented instead to provide
+ * global access restrictions for fields even though $field is passed to this
+ * hook as well.

Not sure what we mean to convey here. Perhaps omit it?

If a reference denies access, eventually existing additional references are checked.

This seems to imply that we are not short circuiting a denial when the code says that we are. Or I am missing the meaning.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.54 KB
PASSED: [[SimpleTest]]: [MySQL] 22,930 pass(es). View

I'm fine with omitting that :)

We only short circuit an allow. For example, if you are a user that can view nodes but not user profiles and the a file is attached to a node and a user profile, you are allowed to download that file even though the reference to the user profile will deny access and it shouldn't matter which reference is checked first.

The following cases allow download:
- At least a single reference explicitly allows access, even if another references denies
- No reference allows nor denies access

That's why I'm not 100% about the default value and the handling when one references denies access and another one does nothing.

Re-roll with the first thing removed.

moshe weitzman’s picture

OK, that logic makes sense.

Now that I think of it, I think we should add a drupal_alter('file_download_access', $grants) after our first loop. This gives flexibility to site specific modules to get the behavior they want. This is what we added for node access (see http://api.drupal.org/api/function/node_access_acquire_grants/7) so we should do same here. This does mean that we can't short circuit anymore. Not a big loss.

Berdir’s picture

Assigned: Unassigned » Berdir
Berdir’s picture

FileSize
6.6 KB
PASSED: [[SimpleTest]]: [MySQL] 22,957 pass(es). View

Ah, I think we talked about two different things regarding short-circuit... :)

There are two levels here:

- Whole thing: That's what I talked about. We short-circuit ALLOW in the sense that if an reference allows access, we allow access to the file.

- Per reference: I guess that's what you meant. The old patch did short-circuit ALLOW and DENY for the current reference only. The patch changes this and doesn't, *but* (not sure hot how to explain it), ALLOW is strong than DENY, so TRUE is checked first.

What changed:
As described above, this patch calls all hook_file_download_access() implementations, then calls the alter hook and then checks if there is a TRUE in the array (short-curcuit here!) and last, checks if there is a FALSE (sets denied to FALSE but doesn't short-curcuit the whole thing).

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK, thats exactly what I meant. RTBC. I'll summarize.

If you add a file or image Field to comments or terms or a custom entity, you currently have no good way to deny to the file if the user has no access to view that entity. Here is example code for privatemsg which uses entities for its messages in D7:


/**
* Implements hook_file_download_acces().
*/
function privatemsg_file_download_access($field, $entity_type, $entity) {
  global $user;

  if ($entity_type == 'privatemsg_message') {
    // Users with read all private messages permission can view all files too.
    if (user_access('read all private messages')) {
      return TRUE;
    }

    // Check if user is a recipient of this message.
    return (bool)db_query_range("SELECT 1 FROM {pm_index} WHERE recipient = :uid AND type IN ('user', 'hidden') AND mid = :mid", 0, 1, array(':uid' => $user->uid, ':mid' => $entity->mid))->fetchField();
  }
}

webchick’s picture

Issue tags: +Needs tests, +API change

Coming to you LIVE from Drupalcon CPH!

Reviewed this patch with Dries, and we both agreed it looks good. But it seems we need expanded test coverage for the original bug? Something like adding a file field to a comment and verifying that access carries over properly.

With that change, looks like this is good to go.

This is also an API change, so tagging accordingly.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
webchick’s picture

Oh. We also need docs for hook_file_download_access_alter(), too.

I do like the parity here with the node access system. +1.

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.16 KB
PASSED: [[SimpleTest]]: [MySQL] 23,301 pass(es). View

Tests++ ;)

Added a new test, that does..

- Add a file field to comment on article node type
- remove access comments permission from anon
- create a node and comment with a file
- download file as user with access
- try to download file as anon user without access

Also added documentation for hook_file_download_access_alter()

moshe weitzman’s picture

Status: Needs review » Needs work

great. just found one typo: authenticed

agentrickard’s picture

Status: Needs work » Needs review
FileSize
10.35 KB
FAILED: [[SimpleTest]]: [MySQL] 23,244 pass(es), 2 fail(s), and 0 exception(es). View

I found a bunch of problems, discussed them to Joakim, Berdir and webchick. New patch attached. Summary:

-- Added return documentation for hook_file_download_access().
-- Added documentation for hook_file_download_access_alter().
-- Made the $grants array passed to the alter hook contain some meaningful context. As written before, the alter hook was passed a positional array of Boolean values. Not enough context to 'alter'. Now the $grants array is keyed by the module that controls the entity.
-- Default access set to FALSE (assigned to system module), so that at least one entity must assert TRUE for access to be granted.

Berdir's tests are incorporated here.

agentrickard’s picture

FileSize
12.79 KB
FAILED: [[SimpleTest]]: [MySQL] 23,242 pass(es), 2 fail(s), and 0 exception(es). View

Forgot that file.api.php is a new file.

Status: Needs review » Needs work

The last submitted patch, 846296-file-download2.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.98 KB
PASSED: [[SimpleTest]]: [MySQL] 23,294 pass(es). View

There was a dsm() left over in the previous patch, removed that.. let's see if this passes..

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK, I think we are ready.

@agentrickard - nice catch in #43 with extra context in hook and change to $grants deny logic.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work, folks! Committed to HEAD.

Randy, the summary of this API change is in #57 (but cross-reference with the example docs in file.api.php because I think the function signature changed in the latest patch).

agentrickard’s picture

Oh man, the dsm() made poor Andrew re-test the patch all morning. D'oh!

rfay’s picture

OK, looking for the issue and API change summary. Not finding it in #57 :-) Can somebody point me to it or write it?

Thanks!

agentrickard’s picture

#37 and #43 have the revelant features. There is also a new file.api.inc that explains the new hooks,

Berdir’s picture

Priority: Critical » Normal
Status: Fixed » Needs work

I think we have some inconsistencies between the comments and the actual code due to the changes added by agentrickard (which are fine, just the comments aren't correct anymore). Also, it looks like the code could be simplified a bit.

This is the problematic part:

+++ modules/file/file.module
@@ -141,46 +141,69 @@ function file_file_download($uri, $field_type = 'file') {
+  // Default to allow access.
+  $denied = FALSE;
+  // Loop through all references of this file. If a reference explicitly allows
+  // access to the field to which this file belongs, no further checks are done
+  // and download access is granted. If a reference denies access, eventually
+  // existing additional references are checked. If all references were checked
+  // and no reference denied access, access is granted as well. If at least one
+  // reference denied access, access is denied.

Because of the system => FALSE change, that is not true anymore. Instead, it will deny access if no module allows access and there are references. *Except* someone actually implements an alter hook to remove the system => FALSE default for whatever reason. That special case is also the only case when the default value would be used because #898036: Private images broken (@agentrickard: remember that the private image field didn't work when we tested the patch? That issue fixes that... ) is going to add an empty($references) check and returns early.

Powered by Dreditor.

sun’s picture

Looks like there's no longer an API change here, just needs a documentation fix.

jhodgdon’s picture

Can someone clarify what documentation needs to be fixed? Webchick mentioned a summary above, but she said it was on comment #57, which doesn't exist (and neither does #47).

And does this change need to be put into the module 6/7 upgrade guide? If so, please tag "Needs Update Documentation". Thanks.

Island Usurper’s picture

Issue tags: -Needs Documentation

I think we still need a taxonomy_file_download_access() for files attached to terms and vocabularies. Try adding an image field from the private filesystem to a term. If there are other field data visible, you should see a broken image. Since there's an alter hook already, just returning TRUE when the $entity_type is 'taxonomy_term' should be enough for core.

rfay’s picture

Issue tags: +Needs Documentation

Looks like an x-post cleared the Needs Documentation tag.

Island Usurper’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
667 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,553 pass(es). View

3 weeks is a long time for a cross-post. Sorry, but I meant to take it off because I wanted to indicate that we need some more code. I guess what we really need are more tests.

johnv’s picture

Status: Needs review » Needs work

I encountered the error as described in #56 and posted this issue: Private files broken for Filefield/Imagefield in Taxonomy Term.
Then I found this issue and I applied the patch in #58, but with no results. Do I need to apply more patches?

Berdir’s picture

Have you cleared the cache after applying the patch?

johnv’s picture

Status: Needs work » Reviewed & tested by the community

That has become a second nature of me, but in this case, I didn't. :-( Thanks for the tip!
Works, like a charm, and I hope it gets in a next -dev version! --> RTBC

Akaoni’s picture

Version: 7.x-dev » 7.0
Status: Reviewed & tested by the community » Needs review

There seems to be an issue with this in the Drupal 7 release code.

/modules/file/file.module Line 143

  // Find out which (if any) fields of this type contain the file.
  $references = file_get_file_references($file, NULL, FIELD_LOAD_CURRENT, $field_type);

  // If there are no references, stop processing, to avoid returning headers
  // for files controlled by other modules.
  if (empty($references)) {
    return;
  }

The functionality spelt out in the commments is sound.
Problem is, if a field of $field_type is defined but doesn't contain a reference to the file what's returned isn't empty().

Example return:

array(2) {
  ["field_file"]=>
  array(0) {
  }
  ["field_file2"]=>
  array(0) {
  }
}

This means that files where there is no reference node (or the reference node isn't accessible to the user) are always accessible.

I haven't got GIT setup yet otherwise I'd submit a patch. :(

jhodgdon’s picture

Version: 7.0 » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Right. This issue has not been marked "fixed", because it hasn't been fixed. Please leave the version as 7.x-dev, since it hasn't been fixed in the development version, much less the released code.

And there is already a patch in #58, and in fact it has already been reviewed. Please don't change the status.

jhodgdon’s picture

#58: 846296_taxonomy_files.patch queued for re-testing.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

@jhodgon The existing patch addresses a different problem (And should probably be in a different issue, since it's really about something additional based on the new API added in this issue), #62 is correct (as in: the (my :() code is wrong and needs to be fixed), so moving this back to needs work.

@Akaoni: would be great if you can create a patch, git isn't that hard to install :) And you can also create a patch with any diff utility, you just need to make sure that you are diffing from the top-level directory and changed file has the correct name ("modules/file/file.module") so that the test bot can apply the patch.

neoglez’s picture

Version: 7.x-dev » 7.0
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
838 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_32.patch. View

Hi!
I've been fighting with this for a couple of days already. I must say that i consider this a very (security?) seriousone. I did extenses tests and i came up with the following clonclusion:
As long as the file access is managed by Drupal core functionalies (no contributed AC module acting over node_access table) the behavior is as expected but in the presence of such a AC module the query that is constructed in file_get_file_references:

      $query = new EntityFieldQuery();
      $query
        ->fieldCondition($file_field, 'fid', $file->fid)
        ->age($age);
      $references[$field_name] = $query->execute();

returns populated arrays (by reference, the query is executed as many times as fild instances of the type 'file' are) of only nodes the user can access, wich IMO shouldn't be becouse then we are NOT going to have the posibility of calling the hook_file_download_access or _alter for files attached to that node and according with the documentation that is exactly what this hook is for:

Control download access to files.

The hook is typically implemented to limit access based on the entity the file is referenced, e.g., only users with access to a node should be allowed to download files attached to that node.

On the other hand it seems that for other types of entities than nodes, it should work fine, the following is an example of the query executed by file_get_file_references on a node with a file attached and in the presence of the Tac Lite (AC module) (note the field_data_field_datei0.entity_type <> 'node'):

SELECT DISTINCT field_data_field_datei0.entity_type AS entity_type, field_data_field_datei0.entity_id AS entity_id, field_data_field_datei0.revision_id AS revision_id, field_data_field_datei0.bundle AS bundle
FROM 
field_data_field_datei field_data_field_datei0
LEFT OUTER JOIN node_access na ON na.nid = field_data_field_datei0.entity_id
WHERE  (field_data_field_datei0.field_datei_fid = '208') AND (field_data_field_datei0.deleted = '0') AND(((( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '0') AND (na.realm = 'tac_lite') ))AND (na.grant_view >= '1') AND (field_data_field_datei0.entity_type = 'node') )OR (field_data_field_datei0.entity_type <> 'node') )

Here we see that the query checks (when the entity is 'node') the access on the node to return the result, delivering an empty array and therefore bannig file_file_download to call the corresponding hooks.
I'm posting a patch to resolve the issue when the $references array is an array of empty values but i really think references should always be returned and not only when the entities they are attatched to are accessible by the user.
The patch is against the 7.0 -> changing Version and Status to schedule tha patch test.
I consider this major becouse it limits the posibility to use contributed AC modules, exposing the private files attached to a nodes, even when nodes are not accessible.

Status: Needs review » Needs work

The last submitted patch, file.patch, failed testing.

neoglez’s picture

Status: Needs work » Needs review
FileSize
852 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es). View

...Ups, i'm sorry, it's the first time i'm posting a patch..

nonzero’s picture

FileSize
550 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,558 pass(es). View

Hm, this seems similar to what I've been doing. I haven't compared your patch with mine in detail yet though. Just throwing this out there for more eyes to see.

The problem is that file.module :: file_get_file_references() sets $references[$field_name] to an empty array if the EntityFieldQuery fails to find the file. EntityFieldQuery returns an empty array because it actually does honor node_access and finds the file restricted. Unfortunately, setting $references (even to an empty array) leads the calling function, file_file_download(), to try to traverse the empty array looking for access denial. Since it's empty, the function assumes access is granted. Attached is my patch to fix the problem by checking if the EntityFieldQuery returned an empty array.

nonzero’s picture

It appears that my patch tackles the bug more upstream, which is good if code elsewhere uses file_get_file_references(), whereas #68 only works within file_file_download().

neoglez’s picture

@nonzero I DO really think that would be a nice code to patch file.module (it adresses a more general issue) but the question is:
Should file_get_file_references() honor node_access or other access call on the entities, files are attached to ??
V.S.
Should implement file access checks regardless of node_access or other entities' access check??
As long as i understand the answer is affirmative to the second question (see the blockquote i posted) and in that case neither your nor my patch adresses the issue, the query is just wrong constructed.
I repeat:
references should always be returned and not only when the entities they are attatched to are accessible by the user.
It's just a matter of Business Logic.
This issue is assigned to Berdir, opinions from him (and the community of course) are wellcome.

Berdir’s picture

Assigned: Berdir » Unassigned

I'm assigned because I worked on this during drupalcon cph, removing... :)

Yes, that is the question that needs to be answered, I'm not sure.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

@neoglez

I disagree, simply because file_get_file_references() is merely a helper function for file_file_download() and is designed to force an access check. No other core functions call this code.

I think you want a new function or query to return all files attached to an entity, which is valid but not required in this context.

Otherwise, the solution would have to be an API change that passes file information and access information back to the caller.

Marking #69 RTBC.

rfay’s picture

Version: 7.0 » 8.x-dev
Status: Reviewed & tested by the community » Needs review

D8 first. Let's get these fixed on D8 and into D7.

catch’s picture

Status: Needs review » Closed (duplicate)

This is a duplicate of [#1168756] afaik.

David_Rothstein’s picture

Issue summary: View changes
David_Rothstein’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (duplicate) » Fixed

This issue was unpublished a few years ago, because the followups discussed here touched on a security issue. However, that security issue was fixed a long time ago in SA-CORE-2011-001. I'm republishing it now because I want to refer to it in #2305017: Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved, which relates to some of the code that was originally added here.

I'm also moving this issue to "Fixed" because the original patch here was committed to Drupal 7 a long time ago. Besides the security issue, the two other followups I see are:

  1. Possible missing implementation of hook_file_download_access() in the Taxonomy module. This is a separate issue at #1327224: Access denied to taxonomy term image.
  2. Inconsistency between the code and code comments (#53). However, that ties directly into #2305017: Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved, so we can continue that discussion there. Basically, at first glance it looks to me like the code is doing the wrong thing (not the code comments), but I need more feedback on that issue.

Status: Fixed » Closed (fixed)

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