WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 3 years ago

#17737 new defect (bug)

Be better at forcing data types for query vars

Reported by: juliobox Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch 3.9-early
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 (6)

17737.patch (519 bytes) - added by xknown 7 years ago.
urldecode returns a string, move the cast to the right place
17737.2.patch (1.5 KB) - added by SergeyBiryukov 7 years ago.
17737.3.patch (4.4 KB) - added by SergeyBiryukov 5 years ago.
17737.4.patch (778 bytes) - added by SergeyBiryukov 5 years ago.
17737.5.patch (7.0 KB) - added by SergeyBiryukov 5 years ago.
17737.diff (4.4 KB) - added by wonderboymusic 5 years ago.

Download all attachments as: .zip

Change History (35)

#1 @nacin
7 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
7 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
7 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
7 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
7 years ago

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

#5 @nacin
7 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
7 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
5 years ago

  • Milestone changed from Awaiting Review to 3.6

I think 's' also throws a warning.

#8 @SergeyBiryukov
5 years ago

#23425 was marked as a duplicate.

#9 @SergeyBiryukov
5 years ago

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

#10 follow-up: @dave1010
5 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
5 years ago

  • Milestone changed from 3.6 to Future Release

#12 @wonderboymusic
5 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_
5 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
5 years ago

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

#15 @SergeyBiryukov
5 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
5 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
5 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
5 years ago

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

#19 @johnbillion
5 years ago

  • Keywords needs-refresh added

@wonderboymusic
5 years ago

#20 @wonderboymusic
5 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
5 years ago

  • Keywords needs-refresh removed

#22 @nacin
5 years ago

  • Milestone changed from 3.7 to 3.8

Let's make sure we get this right.

#23 @SergeyBiryukov
5 years ago

#26308 was marked as a duplicate.

#24 @wonderboymusic
5 years ago

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

#25 @SergeyBiryukov
5 years ago

#26583 was marked as a duplicate.

#26 @IwanLuijksQuestMedia
4 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 4 years ago by IwanLuijksQuestMedia (previous) (diff)

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


4 years ago

#29 @juliobox
3 years ago

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

Note: See TracTickets for help on using tickets.