WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17850 closed defect (bug) (fixed)

XMLRPC API Clients can't edit underscore-prefixed custom fields

Reported by: redsweater Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.1.3
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description

Changes that were committed in r17994 changed the behavior of the XMLRPC API such that clients who attempt to create or modify custom field values that start with a "_" underscore, encounter a quiet failure for those values to stick.

It seems at least somewhat common for 3rd party plugins and themes to use underscore-prefixed custom field values, in particular when they provide their own UI for setting those values, and don't want the values to show up in the custom fields UI of the WordPress admin panel.

Because a remote client using the XMLRPC API doesn't cause the plugin's custom code or UI to appear, we need the ability to continue editing these fields via the API.

A concrete example of this problem in action is the All In One SEO pack, which uses custom fields that have a prefix of _aiseop. My customers (MarsEdit) who have custom fields configured to edit these values are finding since 3.1.3 that the specified values simply "don't take" on the server. I have traced this to the changes in r17994:

http://core.trac.wordpress.org/changeset/17994

Particularly to the new is_protected_meta which counts as protected any custom fields that begin with an underscore. Presumably this function ends up getting called on behalf of the API from the "edit_post" API call in post.php, but I haven't confirmed the exact call chain.

It would be great if the XMLRPC API could continue having unfettered access to editing underscore-prefixed custom fields.

Attachments (20)

17850.diff (710 bytes) - added by ryan 3 years ago.
17850.2.diff (3.1 KB) - added by ryan 3 years ago.
17850.3.diff (3.1 KB) - added by ryan 3 years ago.
Reverse order of checks
17850.4.diff (4.2 KB) - added by ryan 3 years ago.
Avoid add_meta()
17850.5.diff (4.3 KB) - added by ryan 3 years ago.
17850.6.diff (5.7 KB) - added by ryan 3 years ago.
All underscore prefixed are protected unless sanitizer is registered
17850.7.diff (7.1 KB) - added by ryan 3 years ago.
register_meta() and edit_post_meta cap, proof of concept.
17850.8.diff (12.0 KB) - added by ryan 3 years ago.
get_metadata_by_id(), error checking in register_meta(), barely tested
17850-tests.diff (1.4 KB) - added by ryan 3 years ago.
17850.9.diff (13.6 KB) - added by ryan 3 years ago.
17850.10.diff (13.6 KB) - added by ryan 3 years ago.
s/create_user_meta/add_user_meta/
17850.11.diff (13.5 KB) - added by ryan 3 years ago.
17850.12.diff (13.5 KB) - added by ryan 3 years ago.
17850.13.diff (13.1 KB) - added by ryan 3 years ago.
17850.14.diff (13.1 KB) - added by ryan 3 years ago.
17850.15.diff (13.1 KB) - added by ryan 3 years ago.
17850.16.diff (13.0 KB) - added by ryan 3 years ago.
register-meta-test.php (345 bytes) - added by ryan 3 years ago.
Allows editing _foo custom field through XML-RPC
17850.17.diff (2.4 KB) - added by xknown 3 years ago.
Do not call stripslashes two times and add verify if the new meta_key is not protected when editing an existing meta
17850.18.diff (2.4 KB) - added by ryan 3 years ago.

Download all attachments as: .zip

Change History (56)

comment:1 daniloercoli3 years ago

  • Cc ercoli@… added
  • Keywords mobile added

ryan3 years ago

comment:2 ryan3 years ago

Patch more selectively marks meta as protected.

comment:3 ryan3 years ago

  • Milestone changed from Awaiting Review to 3.2

ryan3 years ago

comment:4 ryan3 years ago

Introduces is_hidden_meta(). Prevents both hidden and protected from being updated by the ajax handlers and the edit form handler. Allows hidden to be updated by delete_meta() and update_meta(). Introduces a new prefix, '__', that plugins can use to protect their meta.

Last edited 3 years ago by ryan (previous) (diff)

comment:5 redsweater3 years ago

