WordPress.org

Make WordPress Core

#37680 closed defect (bug) (fixed)

PHP Warning: ini_get_all() has been disabled for security reasons

Reported by: dd32 Owned by: dd32
Milestone: 4.6.1 Priority: normal
Severity: normal Version: 4.6
Component: Bootstrap/Load Keywords: has-patch commit fixed-major
Focuses: Cc:

Description (last modified by dd32)

As reported in the Support Forums, WordPress 4.6 calls ini_get_all() which may be disabled on some hosts (possibly due to an old PHP 5.3.x security vulnerability).

https://wordpress.org/support/topic/warning-ini_get_all-has-been-disabled-for-security-reasons

This can be duplicated by adding disable_functions = ini_get_all to your php.ini file.

Looking at our usage of the ini_* functions in core, we:

  • sometimes silence warnings from ini_set() - usually early in the bootstrap, so I'm assuming from before wp_debug_mode() is called.
  • never silence ini_get() calls.

The current breakage on the affected hosts would be:

  • A PHP Warning will be displayed on sites post-upgrade to 4.6.
  • wp_raise_memory_limit() will fail to increase the memory limit, which may result in some admin pages not loading, or image uploads failing to create resized images.
  • WSOD - Single-site installs will not increase their memory limit to 40M, multisite will fail to increase to 64M, both of these could cause WSOD if the memory limit was set arbitrarily low and the site was running plugins which use a lot of memory.

We should fix this to at least not present with a PHP Warning, although I recognise that almost any function could be in the disable_functions setting, some (such as phpinfo() and ini_*) are historically much more likely to be there.

See #32075 for introduction.

Attachments (2)

37680.diff (1.4 KB) - added by dd32 16 months ago.
37680.2.diff (1.3 KB) - added by dd32 16 months ago.

Download all attachments as: .zip

Change History (23)

@dd32
16 months ago

@dd32
16 months ago

#1 @dd32
16 months ago

  • Description modified (diff)

I primarily see two main options:

That then leaves the question of what to do when disabled, Do we return true anyway as a "We can't determine if it's changeable, so we're just assuming it probably is."? or do we just continue to return false as 4.6 currently does?

I've taken the angle that we should return true in the event we can't be sure, this may result in PHP warnings being emitted from ini_set(), however we already silence warnings on the ini_set()'s at least for the memory_limit (which is the only thing this function is used for currently). It's the is_array() block at the end of the function in both patches that's responsible for this behaviour.

#2 follow-up: @blobaugh
16 months ago

From http://php.net/manual/en/function.ini-set.php

Returns the old value on success, FALSE on failure.

I think we should follow in line here an return a false value. That will signify a possible issue with the call. Alternatively it could return a WP_Error if having the ini_* functions available is a hard requirement.

#3 @SergeyBiryukov
16 months ago

#37695 was marked as a duplicate.

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


16 months ago

#5 in reply to: ↑ 2 ; follow-up: @dd32
16 months ago

Replying to blobaugh:

I think we should follow in line here an return a false value. That will signify a possible issue with the call. Alternatively it could return a WP_Error if having the ini_* functions available is a hard requirement.

The problem is that ini_set() and ini_get() are most likely available, it's just that ini_get_all() is disabled which causes an inability to determine if the ini_set() will pass or not.

WP_Error (which went unchecked) or null could be also used thanks to core using strict type checking on the call (for some reason) - (true: changeable, false: not changeable, null or WP_Error: who knows). We'd still only want to bail in the event it was false and not null or WP_Error though.

#6 @swissspidy
16 months ago

wp_is_ini_value_changeable() is being called in wp_initial_constants(), where the ini_get() call is already silenced. Silencing the ini_get_all() call as well seems fine in that case.

#7 in reply to: ↑ 5 @jeremyfelt
16 months ago

Replying to dd32:

The problem is that ini_set() and ini_get() are most likely available, it's just that ini_get_all() is disabled which causes an inability to determine if the ini_set() will pass or not.

This seems like a good reason to silence it.

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


16 months ago

#9 @BinaryKitten
16 months ago

It's considered bad practice to silence calls as it doesn't actually do much other than not show to the world when it generates an error message. The error log will still be populating with the message on every load and additionally the silence operator actually slows the process down (even though it's minute).

The check version is better as it actually detects the problem and processes it

#10 @swissspidy
16 months ago

  • Owner set to dd32
  • Status changed from new to assigned

@dd32 Assigning to you for final review & commit.

We could then keep it in trunk for a bit for folks to test.

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


16 months ago

#12 @jorbin
16 months ago

  • Keywords commit added

route number 2: 37680.2.diff seems best to me.

#13 @dd32
16 months ago

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

In 38431:

Bootstrap: Check that ini_get_all() exists before calling it, allows us to work around hosts who disable the function for "security purposes".

Fixes #37680 for trunk.

#14 @dd32
16 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

re-open for 4.6.1

I took a slightly different route, instead of checking if the function was disabled, simply called function_exists() which in retrospect is the obvious method..

#15 @dd32
16 months ago

  • Description modified (diff)
  • Keywords fixed-major removed

#16 @dd32
16 months ago

  • Keywords fixed-major added

#17 follow-up: @jdgrimes
16 months ago

@dd32 function_exists() doesn't detect disabled functions:

Note:
A function name may exist even if the function itself is unusable due to configuration or compiling options (with the image functions being an example).

#18 in reply to: ↑ 17 ; follow-up: @jeremyfelt
16 months ago

Replying to jdgrimes:

@dd32 function_exists() doesn't detect disabled functions:

Note:
A function name may exist even if the function itself is unusable due to configuration or compiling options (with the image functions being an example).

function_exists() returns false for functions disabled through disable_function in php.ini.

Via the discussion on #26772, it seems like it's possible for a false positive when using suhosin config to disable. We added an additional check for ini_get( 'disable_functions' ) in [29330], but I'm not sure how that works with suhosin anyway, which uses the option suhosin.executor.func.blacklist.

It may be that we've done just fine with function_exists() on it's own beyond that one bug report, but I may also not understand a piece.

#19 in reply to: ↑ 18 ; follow-up: @dd32
16 months ago

Replying to jeremyfelt:

Replying to jdgrimes:

@dd32 function_exists() doesn't detect disabled functions:

Note:
A function name may exist even if the function itself is unusable due to configuration or compiling options (with the image functions being an example).

function_exists() returns false for functions disabled through disable_function in php.ini.

Via the discussion on #26772, it seems like it's possible for a false positive when using suhosin config to disable. We added an additional check for ini_get( 'disable_functions' ) in [29330], but I'm not sure how that works with suhosin anyway, which uses the option suhosin.executor.func.blacklist.

It may be that we've done just fine with function_exists() on it's own beyond that one bug report, but I may also not understand a piece.

Technically you're right, personally though, I don't want to perform silly work arounds like that. I'm fine with a server which uses a hardening extension to disable a safe function throwing warnings.

#20 in reply to: ↑ 19 @jeremyfelt
16 months ago

Replying to dd32:

Technically you're right, personally though, I don't want to perform silly work arounds like that. I'm fine with a server which uses a hardening extension to disable a safe function throwing warnings.

+1

#21 @dd32
16 months ago

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

In 38460:

Bootstrap: Check that ini_get_all() exists before calling it, allows us to work around hosts who disable the function for "security purposes".

Merges [38431] to the 4.6 branch.
Fixes #37680.

Note: See TracTickets for help on using tickets.