Make WordPress Core

Opened 7 years ago

Closed 21 months ago

Last modified 21 months ago

#42619 closed defect (bug) (fixed)

WordPress tries to access.bzr or .git without checking open_basedir restrictions

Reported by: meyegui's profile meyegui Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version: 4.9
Component: Upgrade/Install Keywords: has-patch needs-testing has-unit-tests commit
Focuses: Cc:

Description

Hi,

I'm getting the following error in my logs. I'm aware that I have open_basedir enabled, but I don't think WordPress should try to read files outside its installation directory. If I'm mistaken, I'm sorry and I'll be glad to receive any explanation as to why.

This bug doesn't generate any visible error or message other than this log, so I would definitely consider it "low severity". The log file was generated by a plugin of mine, but as you can see, the error doesn't occur in my own files. It's located in a core file.

Here's what I get in my log file:

[14:35:04]	
******************
PHP SHUTDOWN ERROR

Type: 2
Message: is_dir(): open_basedir restriction in effect. File(/home/.bzr) is not within the allowed path(s): (/home/httpd/vhosts/[hidden]/:/tmp/)
File: /home/httpd/vhosts/[hidden]/subdomains/[hidden]/wp-admin/includes/class-wp-automatic-updater.php:98
******************

Best regards

Attachments (2)

42619.diff (1.1 KB) - added by markjaquith 3 years ago.
Check open_basedir before checking a directory's existence
42619.2.diff (1.6 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (28)

#1 @meyegui
7 years ago

Still happening in WordPress 4.9.2.

Any idea why WP would try to read that file?

#2 @dd32
7 years ago

  • Component changed from General to Upgrade/Install
  • Keywords needs-patch added

Hi @meyegui and welcome to Trac.

I'd like to apologise for this ticket not getting a response until now.

This is caused by WordPress checking to see if it's running within a version-controlled environment, and avoiding autoupdating if that's the case.
The code responsible for this is located here: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-automatic-updater.php?marks=74-106#L58

WordPress doesn't take into consideration the PHP open_basedir setting, which causes it to process further up the path list than expected.

#35536 is a related ticket, where it processes up into completely invalid directories.

If you're curious as to why we care about a .git or .bzr file in a /home/username/ folder, it's because we decided to be ultra-conservative and check all the way up to / instead of just the parent directory of WordPress for if it's running within a VCS environment.
We could probably relax this restriction to checking the immediate parent of WordPress only, but that wouldn't take into account some edge-cases of deployment situations, where for example, the VCS files are in the grandparent instead - Like I said, this code was written extremely conservatively.

Fixing this, adding a check to ensure that it's not going to run into an open_basedir restriction would be good, although maybe we can look at relaxing this restriction in the first place at the same time.

@markjaquith
3 years ago

Check open_basedir before checking a directory's existence

#3 @markjaquith
3 years ago

Added is_allowed_dir() that checks to see if one of of the allowed open_basedir directories starts off the directory to be checked.

#4 @arnolp
3 years ago

  • Summary changed from WordPress tries to access /home/.bzr but to no avail to WordPress tries to access.bzr or .git without checking open_basedir restrictions

Still happening in 5.8.2

is_dir(): open_basedir restriction in effect. File(/.git) is not within the allowed path(s):

class-wp-automatic-updater.php:104

#5 @SergeyBiryukov
2 years ago

#56189 was marked as a duplicate.

#6 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.1

Some history here, via comment:3:ticket:56189:

#7 @SergeyBiryukov
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Refreshed the patch to:

  • Fix WPCS issues
  • Add missing documentation
  • Use the str_starts_with() polyfill introduced in [52040] / #54377

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


2 years ago

#11 @costdev
2 years ago

  • Keywords needs-unit-tests added

Thanks for the ticket @meyegui!

Per the discussion in the bug scrub, I'm adding the needs-unit-tests keyword and I'll work on unit tests for the new function 🙂

Last edited 2 years ago by costdev (previous) (diff)

This ticket was mentioned in PR #3224 on WordPress/wordpress-develop by costdev.


2 years ago
#12

  • Keywords has-unit-tests added; needs-unit-tests removed

#13 @costdev
2 years ago

  • Keywords dev-feedback added

PR 3224 attempts to add PHPUnit tests for the new method. However, tests are failing due to issues resetting open_basedir. See this comment for things I've tried to get this working. Any other ideas are appreciated 🙂

#14 @audrasjb
2 years ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as it still needs dev-feedback and testing.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


2 years ago

#17 @costdev
2 years ago

I've updated PR 3224 to remove the problematic tests that tested open_basedir, as this proved seemingly untestable.

This still needs some manual testing by setting the open_basedir PHP directive.

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


21 months ago

#19 @mukesh27
21 months ago

This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.

Thanks @costdev for PR. ping @robinwpdeveloper for testing.

This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.


21 months ago

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


21 months ago

#22 @costdev
21 months ago

  • Keywords commit added; dev-feedback removed

This ticket was discussed in the bug scrub. Pinging @SergeyBiryukov and @audrasjb for a final test/review for commit consideration.

@costdev commented on PR #3224:


21 months ago
#23

Update: I've added two additional tests to the PR that use @runInSeparateProcess and @preserveGlobalState disabled to allow for testing the open_basedir PHP directive. The docblock for the test methods explains why these annotations are used for these.

#24 @costdev
21 months ago

Additional props to @afragen and @pbiron for rubber-ducking with me on the open_basedir tests.

#25 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 55425:

Upgrade/Install: Introduce WP_Automatic_Updater::is_allowed_dir() method.

As part of determining whether to perform automatic updates, WordPress checks if it is running within a version-controlled environment, recursively looking up the filesystem to the top of the drive, looking for a Subversion, Git, Mercurial, or Bazaar directory, erring on the side of detecting a VCS checkout somewhere.

This commit avoids a PHP warning if the open_basedir directive is in use and any of the directories checked in the process are not allowed:

is_dir(): open_basedir restriction in effect. File(/.git) is not within the allowed path(s)

Follow-up to [25421], [25700], [25764], [25835], [25859].

Props costdev, markjaquith, meyegui, dd32, arnolp, robin-labadie, hellofromTonya, afragen, pbiron, SergeyBiryukov.
Fixes #42619.

@SergeyBiryukov commented on PR #3224:


21 months ago
#26

Thanks for the PR! Merged in r55425.

Note:

  • I have slightly modified the two tests for the open_basedir directive, as they failed on my setup with multiple checkouts and a symlink to the vendor directory in the “primary” checkout. There were some issues with PHPUnit along the lines of:
    Warning: include(): open_basedir restriction in effect. File(S:\home\wordpress.test\develop-trunk\vendor\phpunit\phpunit\src\Framework\TestFailure.php) is not within the allowed path(s): (S:\home\wordpress.test\develop-test/;S:\home\wordpress.test\develop-test/src/) in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
    Warning: include(S:\home\wordpress.test\develop-trunk\vendor\phpunit\phpunit\src\Framework\TestFailure.php): Failed to open stream: Operation not permitted in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
    Warning: include(): Failed opening 'S:\home\wordpress.test\develop-trunk\vendor\composer/../phpunit/phpunit/src/Framework/TestFailure.php' for inclusion (include_path='.;C:\php\pear') in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
    Class "PHPUnit\Framework\TestFailure" not found
    
  • I have also removed the error suppression operator from the is_dir() call, which was added in r25764 as a workaround for the same issue, and should no longer be necessary with this commit.
Note: See TracTickets for help on using tickets.