WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 13 months ago

#9207 accepted defect (bug)

redirect_to wp-admin Should Force SSL If FORCE_SSL_ADMIN is enabled

Reported by: g30rg3x Owned by: hakre
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9
Component: Security Keywords: has-patch reporter-feedback
Focuses: Cc:

Description

Around Lines 406 to 426 on wp-login.php:

	$secure_cookie = '';

	// If the user wants ssl but the session is not ssl, force a secure cookie.
	if ( !empty($_POST['log']) && !force_ssl_admin() ) {
		$user_name = sanitize_user($_POST['log']);
		if ( $user = get_userdatabylogin($user_name) ) {
			if ( get_user_option('use_ssl', $user->ID) ) {
				$secure_cookie = true;
				force_ssl_admin(true);
			}
		}
	}

	if ( isset( $_REQUEST['redirect_to'] ) ) {
		$redirect_to = $_REQUEST['redirect_to'];
		// Redirect to https if user wants ssl
		if ( $secure_cookie && false !== strpos($redirect_to, 'wp-admin') )
			$redirect_to = preg_replace('|^http://|', 'https://', $redirect_to);
	} else {
		$redirect_to = admin_url();
	}

As we can see on the present code, if a redirection is set while login and this redirection goes to the plain version of the dashboard then client will go to the non-SSL version of the dashboard which therefore will move the client to the secure version (generating and extra request).
I know this is kinda a tongue twister sentence so i think is better to put a request example of the problem...

Client: POST http://foo.bar/wp-login.php?redirect_to=http%3A%2F%2Ffoo.bar%2Fwp-admin%2Findex.php
Server: HTTP 302 ... Location: http://foo.bar/wp-admin/index.php
Client: GET http://foo.bar/wp-admin/index.php
Server: HTTP 302 ... Location: https://foo.bar/wp-admin/index.php

I know that wordpress is actually working as suppose to work (cause we told to move to non-SSL version of the dashboard) but and a extra http request is issued.
IMHO if we (admins) have enabled FORCE_SSL_ADMIN, then all redirections to wp-admin should go SSL/HTTPs even if we fill redirect_to with the plain version of the dashboard.
There is part of the code that detect this and replace it but it has issues or well it isn't prepared to do this.
At the moment we can filter login_redirect to fix this but (again) IMHO this should move to the core...

Attachments (1)

9207.patch (2.8 KB) - added by hakre 4 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Denis-de-Bernardy5 years ago

  • Component changed from General to Optimization
  • Keywords needs-patch added; 2nd-opinion removed

comment:2 janeforshort5 years ago

  • Milestone changed from 2.8 to Future Release

Punting due to feature freeze. Reconsider with next release.

comment:3 Denis-de-Bernardy5 years ago

  • Priority changed from normal to lowest
  • Severity changed from normal to trivial

comment:4 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9
  • Type changed from defect (bug) to enhancement

comment:5 hakre5 years ago

That code need to stripslash he value because since it was written, $_REQUEST has been changed and is slashed now.

comment:6 hakre4 years ago

  • Owner changed from anonymous to hakre
  • Status changed from new to accepted

I put this ticket on my list at for a secure wordpress setup

comment:7 ryan4 years ago

  • Milestone changed from 2.9 to Future Release

hakre4 years ago

comment:8 hakre4 years ago

  • Component changed from Optimization to Security
  • Keywords has-patch reporter-feedback added; needs-patch removed
  • Milestone changed from Future Release to 3.0
  • Priority changed from lowest to normal
  • Severity changed from trivial to normal
  • Type changed from enhancement to defect (bug)
  • Version set to 2.9

Attached you find a patch against current head that solves the problem.

The routines were infact already in there, only the FORCE_SSL_ADMIN define was not reflected properly. While doing the review I used the time as well to make the code simpler (and therefore less errorprone).

I already tested the patch and it would be great if the original reporter still is available to test if this is fit for the reported case.

The case infact is a bug because it should have been redirected properly already as far I was able to read the code (see the user wants ssl scenario). So this is not optimization nor an enhancement. I relate this to security because SSL is a security layer.

comment:9 hakre4 years ago

Related: #11643

comment:10 hakre4 years ago

(In [12666]) Typo fix in comment. see #11643 -> patch needs an update, typo in comment as well.

comment:11 nacin4 years ago

  • Milestone changed from 3.0 to Future Release

comment:12 keytuna13 months ago

  • Cc keytuna added

Attempted to reproduce issue with version 3.5.1 of WordPress, XAMPP 1.8.1, and Curl 7.29.0.

Used default settings, attempted with this command:

curl -v -X POST http://localhost/wordpress/wp-login.php?redirect_to=http%3A%2F%2Flocalhost%2wordpress%2Fwp-admin%2Findex.php

Expected to see a 302 redirect, found a 200 OK response.

Is anyone else still able to reproduce this issue?

Note: See TracTickets for help on using tickets.