WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 6 months ago

#52299 closed defect (bug) (fixed)

Exported user data can be listed with directory listing

Reported by: lucasbustamante Owned by: SergeyBiryukov
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Privacy Keywords: has-patch has-unit-tests commit
Focuses: privacy Cc:

Description

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.

Attachments (1)

52299.diff (2.7 KB) - added by whyisjake 6 months ago.

Download all attachments as: .zip

Change History (17)

This ticket was mentioned in PR #871 on WordPress/wordpress-develop by Luc45.


7 months ago

  • Keywords has-patch added

#2 follow-up: @xkon
7 months ago

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.

#3 in reply to: ↑ 2 @lucasbustamante
7 months ago

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!

#4 @SergeyBiryukov
7 months ago

  • Milestone changed from Awaiting Review to 5.6.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @xkon
7 months ago

  • Keywords commit added

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

#6 follow-up: @xkon
6 months ago

  • Keywords commit removed

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?

#7 in reply to: ↑ 6 @lucasbustamante
6 months ago

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:

#9 @lucasbustamante
6 months ago

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.

#10 @xkon
6 months ago

  • Keywords has-unit-tests commit added

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

#11 in reply to: ↑ 8 @freewebmentor
6 months ago

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:

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 months ago

@whyisjake
6 months ago

#13 @whyisjake
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#14 @whyisjake
6 months ago

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.

#15 @SergeyBiryukov
6 months ago

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.

#16 @SergeyBiryukov
6 months ago

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.

Note: See TracTickets for help on using tickets.