Use of get_object_vars() in sanitize_post() and WP_Post constructor does not handle null byte

[bitcomplex]

In places where get_object_vars is used to loop over an objects properties and then trying to access them null bytes are not handled.

There is an old bug-report (from me) for map_deep #47164 but now we are experience this in other places too; in sanitize_post and in the constructor of class-wp-post.

This is totally destroying our business and I don't know what to do. Since I reported the issue for map_deep I have had to manually patch formatting.php every time there is a WordPress update. But now, trying to handle all the places get_object_vars is used in hopeless.

Best approach to handle this would be to always filter the return values from get_object_vars. Something like:

<?php
    $properties = array_filter( fn( $var ) => ord( $var ) !== 0, get_object_vars( $object )); 

[SergeyBiryukov]

#56690 was marked as a duplicate.

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac!

Thanks for the ticket, sorry it took so long for someone to get back to you.

Moving to 6.2 along with #47164 to get more eyes on both tickets and hopefully resolve them.

[cadic]

I've performed a test across various core functions and was able to reproduce the issue with multiple approaches:

<?php
require_once ABSPATH . WPINC . '/class-wp-network.php';
require_once ABSPATH . WPINC . '/class-wp-site.php';

$test_array = array(
        'post_title' => 'Post Title',
        'post_type'  => 'page',
        "\0"         => 'Nullbyte',
);

$test_object = (object) $test_array;

/**
 * Each of these result in a Fatal Error:
 * Cannot access property starting with "\0"
 */
sanitize_post( $test_object );
new WP_Comment( $test_object );
new WP_Network( $test_object );
new WP_Post( $test_object );
new WP_Term( $test_object );
map_deep( $test_object, 'absint' );
new WP_Site( $test_object );

[cadic]

Could be solved by replacing get_object_vars() with a wrapper function

<?php
function wp_get_object_vars( $object ) {
        return array_filter(
                get_object_vars( $object ),
                function( $key ) {
                        return ord( $key ) !== 0;
                },
                ARRAY_FILTER_USE_KEY
        );
}

Trac ticket: https://core.trac.wordpress.org/ticket/52738#comment:4

[mukesh27]

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.

needs-testing added as PR need some testing.

[costdev]

This ticket was discussed in the bug scrub and it was agreed to move this ticket to 6.3 and to reduce the severity to normal. Hopefully we'll land this one next cycle.

[oglekler]

@costdev @audrasjb @SergeyBiryukov @mukesh27, sorry for pinging you all, but can you place look at this patch. The new function looks logical and in WordPress tradition to replace PHP functions with owns to make them more predictable with result, it is covered with unit tests; get_object_vars() isn't replaced for the new wp_get_object_vars() everywhere, but perhaps it is not needed.

@bitcomplex, what is the scenario when get_object_vars() gets an object which has null as a property? It looks like a mistake from some other place we are covering for, but fatal error is not pleasant.

I am also wondering if we need to add an error message to the debug log in cases when it is actually happening to make this easier to debug for developers who are making such a mistake.

[costdev]

Thanks for the ping @oglekler! I meant to get a closer look at this sooner but only ever had a cursory look during scrubs or when taking a look at test failures on the map_deep() ticket.

Sergey was able to reproduce the issue on the map_deep() ticket, and ​here's a 3v4l that might help to visualise the issue and the proposed patch.

My thoughts on whether to patch this:

• Is the value containing NUL bytes produced by Core?
  • If the answer is no, then this is an enhancement request to add support for NUL bytes, not a defect (bug) report, and should be moved out of the 6.3 milestone as we're now in Beta. (Note: A Fatal Error doesn't necessarily mean a bug in Core, it could just mean that PHP says "no" to something extenders are trying to do with Core)
• Is the use case valid?
  • If so, we should continue.
  • If not, we should consider closing this as invalid as the issue should be rectified where the value is generated and/or stored.

Without more information, it's hard to classify this ticket before deciding the right course of action. I'd suggest adding reporter-feedback so we have the information necessary to move forward.

[bitcomplex]

As the reporter of the bug; I'm still convinced this is a serious bug in core. It still affect us every release and we have to patch it manually.

The real issue is when you serialize objects and later change the visibility of a property in the class the object belongs too. Since you've decided that it is a good idea to store serialized objects you should also handle changes of classes in a way that do not cause fatals.

It is impossible to fix the old objects that are permanently stored in the database, but fixing this by ignoring null bytes upon reading, handles the issue perfectly.

Replying to costdev:

Thanks for the ping @oglekler! I meant to get a closer look at this sooner but only ever had a cursory look during scrubs or when taking a look at test failures on the map_deep() ticket.

