Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#49705 new defect (bug)

Sanitizing input for parameterized queries + update_meta_cache

Reported by: classicalrehan's profile classicalrehan Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 5.3.2
Component: Options, Meta APIs Keywords:
Focuses: coding-standards Cc:

Description

So, should we just pass anything we get straight to the database?

There are definitely things you can check about user input, but this is highly context-dependent. Because sanitization is ill-defined and mis-used, I prefer to call this validation.

I checked the WordPress core function which is not sanitized even this is one of the most used function in across the WordPress.

In below code get_result with no prepare statement. I don;t know the reason why?

$meta_list = $wpdb->get_results( "SELECT $column, meta_key, meta_value FROM $table WHERE $column IN ($id_list) ORDER BY $id_column ASC", ARRAY_A );

This function should be something like this:

$id_list_sanity = implode( ', ', array_fill( 0, count( $id_list ), '%d' ) );
                $meta_list = $wpdb->get_results( $wpdb->prepare( "
                   SELECT $column, meta_key, meta_value FROM $table WHERE $column IN ($id_list_sanity) ORDER BY $id_column DESC"
                ,$id_list),ARRAY_A );

Function Name: update_meta_cache
File: wp-includes/meta.php
Line: #825

Change History (3)

#1 @SergeyBiryukov
4 years ago

  • Component changed from Query to Options, Meta APIs
  • Focuses rest-api performance removed

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

Just noting that $id_list is constructed from the function's $object_ids parameter, which is sanitized using intval() earler.

#2 follow-up: @classicalrehan
4 years ago

Hi @SergeyBiryukov thanks for the reply, I just wanted to know why get_results query not used here wit prepare statement?

#3 in reply to: ↑ 2 @dd32
3 years ago

  • Severity changed from critical to minor

Replying to classicalrehan:

I just wanted to know why get_results query not used here wit prepare statement?

To follow up on this old comment - Because prepare isn't necessarily useful here, as intval() is going to be just as secure as sprintf( '%d' ) which is what prepare is and is simpler to read than an array_fill() call.

If WordPress was using proper native prepared queries, then it would make more sense to use it.

Note: See TracTickets for help on using tickets.