WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#27152 closed defect (bug) (fixed)

wp_get_referer() no longer reports off-site referrers

Reported by: bpetty Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.6.1
Component: Bootstrap/Load Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

In r25318, a redirect validation was added to wp_get_referer() and wp_get_original_referer() by @nacin (there's no ticket for this change btw).

The problem here is that this has broken calls to these functions with the purpose of simply fetching the referrer for logging or stats, and not necessarily for redirection. This is a silent failure since callers were already expected to handle a false return value, and now off-site referers return false as well.

This is a regression from 3.6.

We shouldn't just assume that a call to these methods are strictly for the purpose of redirection, and leave the responsibility of validating the URL for redirection up to the method actually performing the redirection. We have wp_safe_redirect() for this, and if this is required for form actions or elsewhere, they should be handling validation themselves.

Attachments (4)

27152.diff (831 bytes) - added by voldemortensen 5 years ago.
27152.2.diff (1.4 KB) - added by swissspidy 4 years ago.
27152.3.diff (2.8 KB) - added by swissspidy 4 years ago.
27152.4.diff (3.4 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (17)

#1 @bpetty
6 years ago

  • Keywords needs-unit-tests removed

There's at least one assertion that can check this on the latest patch submitted to #19856 (ticket-19856-referer.3.patch). Could just apply the unit tests portion of that for tests that cover this.

#2 follow-up: @nacin
6 years ago

  • Version changed from 3.7 to 3.6.1

This was security hardening to avoid open redirection vulnerabilities. Too often core and plugins were using wp_get_referer() without wp_safe_redirect(). The addition of wp_validate_redirect() was very deliberate and was signed off by the security team and the lead developers.

This function is specifically a WordPress utility, given its checking of the _wp_http_referer field. (If you wanted HTTP_REFERER, you should really ask for it.) That, combined with the unreliability of referrers, the danger of un-validated referrers, and the nature of it already allowing for a silent failure, made it a pretty obvious decision.

This is a wontfix. It was a bug for this function to ever return a value without first validating it. wp_validate_redirect() has a filter.

#3 in reply to: ↑ 2 @bpetty
6 years ago

Just for the sake of argument since this never saw any public debate or discussion, and since what was apparently such a significant oversight is now fixed anyway, I'd appreciate some feedback on this. It doesn't seem like the appropriate response to me, and I'd like to clear my concerns up before something like this happens again without notice.

I wouldn't call it a "bug", it was doing it's job. The code using it might have been incorrect, but that's not the fault of wp_get_referer() (which is appropriately documented with the correct usage on the Codex).

Why was it implausible to fix any incorrect usage in core? I'm fairly certain it didn't need fixed in more than a few places, but even if it was, I know there wasn't more than a maximum of about 50 locations in core that would have needed a review, and presumably, the reporter (or exploit) would have pointed out the location(s) to be concerned about.

How do you know how frequently it's used incorrectly in plugins? Do you have some numbers?

It just seems to me like this has diminished the reason for even having wp_safe_redirect() as well as leaving a very generically named wp_get_referer() method with only one single use case (redirecting form submissions), and no longer able to serve it's more globally identified purpose of (possibly more reliably) reporting where the user was referred from; something it was perfectly able to do before, and something the name of the method still indicates. This really just means that developers will be using wp_get_referer() incorrectly now instead, so I wouldn't really call it a "pretty obvious decision".

This could have simply been resolved with better documentation and advocating better security practices including providing better examples of proper use in core code (which will never be fixed up to use wp_safe_redirect() now since there is no point).

Part of my concern here is also that this was much more of an API design decision made by the security team rather than actually fixing security holes at their real points of exposure, all while ignoring the same advice we're still telling plugin developers. Normally this wouldn't even bother me if it was a good, quick fix for a serious issue that would have involved way more work to fix properly, but needed fixed immediately. This wan't one of those cases though.

#4 @nacin
6 years ago

The security team includes a number of lead developers (well, all of them) as well as others who are known for their security and API acumen like dd32 and duck_. Everyone signed off on this change. These changes are not ones we make lightly, whether in public or in private. In this case, we also factored in that dropping a referer would not be critical, especially given how much they are already dropped in practice.

You may be right that it has diminished the need to use wp_safe_redirect(). The fact of the matter was that core was doing it wrong something like 40% of the time. If that's our track record, then we cannot expect a plugin developer to be any better. Sometimes we go with the scalpel; in this case, we chose the hammer.

#5 @nacin
6 years ago

  • Milestone changed from 3.9 to Awaiting Review
  • Severity changed from major to normal

I could see wp_get_referer() gaining a counterpart, like wp_get_raw_referer() or wp_get_unsafe_referer(), or wp_get_referer(bool).

@voldemortensen
5 years ago

#6 @voldemortensen
5 years ago

  • Keywords has-patch added

#7 @chriscct7
4 years ago

  • Keywords needs-unit-tests added

@swissspidy
4 years ago

#8 @swissspidy
4 years ago

  • Milestone changed from Awaiting Review to 4.5

27152.2.diff is an updated patch that makes things more DRY by using wp_get_raw_referer in wp_get_referer.

Unit tests for wp_get_referer are being suggested in #19856. Perhaps those are already enough, because a part of the function is simply extracted into a new one. Otherwise it's easy to add like 2 new tests for wp_get_raw_referer alone.

@swissspidy
4 years ago

#9 @swissspidy
4 years ago

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

#10 @swissspidy
4 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

@swissspidy
4 years ago

#11 @swissspidy
4 years ago

27152.4.diff adds a note regarding wp_get_referer() in the new function's docblock and always resets the super globals on setUp(). Locally, tests suddenly failed because Tests_WP_Customize_Manager::test_return_url() sets $_SERVER['HTTP_REFERER'].

#12 @swissspidy
4 years ago

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

In 36266:

Introduce wp_get_raw_referer() to retrieve unvalidated referer.

For things like redirects wp_get_referer() should be used instead.

Props voldemortensen for initial patch.
Fixes #27152.

#13 @DrewAPicture
4 years ago

In 36500:

Docs: Make a couple of minor improvements to the DocBlock for wp_get_raw_referer(), introduced in [36266].

  • Uses a third-person singular verb in the summary
  • Makes the return types more specific with string|false vs string|bool.

See #27152. See #32246.

Note: See TracTickets for help on using tickets.