Opened 8 years ago
Last modified 5 years ago
#39945 new defect (bug)
WP_Query::get_posts fails to correctly sanitize 'posts_per_page'
Reported by: | 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)
Change History (5)
#2
@
8 years ago
Two considerations:
- 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 tofalse
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'])
.
- 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.
@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.