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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach created an issue. See original summary.

plach’s picture

Status: Needs review » Needs work

The last submitted patch, 2: views_data_export-file_download_acess-2798759-2.patch, failed testing.

The last submitted patch, 2: views_data_export-file_download_acess-2798759-2.patch, failed testing.

plach’s picture

Bad patch...

klausi’s picture

Priority: Normal » Critical

Security issues are critical. Too bad this module is still in beta, otherwise we would handle this in private.

plach’s picture

Status: Needs review » Needs work

This does not handle correctly the "Provide file" option. I'll post an updated patch tomorrow...

plach’s picture

Issue summary: View changes
Steven Jones’s picture

+++ b/views_data_export.module
@@ -31,6 +31,42 @@ function views_data_export_views_api() {
+    if (!user_access('access all views')) {

This 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?

plach’s picture

Well, 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 :)

Steven Jones’s picture

Well, 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 :)

jamsilver’s picture

This patch looks really good. +1

Too bad this module is still in beta, otherwise we would handle this in private.

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!).

This 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?

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?

plach’s picture

plach’s picture

+++ b/views_data_export.module
@@ -31,6 +31,36 @@ function views_data_export_views_api() {
+  if (views_data_export_is_export_file($uri)) {

I'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...

plach’s picture

Issue summary: View changes
marco’s picture

+1 for the patch.

I wonder if there's a case for sending out a security notification at some point anyway?

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.

I'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...

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.

plach’s picture

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.

Yep, 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 :)

plach’s picture

Additionally, 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.

marco’s picture

Additionally, 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.

+1, now I'm sold on the global switch.

Steven Jones’s picture

Thanks 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.

The last submitted patch, 20: views_data_export-2798759-access_test.patch, failed testing.

The last submitted patch, 20: views_data_export-2798759-access_test.patch, failed testing.

  • Steven Jones committed 35c72ce on 7.x-3.x authored by plach
    Issue #2798759 by plach: Ensure access control is performed on batched...
  • Steven Jones committed 9df9fe9 on 7.x-3.x
    Issue #2798759 by Steven Jones: Test that access control is performed on...
Steven Jones’s picture

Status: Needs review » Fixed
Steven Jones’s picture

Thanks @plach for getting this done.

I've created the release node and we're just waiting for the security team to publish.

plach’s picture

Awesome guys, thanks!

plach’s picture

Should we backport this to D6?

No longer supported :)

Alan D.’s picture

@maintainers
If applicable to D6, you can always post here https://www.drupal.org/project/d6lts :)

sagashiteirukedo’s picture

This 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') ...

Steven Jones’s picture

Thanks for the detail, working on a fix for this, and for the issues in #2800895: Access denied when performing batched export as admin

sagashiteirukedo’s picture

Thanks 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.)

Steven Jones’s picture

@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.

sagashiteirukedo’s picture

A-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!

Status: Fixed » Closed (fixed)

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