Private files that are no longer attached to an entity should not suddenly become accessible to people who couldn't see them before

Yowch. Seems like something we need to fix before the upgrade path is supported since I imagine this would be an SA if it did impact D7 core.

In file_file_download(), if a file has no references left, it doesn't explicitly check access and allows other modules to chime in. That's why this happens.

Hm. I understand that the change for D7 might be a problem, but I'm not sure that it's just by design for D8.

As you said, you need a module that actually grants access, like "View *private* files". Which is defined as an admin permission:

    'view private files' => array(
      'title' => t('View private files'),
      'restrict access' => TRUE,
    ),

I would actually consider it to be strange/a bug that I can't access private files despite having that permission? What's the point of that permission then?

And your hook explicitly grants access to *all* files. Again, not very surprising?

That said, I do see that point that is unexpected when access was denied before and then it no longer is. But as mentioned above, in those scenarios, I'm not actually 100% sure that' desired? It might be a logical next step to make files their own thing and not just attachments to other entities that have no existence beyond that, to not deny access anymore? So maybe instead of trying to find workarounds to force denying access to those fields, maybe we should instead make a step forward and remove the -1. That means other modules need to consider private files properly, but again, access code always need to be correct or it is an issue.

Authenticated users are supposed to be able to download a certain class of files (let's say PDF files) that are not necessarily attached to other entities. It might be a library of browsable, donwloadable documents, for example. You need some code that will explicitly grant them that access, because otherwise nothing else will give them access in the case of an unattached file.

Any module that is granting access to all private files that is by extension or any other criteria other than registered records in the database would likely open up security vulnerabilities (as described a couple of different ways above) intentionally is granting access to a file with limited knowledge of the file usage (which generally is a bad idea). If the file is a "stand-alone", a file_usage record should still be registered by the module that provides the stand-alone capability (e.g. File Entity in D7).

Personally I think Drupal should ideally distinguish between files that are intended to be "only attachments" and files that are intended to be able to "stand on their own".

I think this might be a good idea as well, but in the mean time, any file that stands on its own should simply point a file_usage record at the file entity, ensuring the file never is deleted because it's always "in-use" by its own entity (unless that file entity itself is deleted).

So far from all the examples given, they all seem like any problems would be derived from a module implementing hook_file_download() and returning access when they didn't have enough information to be making such a judgement.

I think @Berdir is correct that the thing we're missing here is consistency in granting access to a file. File module should probably *not* return a -1 access for files that may have been attached to a node but to which the user does not have access. If another module wants to grant access to that file (because it may have originally been uploaded through a file browser or some other means), then that module should be allowed to grant access.

EDIT: Amended my statement about causing security vulnerabilities (it doesn't by itself).

As per hook_file_download:

 * @return
 *   If the user does not have permission to access the file, return -1. If the
 *   user has permission, return an array with the appropriate headers. If the
 *   file is not controlled by the current module, the return value should be
 *   NULL.

So is the idea that file_file_download() would

  if (!$file->access('download')) {
    return NULL;
  }

Instead of -1? That would mandate that all other file download hooks would need to do this access check themselves then?

Not sure how would that resolve the problem, given the "problem" is with the lines above that return:

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

  // Stop processing if there are no references in order to avoid returning
  // headers for files controlled by other modules. Make an exception for
  // temporary files where the host entity has not yet been saved (for example,
  // an image preview on a node/add form) in which case, allow download by the
  // file's owner.
  if (empty($references) && ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id())) {
    return;
  }

Whatever is returned in the following lines, this will already return if there are no references left. While its true that a contrib module that manages a file directory should maintain usage records for the files, core already allows in Drupal 8 to keep all files (without usage records). And I don't think we want to keep a usage record for each file referencing itself, because then no file would be ever deleted. Maybe we can keep a usage record for each file if the site is set to keep all files globally, but then we need to update all files when that setting is changed, so the setting change actually results in files removed later. And then you still have this window of the file being accessible until actually deleted which is part of the report too.

I'm not sure how adding a dummy file usage would actually help, that would then still either always allow access or never. So we're back to the question what we should do with those files. Also, how would we remove files then that actually have no usage left and should be deleted?

The thing is that we return a different value for those two cases.

* If there is no file usage, we return NULL, so we don't grant access but we also don't explicitly deny. (access is still denied, unless someone else allows access)
* If there is file usage, but the user doesn't have access, we explicitly deny access.

I think we should be consistent. In which direction, I'm not completely sure, but I'm definitely leaning towards returning NULL in both cases, and allow other hooks to still grant access. That is consistent with how entity access works. Why is it so bad here? If you're implementing file access hooks, then it is your responsibility to handle it correctly.

Also, entity access has three states now. So we actually could differentiate between neutral and explicitly deny there as well, in that case, we would only explicitly deny if access('download') explicitly denies?

My suggestion would look like this. Completely untested, want to see what the test output is.

Uhm, that's what I get for not testing...

@Berdir: Are the removed conditions (owner, permanent) handled in the access method directly? Looking at \Drupal\file\Entity\File it does not seem like.

Yes, they are in FileAccessControlHandler.

The check there didn't actually check access, it just allowed the download access implementation to run which then again checked access. We're still doing that.

I must confess that I am a bit confused and not completely sure about the intent of this ticket. It appears to me that we're trying to solve a security issue that *might* be caused by a poor code or administrator giving out permissions to wrong people.

However, patch in #12 looks good to me. It makes thing more consistent and easier to understand. +1 to get it in.

working on a test

So here's a test based on the OP.
It fails but doesn't pass with the patch from #12
So either the test is wrong, the steps in the OP are wrong or the patch doesn't fix the issue.
I'm tipping it's one of the first two options.

Hoping someone else can shed some light on it.

My patch isn't supposed to fix the described problem, because I think that is not a bug, it's by design. If I have the "View private files" permission (which has restrict access => TRUE), then I expect to have access to *all* private files. Right now, I only have access as long as they are either not used anywhere or I do have access.

Imagine this: You upload a file with file_entity as a private file, and you have access to it. Until someone decides to use that file for a node that you don't have access to (e.g., the node is not yet published). Now you can't see your file anymore?

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -150,6 +150,11 @@ function file_test_file_validate(File $file) {
 function file_test_file_download($uri) {
+  if (\Drupal::state()->get('file_test.allow_all', FALSE)) {
+    $files = entity_load_multiple_by_properties('file', array('uri' => $uri));
+    $file = reset($files);
+    return file_get_content_headers($file);
+  }
   _file_test_log_call('download', array($uri));

So yes, if you implement the hook like this, you have a problem. But that's the same for hook_node_access(), if you return Access::isAllowed() from yourmodule_node_access(), then your site is wide open too. That's not a bug?

My patch is just making the behavior consistent and behaving the same way, so that it doesn't make a difference if there is a file usage without access or not file usage. We don't grant but also don't explicitly deny access. Unless a field/entity access explicitly returns forbid(), then we do forbid access.

So, a test for my patch would test the difference between a field or entity access returning an explicit forbidden and neutral. neutral is identical to no file usages. I'm not 100% sure that is correct (what if a file is used on two fields and one explicitly forbids access? I guess you still want access). Which would actually simplify the patch even more, we'd just have an else/if and FileAccessControlHandler doesn't need to be changed.

Tagging for issue summary update, as OP is wrong

I looked at least how file and node access is documented, not sure that gives a helping hand in understanding this problem:

So for files:

If one or more modules returned headers the download will start with the returned headers. If a module returns -1 an AccessDeniedHttpException will be thrown.

For entities:

Each implementation may explicitly allow, explicitly deny, or ignore the access request. If at least one module says to deny the request, it will be rejected. If no modules deny the request and at least one says to allow it, the request will be permitted.

So the docs on file access are contradicting, it both says that if at least one grants access (returns headers) it is downloaded and also says if someone returns -1 it will not.

Not sure I see why they are contradicting?

They are really the same IMHO:

By default, access is denied.

headers == allow
Not returning anything == neutral
-1 == forbidden

Comparing that to the node access system, wouldn't this be the equivalent of removing the core behavior where users with "access content" permission can see all published nodes by default, and then decreeing that no contrib node access module should attempt to add anything like that permission on their own? How would that work?

In the node access system, we certainly do expect that you can give the "access content" permission freely to unprivileged users without giving up on the idea of implementing node access on your site...

Hm, not a full answer, just some thoughts:
* Private files are already a subset of all files.
* If you want files to be available by default, then don't make them private files.
* access content is a special permission, because without it, we won't even check anything else. There is no equivalent in file access and also no generic entity access concept for that, that's why it is checked in access(), before even calling any hooks
* Even if all your files are private files, as long as they are attached to nodes that are available to anyone anyway.

I still don't see any use cases that would make sense to have that would break..

And what you are saying also completely depends on the core way of using private files, that they are uploaded to nodes and start with having file usages. When you use file_entity/media, then you can upload files without already having usages, and when you create them as private files then, then they would also be visible if such code would exist.

Setting back to needs review. I still think that this is not a security issue but only a non-critical inconsistency.

I'm not sure if the patch in #12 is OK, or if we should remove the neutral/forbidden behavior. The question to answer is this:

Assuming that a file is attached to two different nodes (1 and 2), what is the expected behavior:

N1 | N2 | Result
A   | A   | A
A   | N   | A
A   | F   | ?
N   | N  | F
N   | F   | F
F    | F   | F

Right now, you have access for the case A / F, and it probably has to stay like this, otherwise you can view the node, but the file won't be visible. Which I doubt is the expected case.

Tentatively tagging.

Last week I created another issue for core that can potentially handle these and more complex cases well.
Please take a look at https://www.drupal.org/node/2485807

So I think there is a much much simpler fix:

- Deny access to temporary private files with usage 0 by default.

  /**
   * Implements Drupal\file\FileUsage\FileUsageInterface::delete().
   */
  public function delete(FileInterface $file, $module, $type = NULL, $id = NULL, $count = 1) {
    // If there are no more remaining usages of this file, mark it as temporary,
    // which result in a delete through system_cron().
    $usage = \Drupal::service('file.usage')->listUsage($file);
    if (empty($usage)) {
      $file->setTemporary();
      $file->save();
    }
  }

And we talk about downloading, so that should be okay - if it is no longer attached anywhere ...

As Access + Deny == Access contrib is fine, too.

I am getting a patch for that going ...

The other part of the problem (and why file_entity is a really bad example) is that file_entity in D7 never checks hook_file_download_access() and it also uses the 'view' permission for the entity instead of the 'download' permission. (I just had the pleasure to circumvent that for a client ...)

So the real problem is the broken/strange API around this, but that problem is pre-existing in D7 and hence not a release blocker - unless we allow someone to access 'deleted' files in the race after it was deleted till cron runs.

That API is broken because:

- hook_file_download() can just allow access by sending headers.
- Only file_file_download() calls file_download_access_alter(), but works only on usage by entities

TL;DR: This is pre-existing strange in D7, lets fix the critical issue of the race when the file was deleted in D7 and when it is deleted lazily in D8.

And fixed the tests and added a proper fix.

That puts behavior almost back to where it was in D7.

It just means that if some module wants to grant access to temporary files, they need to make them either:

- permanent

or

- add file usage

Given that system_cron could run at any time (race conditions anyone? ... follow-up though), that is a good thing in general though.

And to have the owner of the file still being able to access the file should be okay ...

Fixed the test failures, the test depended on the old behavior, which nothing except tests would do like this.

Also the file was created for uid 1 by an anon user, which does not really much sense ( in a real world usage) ...

+++ b/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -599,8 +599,16 @@ function file_file_download($uri) {

hook_file_download() isn't document in the file api file. I was looking specifically to see if the return -1 was meant to be a constant or such. I see FileDownloadController::download() specifically checks for -1 though, so all good.

The rest of the patch looks pretty great.

Hm.

Deleting temporary files is actually optional now. I'm wondering if that has any impact on how this works, not sure.

I still think it would be better to go in the other direction but if that's what it takes to get rid of this issue, then so be it.

So could we fix the criticality of that issue and maybe be too harsh about that for now and then follow up on it later?

Are there scenarios where that's a problem? I'm not sure I can think of any. It does sort of reintroduce the requirement (at least for private files) that modules which want to keep an unused file around, e.g. in a "media library" that multiple admins need access to, must add a file usage record for it, so that it always stays permanent. Removing that requirement was sort of the main goal of #1399846: Make unused file 'cleanup' configurable (see the issue summary there)... I am not sure it was a totally valid goal, but it needs some consideration.

Well, they should not make it temporary then. Or at least put it back to permanent afterwards, but that is only a major bug - if at all.

Temporary files with no references / no usage should not be around forever. That completely subverts what 'temporary' means.

Because the file is now not deleted directly, they can still make it "permanent" again. Not totally pretty, but workable.

---

This comment is not quite right - the code is checking references (not usage), and system_cron() will not necessarily ever delete the file; it depends on how temporary file cleanup is configured on the site.

That is a very good point. Lets check for usage() instead first and then deny if usage == 0 && temporary (and not the owner of the file).

Then keep the references code intact, even smaller patch. And then the comment is correct :D.

And here is the patch as promised, which is now even more explicit about things and also improved the documentation.

Yes, file_cron() is also fixed in this patch. Thanks! (Could file a follow-up for BaseFileBase::delete() as that still refers to system_cron(), but out of scope here.)

Its good that we have a pretty good explanation of what is going on.

Issue summary seems to be pretty outdated. Do we need an update?

Yes that'd be useful - both for committers and for anyone who finds this issue one day via git blame.

Updated issue summary. Moving back to RTBC. While working on this also noticed a minor issue in documentation (#2489606: hook_file_download() docblock refers to a non-existing function file_download()).

Added some to the summary I had already written.

The patch attached closes the security hole and behaviour change from Drupal 7 detailed in the issue summary. If contrib wants to change the behaviour it can.

Committed dd0712a and pushed to 8.0.x. Thanks!