Ryan - thanks for taking this bug and running with it.

I applied the patch and it doesn't address the crux of the problem, that XMLRPC clients are unable to update '_' prefixed custom fields.

Is it possible that in post.php it was a mistake to add the is_hidden_meta( $meta->meta_key ) clause to the tests that bail out on editing or deleting a custom field value?

Last edited 3 years ago by redsweater (previous) (diff)

comment:6 nacin3 years ago

Should is_hidden_meta() assume is_protected_meta(), perhaps calling it internally (purpose being the filter, I imagine), rather than requiring the double check?

comment:7 ryan3 years ago

As I finished the patch, I considered doing that and collapsing is_protected_meta() || is_hidden_meta() into just is_hidden_meta(). Could go ahead with that if it sounds good to you.

Last edited 3 years ago by ryan (previous) (diff)

comment:8 ryan3 years ago

  • Keywords has-patch added

comment:9 ryan3 years ago

Although, I kinda prefer retaining is_protected_meta() || is_hidden_meta() for clarity.

comment:10 nacin3 years ago

  • Milestone changed from 3.2 to 3.1.4

3.1.4 for tracking purposes.

is_hidden_meta() is a superset of is_protected_meta(). It will always return true whenever is_protected_meta() returns true. (In which case, we should probably reverse the checks so is_hidden_meta() comes first.) That said, there's an exception: When the protected filter is used to add a meta key there. I'm not sure we should expect them to know they need to filter it in both spots.

ryan3 years ago

Reverse order of checks

comment:11 nacin3 years ago

  • Keywords commit added

After IRC discussion, +1 from me.

ryan3 years ago

Avoid add_meta()

comment:12 redsweater3 years ago

I can confirm that the 17580.4.diff does seem to address the original problem. I was able to edit a custom field with a leading underscore, and it stuck across refreshes of the blog content.

comment:13 xknown3 years ago

As I said in the mail, it would be better if all the internal WP meta keys are standardized. At some point in the future, WordPress will probably have security issues when someone will add a new meta key that is not protected in is_protected_meta. This already happened with the previous approach.

Just to cite one example, with the latest patch (17850.4.diff), WP is vulnerable to persistent XSS attacks because the '_oembed_MD5...' is not covered in is_protected_meta -- an user with the edit_posts capability can also use the XMLRPC API.

For 3.2, we can maybe use your approach, but for 3.3 it would be good to have a strong solution to this problem.

comment:14 nacin3 years ago

  • Keywords commit removed

Removing commit. You're right, things like _oembed_ absolutely need to be protected.

ryan3 years ago

comment:15 ryan3 years ago

That adds _oembed_*. A search across the repo for update_post_meta, *_post_custom, and update_metadata suggests that was the only post meta key that was missed. User and comment meta need a look.

comment:16 ryan3 years ago

This stuff is a mess. Editing through either XML-RPC or the post custom meta box is fraught with peril. I think we need to leave all underscore prefixed meta items as protected from XML-RPC and the custom meta box. Plugins really need to register a sanitizer to make these safe to edit.

ryan3 years ago

All underscore prefixed are protected unless sanitizer is registered

comment:17 ryan3 years ago

To do this right I think we need register_meta() API that allows registering sanitize and auth callbacks. Then use current_user_can('edit_post_meta', $meta_key, $post_id). edit_post_meta would be a meta cap that checks the edit_post cap and also checks for an auth callback for the particular meta key.

ryan3 years ago

register_meta() and edit_post_meta cap, proof of concept.

comment:18 xknown3 years ago

The latest two patches seem good. Both have a little type in wp-includes/post-template.php, it should be is_hidden_meta( $key ), an not is_hidden_meta( $keyt ). Also the code to protect delete_meta at wp_xml_rpc_server may be theoretically bypassed -- one can just send the mid of a protected meta with any random meta key.

Regarding 17850.7.diff, adding a is_callable check would also be good. Will you intend to push this for 3.2? I think it should be tested extensively.