Sergey was able to reproduce the issue on the map_deep() ticket, and ​here's a 3v4l that might help to visualise the issue and the proposed patch.

My thoughts on whether to patch this:

• Is the value containing NUL bytes produced by Core?
  • If the answer is no, then this is an enhancement request to add support for NUL bytes, not a defect (bug) report, and should be moved out of the 6.3 milestone as we're now in Beta. (Note: A Fatal Error doesn't necessarily mean a bug in Core, it could just mean that PHP says "no" to something extenders are trying to do with Core)
• Is the use case valid?
  • If so, we should continue.
  • If not, we should consider closing this as invalid as the issue should be rectified where the value is generated and/or stored.

Without more information, it's hard to classify this ticket before deciding the right course of action. I'd suggest adding reporter-feedback so we have the information necessary to move forward.

[oglekler]

@bitcomplex, where do these objects with broken properties are coming from? If WordPress, or plugin or theme is changing data structure, there is a migration in place which checks the current version stored in the DB and compares it with the one it needs and if it is not the same runs a migration. So, data is transformed once, new tables created etc. whatever it needs and then a new version is stored in the options to indicate current state and there are no more problems.

[bitcomplex]

@oglekler our own plugin, where some old classes (which is core business logic) are stored as meta data to posts.

I'm 100% sure that we havn't followed any WordPress migration principles for the last 5 years or so, but the point is that you should not have a system that can throw fatals where there is even a theoretical chance of it happening in the wild.

To me WordPress is just broken. Since the stored data is permanent it is enough to forget following any wp guidelines even once to have the object always breaking everything if it loads.

[costdev]

Certainly, Core's implementation in map_deep(), for example, doesn't take an (object) (array) $object scenario into account, nor does its documentation exclude that scenario. In that regard, we could do with improving the implementation to account for this.

While introducing and implementing wp_get_object_vars() should cover that scenario, I'm aware that this also enables extenders to forego good practice of following migration principles.

I've left a review on ​PR 3607 to tidy things up and meet Core standards/convention should it get support for inclusion in Core. Adding changes-requested to indicate the current status of the patch.

If it does get support once additional Core developers have weighed in, then as @oglekler mentions, there are other instances of get_object_vars() in Core that haven't been changed to use wp_get_object_vars(). As the PR adds tests for each of the changed calls, we may either want to do everything at once, or follow up with additional patches to tackle the rest where applicable.

[costdev]

@bitcomplex The real issue is when you serialize objects and later change the visibility of a property in the class the object belongs too. Since you've decided that it is a good idea to store serialized objects you should also handle changes of classes in a way that do not cause fatals.

While there's room for improvement in handling cases such as (object) (array) $object, can you clarify who "you" refers to in each of these so that it's clear to myself and others?

1. [you] serialize objects and [you] later change the visibility of a property in the class the object belongs too
2. [you've] decided that it is a good idea to store serialized objects
3. [you] should also handle changes of classes in a way that do not cause fatals

Where "you" refers to Core doing something, can you also provide more information about when Core does this? Thanks!

[chaion07]

Thanks @bitcomplex for reporting this. We reviewed this ticket during a recent bug-scrub session and based on the feedback we are updating the milestone to 6.4. Cheers!

Props to @mukesh27

[marybaum]

I'll test the patch this week and see what heeds to happen afterward.

[marybaum]

@beapi will test the patch, pending some information from @bitcomplex. That supersedes my needing to test the patch. Thanks, Clement!

[bitcomplex]

Replying to marybaum:

@beapi will test the patch, pending some information from @bitcomplex. That supersedes my needing to test the patch. Thanks, Clement!

What information do you need from me?

[costdev]

This ticket was discussed in the bug scrub. It was agreed to move this back to Awaiting Review until we can determine whether this is a bug in Core, a situation of incorrect usage, or an enhancement request. The ticket can be milestoned if this is established and becomes actionable.

Per a suggestion in the bug scrub, pinging @bitcomplex to take a look back at this comment and respond.

[bitcomplex]

Replying to costdev:

While there's room for improvement in handling cases such as (object) (array) $object, can you clarify who "you" refers to in each of these so that it's clear to myself and others?

1. [you] serialize objects and [you] later change the visibility of a property in the class the object belongs too

1. WordPress serialize objects and 3rd party later change the visibility of a property in the class the object belongs too

2. [you've] decided that it is a good idea to store serialized objects

2. WordPress developers have decided that it is a good idea to store serialized objects

3. [you] should also handle changes of classes in a way that do not cause fatals

3. WordPress developers should also handle changes of classes in a way that do not cause fatals. (This does not absolve 3rd party developers from not following WP-guidelines. But not following guidlines should not cause fatals).