Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file access control on batched export downloads - broken by recent commit #1

Open
wants to merge 1 commit into
base: 6.x-3.x
Choose a base branch
from

Conversation

amorsent
Copy link

The security fix b22a769 seems to always return access denied even when it should allow access.

The problem is that the $filepath passed to views_data_export_is_export_file() is an absolute system path and so the strpos check doesn't work.

Wrapping realpath() around file_directory_temp() seems to solve the issue, although I'm not sure if this is the right fix. For one thing it means that every call to hook_file_download() will need to compute the realpath of the temp dir just to compare it...

Is it even correct to pass absolute system paths to hook_file_download()? The record in the files table also contains the absolute path.

This all seems to originate here in views_data_export_plugin_display_export::outputfile_create()
$path = tempnam(realpath($dir), 'views_data');

tempnam() returns an absolute path and this ends up as the filepath on the file record and we're always dealing with an absolute path from then on..

So maybe it's better to fix it there?

@amorsent
Copy link
Author

amorsent commented Dec 8, 2020

Taking another look, I'm realizing that this bug is also somewhat specific to my configuration.

Aegir had configured my file_directory_temp = 'sites//private/temp', so this is what file_directory_temp() returns.
This is why the strpos check does not work against the absolute $filepath passed in.

function views_data_export_is_export_file($filepath) {
  return strpos($filepath, file_directory_temp() . '/views_plugin_display') === 0;
}

If file_directory_temp is not configured, it works because file_directory_temp() also returns an absolute path.
It also works if I configure file_directory_temp using an absolute path. I will be doing this for now.

However, I don't think it's incorrect to use a relative path for file_directory_temp, so I think views_data_export is still a little wonky here.

¯_(ツ)_/¯ Hope someone finds this helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant