Make WordPress Core

Opened 4 years ago

Last modified 4 weeks ago

#47164 new defect (bug)

map_deep in formatting.php do not handle null-byte

Reported by: bitcomplex's profile bitcomplex Owned by:
Milestone: 6.2 Priority: normal
Severity: critical Version: 5.2.2
Component: Formatting Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

<?php
foreach ( $object_vars as $property_name => $property_value ) {
        $value->$property_name = map_deep( $property_value, $callback );
}

The above code snippet in the function map_deep in formatting.php will trigger a fatal error if for some reason $property_name starts with a null-byte. null-bytes can exist in this context if $object_vars for some reason is from an object cast to an array. private and protected properties will be prefixed with null * null

We've encountered it in the wild with serialized objects, and even though this is because of faulty programming (child classes with stricter access for properties than the parents) wordpress should handle this.

The simples solution I can think of id to add:

<?php
foreach ( $object_vars as $property_name => $property_value ) {
                        **if (ord($property_name) === 0) {
                                continue;
                        }**
                        $value->$property_name = map_deep( $property_value, $callback );
                }

Attachments (1)

47164.diff (522 bytes) - added by bitcomplex 3 years ago.

Download all attachments as: .zip

Change History (18)

#1 @bitcomplex
3 years ago

  • Severity changed from normal to critical
  • Version set to 5.2.2

I was pretty sure this was an issue related to a php bug patched in php7.2 ( https://bugs.php.net/bug.php?id=49649 )
But after upgrading to 7.3.7 (from 7.1.x) we get issues with this.

serialized objects fetched from db fails here. It is possible because of the objectes in the db is serialized with an old php version where the bug exists. But wp could handle it better.

Error: Cannot access property started with '\0'
#12 /home/httpd/rackesbutiken/rackesbutiken.se/wp-includes/formatting.php(4742): map_deep
#11 /home/httpd/rackesbutiken/rackesbutiken.se/wp-includes/formatting.php(2691): stripslashes_deep
#10 /home/httpd/rackesbutiken/rackesbutiken.se/wp-includes/formatting.php(5342): wp_unslash
#9 /home/httpd/rackesbutiken/rackesbutiken.se/wp-includes/meta.php(182): update_metadata
#8 /home/httpd/rackesbutiken/rackesbutiken.se/wp-includes/post.php(2061): update_post_meta

#2 @bitcomplex
3 years ago

Even though this is old, it pretty much explains the problem in a short and concise way: https://cweiske.de/tagebuch/php-property-started-nul.htm

#3 @SergeyBiryukov
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hi @bitcomplex, welcome to WordPress Trac! Thanks for the report.

Just noting that I was able to reproduce the fatal error with this code:

class Dummy {
    public $pub = 0;
    protected $prot = 1;
    private $priv = 2;
}

$test = (object) (array) new Dummy();
$test = stripslashes_deep( $test );

@bitcomplex
3 years ago

#4 @bitcomplex
3 years ago

  • Keywords has-patch added; needs-patch removed

#5 @bitcomplex
3 years ago

@SergeyBiryukov what can I do to get this patched? Our site breaks for each wp update that touches formatting.php. It's no joke :(

#6 @bitcomplex
2 years ago

@SergeyBiryukov Another update that changes formatting.php so another ping to you on this issue. I don't know what more I should do...

It's not only in formatting.php this issue can arise, but it's the most severe place.

#7 @bitcomplex
2 years ago

2 years and counting. Still have to manually add the fix for this each time there comes a WordPress update. Any news @SergeyBiryukov ?

#8 @rachelbaker
20 months ago

  • Keywords needs-unit-tests added

Some unit tests would be helpful to move this along.

Last edited 20 months ago by rachelbaker (previous) (diff)

#9 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 6.2

This ticket was mentioned in PR #3597 on WordPress/wordpress-develop by @martin.krcho.


4 weeks ago
#10

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

Pull request contains the original patch from Trac ticket plus a unit test covering the change.

Trac ticket: https://core.trac.wordpress.org/ticket/47164

@martin.krcho commented on PR #3597:


4 weeks ago
#11

The new unit test fails on PHP 5.6

1) Tests_Formatting_MapDeep::test_map_deep_object_with_private_and_protected_properties
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 0
     1 => 1
-    2 => 2
+    2 => 1
+    3 => 2
 )

Can anyone think of better way to compare the Dummy_47164 object with stdClass? I saw ReflectionMethod being used in other unit tests so perhaps that might be a way forward?

