WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23289 closed defect (bug) (fixed)

wp_protect_special_option rejects things it shouldn't

Reported by: agarden Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version: 2.2
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

If wp_protect_special_option is called with $option == 0 it will die. I have a client for whom this took down their entire WordPress install. Now, I assume that some plugin or some such should not have been passing zero as an option name, so that's another bug. But a buggy plugin should not be able to take down an entire install so carelessly.

Fix is trivial. Have in_array use strict checking. Patch is attached.

Attachments (2)

patch (509 bytes) - added by agarden 2 years ago.
Patch
23289.diff (793 bytes) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (10)

@agarden2 years ago

Patch

comment:1 @SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.6
  • Version changed from trunk to 2.2

Related: #2268

Confirmed that in_array( 0, array( 'alloptions', 'notoptions' ) ) returns true.

comment:2 follow-up: @TobiasBg2 years ago

PHP, this earns you another WTF?

Compare

var_dump( in_array( 0, array( '1', 'a2' ) ) ); // bool(true)

to

var_dump( in_array( 0, array( '1', '2a' ) ) ); // bool(false)

I'm a little bit speechless right now...

comment:3 in reply to: ↑ 2 @rmccue2 years ago

  • Keywords commit added

Replying to TobiasBg:

PHP, this earns you another WTF?

Makes full sense given that it's a non-strict comparison. When converting strings to integers, it takes as many digits as possible before a non-digit and turns that into an integer. In this case, 2a becomes 2, but there are no digits at the start of a2, so that becomes 0.

Patch looks good, +1.

comment:4 @TobiasBg2 years ago

Ok, I'm no longer speechless. I guess that does make sense, given that

var_dump( 0 == 'a2' );

also gives true.
Initially, I was expecting (or hoping?) for strict comparison to be the default in in_array, but that would be inconsistent with other functions. So, yeah, it does make sense.

@nacin2 years ago

comment:5 @nacin2 years ago

Best I can tell, this would only affect delete_option(). The other methods — get, update, and add — all trim( $option ) then check if $option is empty, and if so, return false. trim() would convert 0 to '0' but this is still empty.

comment:6 @nacin2 years ago

In 1222/tests:

Tests for wp_protect_special_option(), and for delete_option() trimming the option and checking for emptiness like other functions. see #23289.

comment:7 @nacin2 years ago

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

In 23510:

Ensure we strictly compare 'alloptions' and 'notoptions' when protecting these special options.

Trim and check for emptiness of $option in delete_option(), as done for get, update, and add.

fixes #23289.

comment:8 @SergeyBiryukov2 years ago

#24108 was marked as a duplicate.

Note: See TracTickets for help on using tickets.