Security issue: Private files are allowed to download via download link, an attacker who knows the url can get any private file

Hmm, I'm assuming you're using some additional module like http://drupal.org/project/private_files_download_permission to prevent files from being accessed directly. Could you let me know which one and I'll take a look.

hey there, could you update on this please?

It looks like implementation of hook_file_download by file_force_file_download() has no logic to check for file access permissions.

The way an implementation of hook_file_download() is supposed to work, is it either should return -1 if module denies access -- then any modules that allow access are ignored, or return an array of headers that hints that the file access is allowed. Returning FALSE does neither allow nor deny access.

If none of the modules return -1, and there is an array returned by any of the modules, the access is granted.

So in summary, file_force_file_download() always returning an array allows any of the files that would be denied otherwise. The only way to configure files access with file_force module installed is to use another module that explicitly returns -1 from its hook_file_download.

As it is a security hole, it should be at a minimum documented on the project page and in the readme.

As to the fixing part, I can think of only couple of things:

• Secure token for download query
• invoke hook_file_download on all modules (except ourselves) and return an array only if there is no -1 and some array is returned. See file_download() for code example. This seems totally doable.

I think it's serious, so I rolled a patch based on my second bullet in #4. It seems to work ok.

Please review and test with your setup. Post here your results.

I think security issues should be reported to the security team: https://security.drupal.org/node/add/project-issue/file_force

Hmm, OK, two comments:

1. What is really the difference between returning a NULL vs FALSE in there? neither will have any effect on file_download() (besides, you didn't adjust the NULL at the top of the function).

2. In file_download(), all headers are thrown away if AT LEAST ONE module's implementation of hook_file_download returns "-1", regardless if that implementation is before or after some module (like file_force) that returns some headers. In other words, returning headers does not "allow access" - access is allowed by default, and only if at least one module's hook_file_download returns "-1", then access is denied, regardless of what all other modules are doing.

So, bottom line is, what is the issue with this module then?

PS. I do appreciate you for taking time to report/look into this. Making sure this is thoroughly investigated though before committing anything.

Cheers

Re #6
It is not a security breach. It is a security issue of admins not realizing that they are opening the door. There are plenty of modules that do the same by design. This one does it inadvertently for private files, but it has a good use with public files. Documenting it is sufficient to make it a non-security concern (so people will understand how they can have the door open, and what to do to have it closed). Of course fixing it will make this module totally awesome. (I'm looking to use it for Download links of MediaElement module that don't currently download but play the videos).

Re #7
1. I changed NULL to FALSE just to stay consistent with established design of hook_file_download. I missed another one? It happens when working late at night. Functionally it should be no difference.

2. The assumption that there may be a module returning -1 is incorrect. It is rarely the case. The rules for hook_file_download are a little convoluted, but after spending time on it they make sense. The rules are to return non-FALSE values ONLY for the files owned by that module. The access denied is based on either -1 (authoritative deny) or if none of the modules return an array if they don't own the file (lack of authoritative permission). File Force needs to change headers, but as a side-effect it breaks the rules by returning "authoritative permission" value for the files it DOES NOT OWN while other modules will agree to not allow the files (all other modules return FALSE).

Use case analysis... Let's say there is only one module that gives access to its files (before installing File Force). It will return FALSE for all files except the ones that it permits (most modules are implemented this way - I grepped and reviewed a big contrib tree, very few modules return -1 at all and ONLY for their files). Now if File Force is installed, it will give access to ALL private files because there is no module returning -1. It is incorrect implementation of File Force. A correct implementation should either 1) know each file access rules (which is impossible in modular architecture), or 2) not mess up with permitting other module's files.

People should realize that File Force uses hook_file_download for something it was not designed... for altering the output headers. If there was a hook_file_headers_alter or such, we would not have this problem. So to fix the problem I propose to gather all other modules output and analyze if there is a grant or not. If there isn't, we don't return any headers. This makes it sufficiently a "hook_file_headers_alter".

I re-rolled the patch with #7-1 and an additional fix for[#1933724] as splitting it into two patches will create a problem applying both patches (they work on the same function).

Also noticed one corner case in #1933728: Improve file_force_create_url() to add download=1 automatically. that needs improvement, including it here to avoid patch applying problems too.

Thanks, looks good to me (though I don't care much for the weird code formatting rules in PHP/Drupal, which I committed anyway :D). All committed and I guess I'll roll out a new stable release in the next couple of days.

Cheers