Exported user data can be listed with directory listing

[lucasbustamante]

Disclosure

This was opened as a HackerOne report at first, but upon triage it was requested that this was opened as a public Core ticket.

Description

WordPress 4.9.6 added the tool to "Export Personal Data", in compliance with GDPR: ​https://wordpress.org/support/article/tools-export-personal-data-screen/

Once generated, this data is stored as a .zip file in wp-content/uploads/wp-personal-data-exports, or wp-content/uploads/sites/{site_id}/wp-personal-data-exports in multi-site installations.

This directory is protected from directory listing with a index.html. However, it should be an index.php as used in wp-content/plugins/index.php and wp-content/themes/index.php. This is because index.php is the entrypoint to WordPress itself, so if a server has it's webserver's index pointing to something else than index.php, WordPress won't work at all.

That's why the index points to index.php in these examples from WordPress and Nginx:

​https://wordpress.org/support/article/nginx/
​https://www.nginx.com/resources/wiki/start/topics/recipes/wordpress/

In Apache, this is controlled by DirectoryIndex (​https://httpd.apache.org/docs/2.4/mod/mod_dir.html)

If you have directory listing enabled, and you don't have the optional directory index pointing at index.html, that directory can be listed, and the zip files with the exported user data can be downloaded directly from the browser.

Steps To Reproduce:

1. Install WordPress using Nginx or Apache, enable directory listing and point the directory index at index.php
2. Go to Tools -> Export Personal Data
3. Enter a username, eg: admin and click "Send Request"
4. Click "Download Personal Data"
5. Now go to ​https://your-url/wp-content/uploads/wp-personal-data-exports/

Recommendations

Replace index.html with index.php to prevent directory listing on wp-admin/includes/privacy-tools.php:325 and wp-admin/includes/privacy-tools.php:510

Impact

An attacker could have access to these information from users who exported their data for privacy reasons:

User's name, username, e-mail, session-tokens with expiration date, links to the files he/she uploaded, IP address, etc.

See https://core.trac.wordpress.org/ticket/52299

[xkon]

Hey @lucasbustamante thanks for report & PR!

I see that you changed the actual html export as well.

Ref:

if ( ! $zip->addFile( $html_report_pathname, 'index.html' ) ) {
	$error = __( 'Unable to add data to user privacy export file (HTML format).' );

This should stay in .html format in my opinion so users can easily open it if need be so there's no need to change that into a .php as well.

[lucasbustamante]

Replying to xkon:

Hey @lucasbustamante thanks for report & PR!

I see that you changed the actual html export as well.

Ref:

if ( ! $zip->addFile( $html_report_pathname, 'index.html' ) ) {
	$error = __( 'Unable to add data to user privacy export file (HTML format).' );

This should stay in .html format in my opinion so users can easily open it if need be so there's no need to change that into a .php as well.

Fixed. Thanks!

[xkon]

Looks good to me! Applied & tested as well, marking for commit.

[xkon]

Revisiting this, there are a couple more places that we'll need to update for index.php change.

The function wp_privacy_delete_old_export_files() should be changed for the .php file to remain untouched.

As well as in phpunit tests the test_creates_index_in_export_folder() in wpPrivacyGeneratePersonalDataExportFile.php and self::$index_path in wpPrivacyDeleteOldExportFiles.php check against index.html as well. These should be updated also to avoid failures while testing :) .

@lucasbustamante could you give it a look and update your PR please?

[lucasbustamante]

Replying to xkon:

Revisiting this, there are a couple more places that we'll need to update for index.php change.

The function wp_privacy_delete_old_export_files() should be changed for the .php file to remain untouched.

As well as in phpunit tests the test_creates_index_in_export_folder() in wpPrivacyGeneratePersonalDataExportFile.php and self::$index_path in wpPrivacyDeleteOldExportFiles.php check against index.html as well. These should be updated also to avoid failures while testing :) .

@lucasbustamante could you give it a look and update your PR please?

Updated, thanks! Commits:

• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/fda059e0c8dd82d7f73e64c88c1fd48650a34a35
• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/f5f6e1fa6d21d8f05959ddf02c14dd5fecd81875

@Luc45 Thank you for your contribution to WordPress!

In this PR PHPUnit tests are failing due to you haven't updated the PHPUnit test for your changes.

Please make PHPUnit test change here: ​https://github.com/Luc45/wordpress-develop/blob/exported-user-data-directory-listing/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportFile.php#L225

Updated, thanks! Commits:

• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/fda059e0c8dd82d7f73e64c88c1fd48650a34a35
• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/f5f6e1fa6d21d8f05959ddf02c14dd5fecd81875

[lucasbustamante]

I haven't replaced the index.html references from tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportFile.php, as they refer to the index.html that's placed inside the .zip file for the user to view the exported report.

[xkon]

Thanks @lucasbustamante !

I haven't replaced the index.html references from...

Correct some are testing against the actual exported index.html so they should stay the same.

Looks fine to me now & tests are passing :).

[freewebmentor]

Replying to prbot:

​Luc45 commented on ​PR #871:

@Luc45 Thank you for your contribution to WordPress!

In this PR PHPUnit tests are failing due to you haven't updated the PHPUnit test for your changes.

Please make PHPUnit test change here: ​https://github.com/Luc45/wordpress-develop/blob/exported-user-data-directory-listing/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportFile.php#L225

Updated, thanks! Commits:

• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/fda059e0c8dd82d7f73e64c88c1fd48650a34a35
• ​https://github.com/WordPress/wordpress-develop/pull/871/commits/f5f6e1fa6d21d8f05959ddf02c14dd5fecd81875

Thanks @lucasbustamante for the quick change, now all PHP tests are passing. Looks good 👍

[whyisjake]

In 50037:

Privacy: Ensure that exported user data reports can't be found with directory listings.

By moving from .html to .php files, we can prevent directory listings, and ensure that WordPress can load.

Fixes #52299.

Props lucasbustamante, xkon, freewebmentor, SergeyBiryukov, whyisjake.

[whyisjake]

In 50038:

Privacy: Ensure that exported user data reports can't be found with directory listings.

By moving from .html to .php files, we can prevent directory listings, and ensure that WordPress can load.

This brings the changes from [50037] to the 5.6 branch.

Fixes #52299.

Props lucasbustamante, xkon, freewebmentor, SergeyBiryukov, whyisjake.

[SergeyBiryukov]

In 50055:

Privacy: Add newlines to the index.php file in wp-personal-data-exports directory, for consistency with other similar files.

Follow-up to [50037].

See #52299.

[SergeyBiryukov]

In 50056:

Privacy: Add newlines to the index.php file in wp-personal-data-exports directory, for consistency with other similar files.

Follow-up to [50037].

Merges [50055] to the 5.6 branch.
See #52299.