WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 8 months ago

#51806 new defect (bug)

Add an early exit for files with _deprecated_file() calls

Reported by: SergeyBiryukov Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

Background: #35835

[49619] added an early exit when calling the wp-includes/rss-functions.php file directly to avoid unnecessary errors in server logs.

There are ~30 other files in core with _deprecated_file() calls so it seems like a good idea to have a consistent policy for the other instances.

That said, it's not limited to _deprecated_file(), as noted in comment:3:ticket:35835 and comment:1:ticket:35835:

You'll get a similar error if you access wp-includes/default-filters.php or any number of others too potentially.

So perhaps other files that can clutter server logs with unnecessary errors would benefit from an early exit too.

Change History (6)

#1 @ayeshrajans
11 months ago

This is probably not an ideal place to discuss this, but I'd like to raise a concern I had in my mind for a long time.

In WordPress, we have files that we can call directly from the browser (for example wp-admin/options-writing.php), and ones that we cannot (such as wp-blog-header.php). I totally agree that we need some sort of consistency, because at the moment, it is not clear which files are meant to be entry points, and which are to be included.

I wonder if there was any discussion to move the include files to where they belong, or rename them (for example to .inc in Drupal style), so they can be blocked from web server configuration and save ourselves of the early-exit calls, while also getting some potential opcache benefits?

#2 @zodiac1978
11 months ago

As a first step I checked the 30 search results which are using _deprecated_file():

These files are all resulting in a full path disclosure if display_errors is enabled:

wp-admin/admin-functions.php
wp-admin/custom-background.php
wp-admin/custom-header.php
wp-admin/includes/class-wp-upgrader-skins.php
wp-admin/upgrade-functions.php
wp-includes/class-feed.php
wp-includes/class-json.php
wp-includes/class-oembed.php
wp-includes/class-snoopy.php
wp-includes/class-wp-customize-control.php
wp-includes/class-wp-feed-cache.php
wp-includes/customize/class-wp-customize-new-menu-control.php
wp-includes/customize/class-wp-customize-new-menu-section.php
wp-includes/date.php
wp-includes/embed-template.php
wp-includes/locale.php
wp-includes/registration-functions.php
wp-includes/registration.php
wp-includes/rss-functions.php
wp-includes/rss.php
wp-includes/session.php
wp-includes/spl-autoload-compat.php
wp-includes/theme-compat/comments.php
wp-includes/theme-compat/footer.php
wp-includes/theme-compat/header.php
wp-includes/theme-compat/sidebar.php

PHPMailer is checking via function_exists first:

wp-includes/class-phpmailer.php

Not sure why this is not reproducing the FPD:

wp-includes/class-smtp.php

This file has a check and uses _deprecated_file() only if a my-hacks.php file exists (but would generate FPD if available, I think):

wp-includes/load.php

And the 30th search result is just the function itself:

wp-includes/functions.php

And for completeness I would like to point again to the issue on the Health Check plugin to check for display_errors:
https://github.com/WordPress/health-check/issues/370

because we recommend to disable this setting:
https://make.wordpress.org/core/handbook/testing/reporting-security-vulnerabilities/#why-are-there-path-disclosures-when-directly-loading-certain-files

Last edited 11 months ago by zodiac1978 (previous) (diff)

#3 @SergeyBiryukov
11 months ago

Thanks for the list of files! Just noting that this is not so much about full path disclosure specifically (which is covered by the handbook link above), but more about just preventing unnecessary errors in server logs.

#4 @zodiac1978
11 months ago

Just noting that this is not so much about full path disclosure specifically

I understand, but then we need to discuss our recommended path here.

The error log mentioned in the first ticket could be generated through testing tools, like wpcheck, which are just taking one of those files to check for full path disclosure:

For example: https://github.com/sergejmueller/wpcheck/blob/50fea1c1fe9b46d3fda8c2dae3b2214e9c0f5671/lib/rules/fpd-vulnerability.js#L32

If we fix it in one file, these tools will change to another file and generate unnecessary error logs again.

I think we have two (or three) possible solutions here:

  • 1. Fix every single file to not show any errors on direct access.
  • 2a. Force disabling display_errors per default (maybe not if WP_DEBUG is true)
  • 2b. Add a check in Site Health to inform the user that display_errors is on.

If we have a decision on the path, then we could go forward.

Maybe @clorith can add an opinion on the Site Health idea?

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


9 months ago

#6 @lukecarbis
8 months ago

  • Milestone changed from 5.7 to Future Release
Note: See TracTickets for help on using tickets.