Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#39945 new defect (bug)

WP_Query::get_posts fails to correctly sanitize 'posts_per_page'

Reported by: biisent's profile biisent Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7.2
Component: Query Keywords: has-patch needs-testing 2nd-opinion
Focuses: Cc:

Description

WP_Query::get_posts fails to correctly sanitize the 'posts_per_page' argument when a negative value in range (-2, -1) is supplied.

Example

The following get_posts query causes an exception:

get_posts(array('posts_per_page' => '-1.5'));

Exception: WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-1' at line 1 for query SELECT wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.post_date DESC LIMIT 0, -1 made by get_posts, WP_Query->query, WP_Query->get_posts

Cause

Incomplete sanitization in WP_Query::get_posts(), line 1775 - 1779:

    $q['posts_per_page'] = (int) $q['posts_per_page'];
    if ( $q['posts_per_page'] < -1 )
        $q['posts_per_page'] = abs($q['posts_per_page']);
    elseif ( $q['posts_per_page'] == 0 )
        $q['posts_per_page'] = 1;


Impact

Some plugins (e.g. Woocommerce) initialize the posts_per_page argument with user supplied values and may suffer from an information disclosure vulnerability, depending on the webserver configuration.

Confirmed on the latest Wordpress version 4.7.2.

First reported at 19.02.2017 to security[at]wordpress.org without response (not nice!), so I assume you do not consider this security relevant in accordance with e.g. https://make.wordpress.org/core/handbook/testing/reporting-security-vulnerabilities/#why-are-there-path-disclosures-when-directly-loading-certain-files

Attachments (1)

39945.diff (1.0 KB) - added by asalce 7 years ago.

Download all attachments as: .zip

Change History (5)

@asalce
7 years ago

#1 @asalce
7 years ago

  • Keywords has-patch needs-testing 2nd-opinion added

@biisent looks like the bigger issue is that post_per_page is allowed to have a negative value... BUT that check was happening before any sanitization is done on the variable. It also looks like the negative value is used when the user wants to query ALL posts. This patch I submitted still will cause an unwanted behaviour for WooCommerce. They should also do some sanitization on their end before passing it to get_posts/WP_Query.

PS. I also modified the check to look for anything less than or equal to -1. So a -2 value will also retrieve all posts. Not sure if that was the correct thing to do, but better safe than sorry. Maybe a hard error should be thrown instead of allowing negative values to continue on after line 1790.

#2 @biisent
7 years ago

Two considerations:

  1. When $this->is_feed == true and $this->is_singular == false, then $q['posts_per_page'] is set to $q['posts_per_rss'] and $q['nopaging'] is set to false in https://core.trac.wordpress.org/browser/branches/4.7/src/wp-includes/class-wp-query.php#L1768 . This however prevents the later sanitization of $q['nopaging'] since it is only performed when !isset($q['nopaging']).
  1. In my opinion, retrieving all posts for negative posts_per_page values seems consistent. However it may break compatibility (?) as currently negative posts_per_page values <= -2 will just become positive.
Last edited 7 years ago by biisent (previous) (diff)

#3 @bernsteina
6 years ago

You can also overflow to get a float.
i.e. -9223372036854775808 on 64-bit servers and -2147483648 on 32-bit

WordPress database error:
[...]
ASC LIMIT 0, 9.2233720368548E+18

#4 @Howdy_McGee
4 years ago

The patch for #24142 should solve this issue, if it isn't solved already. I do not believe it solves the scientific notation issue though, I don't have a good solution for that.

Note: See TracTickets for help on using tickets.