comment:19 ryan3 years ago

That patch is not even close to finished. It is just a proof-of-concept along one code path. I think this is too much for 3.2. I suggest doing this early in 3.3, letting it soak a bit, and then back porting to 3.2.1. A survey of a few plugin authors indicates the expectation is that underscore prefixed meta keys should not be editable by xml-rpc. Hopefully this means not too many will be hampered by these remaning protected awhile longer.

In conclusion, opening up underscore meta without introducing some cap and sanitize hooks is just opening the security hole back up, whether it be for protected core meta or protected plugin meta. Since registering cap and sanitize hooks is too big an undertaking for 3.2, I suggest punting to 3.3.

comment:20 azaozz3 years ago

  • Keywords 3.3-early added

Agree with @ryan on this. Seems too big a change to go in now. Also this whole API/UI would probably need some re-thinking in 3.3. I personally don't see a point of the meta postbox at all (but that's for another discussion).

comment:21 nacin3 years ago

  • Keywords punt added

I like 17850.6.diff for 3.2. I think it's clever.

But thinking about it some more though, I don't know if having a callback should be enough to un-protect metadata. Come 3.3 when we introduce register_meta(), we're going to want to undo that, and have some sort of a visibility setting.

Maybe we just leave everything as is, and get a plugin like AIOSEOP to add a filter to unprotect certain meta. That then means those meta pieces are restricted by the same capabilities as editing the rest of the post.

comment:22 ryan3 years ago

  • Milestone changed from 3.1.4 to Future Release

comment:23 ryan3 years ago

  • Milestone changed from Future Release to 3.3

ryan3 years ago

get_metadata_by_id(), error checking in register_meta(), barely tested

ryan3 years ago

ryan3 years ago

ryan3 years ago

s/create_user_meta/add_user_meta/

ryan3 years ago

comment:26 nacin3 years ago

Our future selves will want to know that today's IRC logs have extensive conversations on this API. Wanted to drop a note before I forgot.

ryan3 years ago

ryan3 years ago

ryan3 years ago

ryan3 years ago

ryan3 years ago

ryan3 years ago

Allows editing _foo custom field through XML-RPC

comment:28 ryan3 years ago

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

In [18445]:

Introduce register_meta(), get_metadata_by_mid(), and *_post_meta capabilities. fixes #17850

comment:29 xknown3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think I missed the party :)

Currently this protection can be easily bypassed in two different ways using the ajax or xmlrpc api. I am able for example to add the _wp_attached_file meta to some post. I describe the steps to reproduce the problems using the ajax api.

  • Create a new meta key, for example "foo" using the post editor. Then, rename this meta key to _wp_attached_file.
  • Create a new meta with the following key \_wp_attached_file. The stripslashes function is called to times when adding creating a new meta with the add_meta function.
Last edited 3 years ago by xknown (previous) (diff)

xknown3 years ago

Do not call stripslashes two times and add verify if the new meta_key is not protected when editing an existing meta

comment:30 scribu3 years ago

  • Keywords mobile 3.3-early punt removed

comment:31 xknown3 years ago

About the problem of calling stripslashes two times, I don't like the "fix" I proposed. It assumes that the data is properly escaped when calling add_meta, if not, then there will be problems.

ryan3 years ago

comment:32 ryan3 years ago

add_meta() was double stripping and calling maybe_serialize() too early. 17850.18.diff is a derivation of 17.diff. It retrains the stripping of the key so that is_protected_meta() and current_user_can() have the proper key to work with. esc_sql() adds the slashes back prior to calling add_post_meta().

comment:33 xknown3 years ago

That's better. I was thinking to do the same, but I got distracted.

comment:34 ryan3 years ago

In [18449]:

Check caps for both old and new meta keys when changing the key for a mid. Properly handle slashes when checking meta caps. Props xknown. see #17850

comment:35 ryan3 years ago

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

comment:36 jane3 years ago

  • Component changed from General to XML-RPC
Note: See TracTickets for help on using tickets.