Problem/Motivation
The Views Data Export module does not check ownership of export files when offering them for download. This means that theoretically any user having access to the export view and thus knowing the download URL can download any export file. This may lead to access bypass, as the view result can be filtered depending on the access role, or any other rule.
A mitigating factor is that export files are stored in the temp directory, so they are periodically wiped.
Proposed resolution
Ensure the user downloading the export is the one that launched the export or has access bypass permissions for all views.
Remaining tasks
Reviews
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#20 | views_data_export-2798759-access_test-and_fix.patch | 8.92 KB | Steven Jones |
#20 | views_data_export-2798759-access_test.patch | 4.94 KB | Steven Jones |
#13 | views_data_export-file_download_access-2798759-13.patch | 3.98 KB | plach |
Comments
Comment #2
plachComment #5
plachBad patch...
Comment #6
klausiSecurity issues are critical. Too bad this module is still in beta, otherwise we would handle this in private.
Comment #7
plachThis does not handle correctly the "Provide file" option. I'll post an updated patch tomorrow...
Comment #8
plachComment #9
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThis seems unnecessary, if we're tightening up security of what's been exported, then why not always enforce that the user that generated the export is the only user that is allowed to download it?
Comment #10
plachWell, in my original work I didn't add that check, but then I thought it could be helpful, if for any reason an admin had to troubleshoot an export or similar stuff... Totally fine with removing it if it becomes a blocker, though :)
Comment #11
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedWell, if we're already saying that the output of the view can be sensitive per-user and not just per role/permission, then I think we should respect that everywhere we can.
So this won't be a blocker, but I'll remove it after committing this if it's not in the patch :)
Comment #12
jamsilver CreditAttribution: jamsilver commentedThis patch looks really good. +1
Yes. It really is a shame given how many sites are using Views Data Export. I wonder if there's a case for sending out a security notification at some point anyway? I've raised https://security.drupal.org/node/160879 to ask this question (not sure if that's the best way of doing it!).
Agree that it's probably absolutely fine to always enforce that the export author is the only one permitted to download the export. We could allow a general permission, but it should probably be a brand new permission (rather than access all views) so what's being granted is clear? Probably is easier to not include it isn't it?
Comment #13
plachThis seems to work fine both with "Provide as file" enabled and disabled.
Comment #14
plachI'm wondering whether it would make sense to add a variable here to disable this check so other modules can implement their own access control. Otherwise, if
-1
is returned, no other module will have a chance to speak...Comment #15
plachComment #16
marco CreditAttribution: marco at Tag1 Consulting commented+1 for the patch.
IMHO there should be at least a notice in the project home page ("Before 7.x-3.0-beta10 there was a critical security issue..."), but a notification would be even better.
For the file to download I believe it's ok to force the ownership check, as the usual Views permissions seem enough, unless I'm missing some use case.
For example, I could share the link to the exported view with you, permission checks will run for the exported view, and eventually create a link to a file only you can download.
Comment #17
plachYep, but this wouldn't guarantee what I'm seeing is the same of what you would see. Anyway, apparently I'm the only one seeing a use case for this, so I will just shut up :)
Comment #18
plachAdditionally, if this patch were to create problems on one of the thousands sites out there using VDE, a variable would allow them to disable its behavior and replace it with something more suitable to the site's needs.
Comment #19
marco CreditAttribution: marco at Tag1 Consulting commented+1, now I'm sold on the global switch.
Comment #20
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThanks for your work on this!
The patch in #13 looks great, I think we could add a variable for 'access opt-out' in a follow up issue.
Here's a patch that adds a test, and then another patch that is the patch in #13 plus the test.
Comment #24
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedComment #25
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThanks @plach for getting this done.
I've created the release node and we're just waiting for the security team to publish.
Comment #26
plachAwesome guys, thanks!
Comment #27
plachShould we backport this to D6?No longer supported :)
Comment #28
Alan D. CreditAttribution: Alan D. commented@maintainers
If applicable to D6, you can always post here https://www.drupal.org/project/d6lts :)
Comment #29
sagashiteirukedo CreditAttribution: sagashiteirukedo commentedThis new code may be problematic in a rather specific situation:
Our live site uses a load balancer and we'd sometimes run into a problem with exporting a file to the server-specific temp directory, but then have the admin get shunted to another web server and not have the exported file waiting for Drupal to find in that other server's temp directory.
We worked around that by putting this line into settings.php:
$conf['views_data_export_directory'] = 'private://';
The private file area is on a shared file server common to the respective web servers, so any temp file made is visible from any of the web servers (until it naturally goes away).
With that line in settings.php, the new code is preventing the file transfer since the if(count($headers)) conditional check comes back with a 0 and you drop to drupal_access_denied().
If I comment out that line in settings.php (on a dev system which isn't behind the load balancer) and use the normal temp file location right there on the web server, then the if(count($headers)) conditional check comes back with a 4 and one gets to file_transfer($uri, $headers) as expected.
I'm not sure why it can't read in the $headers to have a count for them when the temp file is written to the private area on the common file server, but that seems to be what is happening. I could certainly be missing something obvious, but for now I'm trying to figure out how I can still use that common private area for the temporary export file to write to -- AND be able to have the file transferred. Perhaps if(count($headers) || $scheme = 'private') ...
Comment #30
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThanks for the detail, working on a fix for this, and for the issues in #2800895: Access denied when performing batched export as admin
Comment #31
sagashiteirukedo CreditAttribution: sagashiteirukedo commentedThanks for checking that out.
I'm wondering if it has something to do with the private file area being on a different server (folder on web servers are mapped with fstab to the folder on the file server for private files). That is, when it fetches header info from the file if just on the same server's temp area -- it can get header info to work with. However, when it tries to fetch that info off of a file it can see but that is actually residing on the file server -- no dice.Oops -- just looked at the other issue as well as more closely at the new code in views_data_export.module. Line 60 does look to want things to just be the 'temporary' scheme, rather than allowing for others such as 'private'. Sorry for the different server speculation!Aside: My little tweak of if(count($headers) || $scheme = 'private') tested out for me, but that just bypasses the desired behavior in this thread of making things user-who-launched-the-export-specific. Thus: doh! For our application, that the exported files are role-specific is fine -- and they continue to be so in my tests with the new code and that one tweak. (Tests being -- Using the tweak and the new code, can I kick out a file as someone with access? Yep -- huzzah! Can I snag that link and use it as someone else in the right role? Yep -- ok for us. Can I use that link as someone without the right role or an anonymous user? Nope -- as desired.)
Comment #32
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented@sagashiteirukedo I would love you to try out the patch in #2800895-8: Access denied when performing batched export as admin and see if that fixes the issue you're seeing.
Comment #33
sagashiteirukedo CreditAttribution: sagashiteirukedo commentedA-yep -- that patch lets me retain the line in settings.php to have it write to the private file area, and it enforces the per-user restriction on the download link, so only the person who made it can download it.
Thanks for such a quick response!