Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#57670 closed defect (bug) (fixed)

Invalid return type for wp_get_raw_referer()

Reported by: xknown's profile xknown Owned by: johnbillion's profile johnbillion
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.6
Component: General Keywords: php81 has-patch has-unit-tests commit
Focuses: Cc:

Description

The return type for wp_get_raw_referer indicates it returns string|false. However, this is not the case if the user provided $_REQUEST['_wp_http_referer'] variable is an array for example.

This results in PHP 8.x fatals in code that assumes this function only returns strings/booleans.

strpos( wp_get_raw_referer(), 'foo' );

https://3v4l.org/CoekE

Change History (13)

This ticket was mentioned in PR #4034 on WordPress/wordpress-develop by xknown.


2 years ago
#1

  • Keywords has-patch has-unit-tests added

#2 @costdev
2 years ago

  • Version set to 4.6

#3 @johnbillion
2 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to johnbillion
  • Status changed from new to accepted

xknown commented on PR #4034:


2 years ago
#4

The patch looks good to me.

I am left wondering about two things though:

  1. Is the _wp_http_referer REQUEST variable documented anywhere ? And if so, is that documentation clear enough that this should always be set to a string ? And if not documented, is there anywhere we can add documentation about this ?
  1. Can we think of any legitimate reasons for the variable to be set to an array ? The only thing I can come up with is a redirect-chain.

I don't think it's a documentation problem, or developers using it on the wrong way. We noticed this issue because there were lots of PHP fatals related to people running automated security tools.

@jrf commented on PR #4034:


2 years ago
#5

  1. Can we think of any legitimate reasons for the variable to be set to an array ? The only thing I can come up with is a redirect-chain.

`

I don't think it's a documentation problem, or developers using it on the wrong way. We noticed this issue because there were lots of PHP fatals related to people running automated security tools.

Well, servers don't set that variable by default, so it gets set somewhere.... whether it is by WP Core, by a plugin or by an automated security tool. So, what I'm really asking is, can we identify in some way, what code is setting the variable to an array and for what purpose.
Based on _this_ function's documentation, setting the variable to an array is _doing it wrong_, but I'm wondering if there are legitimate reasons for that and what the source is (and if it is Core, we need to fix Core or decide that arrays are allowed).

#6 @johnbillion
21 months ago

  • Keywords commit removed

#7 @costdev
21 months ago

Hi @xknown,

Some questions:

  1. Have you reproduced this in Core's default functionality, or is it only when using automatic security tools?
  2. What code produced the array for _wp_http_referer? (link if possible)

Thanks!

#8 @xknown
21 months ago

Hi @costdev!

Have you reproduced this in Core's default functionality, or is it only when using automatic security tools?

I didn't test it in core, but it took me less than five minutes to find an example:
'http://WP.site/wp-login.php?action=postpass&_wp_http_referer[]='.

Fatal error: Uncaught TypeError: trim(): Argument #1 ($string) must be of type string, array given in /var/www/html/wp-includes/pluggable.php:1558
Stack trace:
#0 /var/www/html/wp-includes/pluggable.php(1558): trim(Array, ' \t\n\r\x00\x08\v')
#1 /var/www/html/wp-includes/functions.php(1962): wp_validate_redirect(Array, false)
#2 /var/www/html/wp-login.php(724): wp_get_referer()
#3 {main}
  thrown in /var/www/html/wp-includes/pluggable.php on line 1558

And even if we can only reproduce it via security tools, I think this is something we need to fix.

What code produced the array for _wp_http_referer? (link if possible)

It's likely somewhere in the PHP internals, but I don't think it's relevant for this ticket.

Last edited 21 months ago by xknown (previous) (diff)

#9 @costdev
21 months ago

Thanks @xknown!

My understanding is that _wp_http_referer, like HTTP_REFERER, should always be a string. I haven't found any references to it being acceptable or used as an array.

So if the issue is just that wp_get_referer() isn't enforcing its documented return types, then the PR looks good to me! - I just left a comment with a small change to the test method's name.

#10 @azaozz
21 months ago

  • Keywords commit added

even if we can only reproduce it via security tools, I think this is something we need to fix

+1. Thanks for catching this and for the PR!

My understanding is that _wp_http_referer, like HTTP_REFERER, should always be a string.

Yep, that's right. As far as I know _wp_http_referer is intended as a replacement/override for HTTP_REFERER and is used for redirects (in combination with wp_safe_redirect()). It can either be a string or not set.

Last edited 21 months ago by azaozz (previous) (diff)

@azaozz commented on PR #4034:


21 months ago
#11

Yep, more/better docs are always a big plus! The question is: where would be a good place to add them :)

servers don't set that variable by default, so it gets set somewhere...

Right. It gets set in few places, mostly through wp_nonce_field() (which is used a lot) that calls wp_referer_field() that returns a hidden HTML input element.

Seems the docblock for wp_referer_field() is the most appropriate place although not sure how "visible" it would be as that function is rarely used on its own. So perhaps can add the docs in two places: the docblocks for wp_referer_field() and for wp_get_raw_referer()?

#12 @johnbillion
20 months ago

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

In 56115:

General: Ignore invalid types for the '_wp_http_referer' URL query variable.

It's expected that this query variable contains a string when it's set, but it's possible for its type to be something else such as an array. Ignoring non-string values prevents cascading errors when its value is passed through functions that expect a string.

Props xknown, costdev, jrf, azaozz

Fixes #57670

Note: See TracTickets for help on using tickets.