WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#7141 closed defect (bug) (wontfix)

pluggable.php: auth_redirect() contains invalid test for SSL request

Reported by: wet Owned by:
Milestone: Priority: low
Severity: normal Version: 2.6
Component: Security Keywords: has-patch tested
Focuses: Cc:

Description

auth_redirect() tries to locate 'http' in $_SERVER['REQUEST_URI'] to determine whether SSL is used:

if ( 0 === strpos($_SERVER['REQUEST_URI'], 'http') ) {
	wp_redirect(preg_replace('|^http://|', 'https://', $_SERVER['REQUEST_URI']));

$_SERVER['REQUEST_URI'] won't contain any protocol part, so this test will not work.

The alternate code path apparently builds yet another SSL admin URI.

Attachments (1)

7141.diff (589 bytes) - added by Denis-de-Bernardy 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 @wet7 years ago

  • Component changed from General to Security
  • Priority changed from normal to high
  • Severity changed from normal to major

Wouldn't this need to be fixed for 2.6?

comment:2 @DD327 years ago

  • Keywords needs-patch added
  • Milestone set to 2.6
  • Version set to 2.6

comment:3 @ryan7 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Some setups do have the protocol in REQUEST_URI. This is just a cover your ass check. It doesn't hurt anything.

comment:4 @thee177 years ago

  • Milestone 2.6 deleted

comment:5 follow-up: @grotfl16 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

This is still a bug, but for a different reason. Instead of this line:

if ( 0 === strpos($_SERVER['REQUEST_URI'], 'http') ) {

There should be this one:

if ( 0 === strpos($_SERVER['REQUEST_URI'], 'http:') ) {

Otherwise there would be a redirect loop (because 'https:/...' also starts with 'http'!).

I stumbled across this because I did have a redirect loop (though this was not the reason for it). I guess there aren't many setups that would have protocol in REQUEST_URI or this bug would have surfaced a long time ago. ;)

Btw, there are two such checks in auth_redirect(). IMHO they should both be removed.

comment:6 @Denis-de-Bernardy6 years ago

  • Milestone set to 2.8
  • Priority changed from high to low
  • Severity changed from major to normal

@Denis-de-Bernardy6 years ago

comment:7 @Denis-de-Bernardy6 years ago

  • Keywords has-patch tested added; needs-patch removed

comment:8 in reply to: ↑ 5 @ryan6 years ago

  • Milestone 2.8 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Replying to grotfl1:

This is still a bug, but for a different reason. Instead of this line:

if ( 0 === strpos($_SERVER['REQUEST_URI'], 'http') ) {

There should be this one:

if ( 0 === strpos($_SERVER['REQUEST_URI'], 'http:') ) {

Otherwise there would be a redirect loop (because 'https:/...' also starts with 'http'!).

I stumbled across this because I did have a redirect loop (though this was not the reason for it). I guess there aren't many setups that would have protocol in REQUEST_URI or this bug would have surfaced a long time ago. ;)

Btw, there are two such checks in auth_redirect(). IMHO they should both be removed.

'http' instead of 'http:' is done on purpose. Since this is inside an !is_ssl() check, I don't see a redirect loop happening.

Note: See TracTickets for help on using tickets.