cc @SergeyBiryukov , @rachelbaker , @hellofromtonya

@costdev commented on PR #3597:


4 weeks ago
#12

@martinkrcho Try passing $property_value by reference in the foreach() and assigning $property_value directly:

Current:
{{{php
foreach ( $object_vars as $property_name => $property_value ) {

if ( 0 === ord( $property_name ) ) {

continue;

}
$value->$property_name = map_deep( $property_value, $callback );

}
}}}
Suggested:
{{{php
foreach ( $object_vars as $property_name => &$property_value ) {

if ( 0 === ord( $property_name ) ) {

continue;

}
$property_value = map_deep( $property_value, $callback );

}
unset( $property_value );
}}}

@martin.krcho commented on PR #3597:


4 weeks ago
#13

Thank you for suggestion, @costdev.

Passing the property value by reference broke a lot of other tests across multiple versions of PHP. I'll revert and keep thinking about this.

@costdev commented on PR #3597:


4 weeks ago
#14

Sorry about that @martinkrcho! I made a really obvious mistake in my suggestion too 😅. I'll keep looking into this also, but would welcome others provide any insight they have about this issue.

#15 @costdev
4 weeks ago

  • Keywords dev-feedback added

I saw the failing test on PHP 5.6 on the PR and starting looking into why it's failing.

@hellofromTonya @jrf @SergeyBiryukov Would appreciate a confidence check on the below.


Sample:

<?php
class MyClass {
    public    $pub  = 0;
    protected $prot = 1;
    private   $priv = 2;
}

// Produce an object with null byte in the property name.
$obj      = (object) (array) new MyClass();
$obj_vars = get_object_vars( $obj );
  • In PHP 5.6, $obj_vars is [ 'pub' => 0, 'prot' => 1 ];
  • In PHP 7.0+, $obj_vars is [ 'pub' => 0, '\0*\0prot' => 1, '\0MyClass\0priv' => 2 ];

3v4l


<?php
foreach ( $obj_vars as $name => $value ) {
    $obj->$name = $value;
}
  • PHP 5.6 will not throw an error.
  • PHP 7.0+ will throw an error: Fatal error: Cannot access property started with '\0'

3v4l


If we then clean up the property name with, for example, trim( $name, "\0*" ):

  • PHP 5.6 adds one new public property with the same name as the protected property.
  • PHP 7.0+ adds two new public properties with the same names as the protected and private properties.

3v4l


Provided there's no issues I've missed/created along the way, could I get a confidence check?

Assuming we were to allow the creation of the public variants of the protected/private properties, when writing a test to cover this, we must test different values depending on the PHP version. Correct, or incorrect?

i.e.

<?php

// PHP 5.6
$expected = array(
    0, // public $pub
    1, // protected $prot
    1, // public $prot
    2  // private $priv
);

// PHP 7.0+ adjustment
if ( version_compare( PHP_VERSION, '7.0', '>=' ) ) {
        $expected[] = 2; // public $priv
}

// Assertion.
Last edited 4 weeks ago by costdev (previous) (diff)

#16 @costdev
4 weeks ago

Alternatively:

The current patch will continue on protected / private properties using 0 === ord( $property_name ) to detect NUL. However, it appears that this does not detect NUL in the protected property in PHP 5.6. I'm not sure if the addition of NUL is PHP 7.0+ behaviour, if the detection method is inaccurate, or something else.

Would appreciate thoughts on this also.

#17 @cadic
4 weeks ago

With the suggested solution, the function

<?php
$actual = map_deep(
        (object) array(
                'var0'   => '1',
                chr( 0 ) => (object) array(
                        'var0'   => '2',
                        chr( 0 ) => '3',
                ),
                'var1'   => (object) array(
                        'var0'   => '4',
                        chr( 0 ) => '5',
                ),
        ),
        'intval'
);

It will return an object:

object(stdClass) (3) {
  ["var0"]=>
  int(1)
  ["\0"]=>
  object(stdClass) (2) {
    ["var0"]=>
    string(1) "2" // <-- intval wasn't applied, parent key is \0
    ["\0"]=>
    string(1) "3" // <-- intval wasn't applied, parent key is \0
  }
  ["var1"]=>
  object(stdClass) (2) {
    ["var0"]=>
    int(4)
    ["\0"]=>
    string(1) "5" // <-- intval wasn't applied, key is \0
  }
}

It's better than a Fatal error, but that also means that map_deep hasn't applied the callback properly to all nested properties.

Note: See TracTickets for help on using tickets.