WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 5 years ago

Last modified 4 years ago

#10267 closed defect (bug) (fixed)

Login form SSL is confusing

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Security Keywords:
Focuses: Cc:

Description

With ssl_admin off, and ssl_login on, the login form sends a secure POST request. But end-users can be confused into thinking that they're about to send a non-secure post unless they view the page's source code.

The attached patch enforces SSL on the form as well, to avoid this confusion.

Brought this up in IRC, and it gets +1 from Viper007Bond and DD32 as well.

Attachments (1)

10267.diff (1.1 KB) - added by Denis-de-Bernardy 10 years ago.

Download all attachments as: .zip

Change History (27)

#1 @Denis-de-Bernardy
10 years ago

  • Keywords tested removed

mm, needs a bit more work, actually. it ends up returning the dashboard in ssl when logging in.

#2 @Denis-de-Bernardy
10 years ago

the issue is this, in site_url:

$scheme = ( is_ssl() ? 'https' : 'http' );

the entire check might need to look like the following:

if ( 'login_post' == $scheme )
	$scheme = ( force_ssl_login() || force_ssl_admin() ) ? 'https' : 'http';
elseif ( 'login' == $scheme )
	$scheme = ( force_ssl_login() || force_ssl_admin() ) ? 'https' : 'http';
elseif ( ('admin' == $scheme) )
	$scheme = force_ssl_admin() ? 'https' : 'http';
else
	$scheme = is_ssl() ? 'https' : 'http';

but we might then end up needing an extra scheme for #10268 and the likes of #10253.

#3 @Denis-de-Bernardy
10 years ago

  • Keywords dev-feedback added

#5 @ryan
10 years ago

The current behavior is on purpose and patterned after gmail where you have the option of using SSL or not by using either https or http. If we redirect logins to https the user will end up dumped in an SSL admin session after login even though they requested http. I think if someone is concerned with this they should just force ssl admin.

#6 @ryan
10 years ago

  • Milestone changed from 2.8.1 to 2.9

#7 @Denis-de-Bernardy
10 years ago

well, I figured the same, but ssl is a tad bit slow (a lot slower even)...

#8 @Denis-de-Bernardy
10 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

#9 @ryan
10 years ago

  • Milestone changed from 2.9 to Future Release

#10 @willnorris
6 years ago

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

I agree with ryan's point that if this is the desired behavior, then you should set FORCE_SSL_ADMIN. Google has published research about the actual CPU, memory, and network overhead that SSL introduces, and it's really not an issue with modern servers. I'm sure the numbers are even lower now, since this research is three years old.

closing as wontfix... feel free to reopen if you still believe this is an issue.

#11 @helen
6 years ago

  • Milestone Future Release deleted

#12 follow-up: @willnorris
6 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

after talking with @nacin tonight, he pointed out that this is more than just confusing, it has some actual security implications. It's part of a handful of similar bugs that are slated to get fixed in 3.7, so reopening.

#13 @dd32
6 years ago

  • Milestone set to Future Release

#14 @nacin
6 years ago

  • Milestone changed from Future Release to 3.7

#15 @johnbillion
6 years ago

#22283 was marked as a duplicate.

#16 @johnbillion
6 years ago

  • Milestone changed from 3.7 to 3.8

No patch and needs some more discussion. Also see discussion on #22283. We should try to get this into 3.8.

#17 @ethitter
6 years ago

  • Cc erick@… added

#18 in reply to: ↑ 12 @Denis-de-Bernardy
6 years ago

Replying to willnorris:

after talking with @nacin tonight, he pointed out that this is more than just confusing, it has some actual security implications. It's part of a handful of similar bugs that are slated to get fixed in 3.7, so reopening.

Along similar lines, there's #10268.

#19 @samuelsidler
6 years ago

  • Milestone changed from 3.8 to Future Release

Still no patch, not going to make 3.8.

#20 @nacin
5 years ago

  • Milestone changed from Future Release to 4.0

Time to retire login SSL. Admin or GTFO. Upcoming commit forces SSL in the admin if you had FORCE_SSL_LOGIN set.

The alternative is to let it rot / discourage its use / issue a deprecation notice. But if someone wants FORCE_SSL_LOGIN they probably care about security more than the potential for trouble. The current security is smoke and mirrors, while a lot of that trouble (like mixed content issues) do plan to be fixed in 4.0.

#21 @nacin
5 years ago

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

In 28609:

Forcing SSL logins now forces SSL for the entire admin, with no middle ground.

fixes #10267.

#22 @iandunn
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I ran into a problem after updating an MU installation to 4.0-beta1. I'm not sure how common the site's configuration is, or if it's even considered a supported use case, but I figured I'd mention it just to be safe.

Configuration:

  • Main site does have an SSL certificate
  • Sub-sites do not have SSL certificates
  • wp-config.php enables FORCE_SSL_LOGIN
  • Logging in on the sub-sites is redirected to the main site via an mu-plugin

r28609 causes two problems with this setup:

  1. wp-login.php redirects the user to the sub-site's HTTPS login URL, and their browser shows them a certificate error.
  2. The auth cookie scheme changes from auth to secure_auth, and the user gets stuck in a loop when they login.

The step-by-step process looks like this:

If backwards compatibility isn't going to be maintained in this case, then the problems can be fixed on the site's end by making these adjustments:

  1. Redirecting to https://sub-site.example.org/wp-admin can be avoided by disabling FORCE_SSL_LOGIN on the sub-sites. This is really how it should have been configured all along, but before r28609 there wasn't any obvious consequence to setting it incorrectly.
if ( 'example.org' == $_SERVER['HTTP_HOST'] ) {
        define( 'FORCE_SSL_LOGIN', true );
} else {
        define( 'FORCE_SSL_LOGIN', false );
}
  1. The login redirection loop can be avoided by using the auth scheme when redirecting to a site without a certificate:
if ( ! empty( $_REQUEST['redirect_to'] ) && 'http://' == substr( urldecode( $_REQUEST['redirect_to'] ), 0, 7 ) ) {
        add_filter( 'secure_signon_cookie',    '__return_false' );
        add_filter( 'secure_auth_cookie',      '__return_false' );
        add_filter( 'secure_logged_in_cookie', '__return_false' );
}

#23 @jeremyfelt
5 years ago

It looks like https://wordpress.org/plugins/ssl-subdomain-for-multisite/ does something similar. Not sure if that's likely to be used on many installations. In my multi-network configuration I set FORCE_SSL_LOGIN and FORCE_SSL_ADMIN in sunrise for domains that are deemed SSL ready, similar to iandunn's example (1) above. This works really well, though I don't rely on a global auth point. It also haunts me whenever a domain lives in un-SSL-ready territory for more than a few hours. :)

I think nacin sums it up in comment:20. Providing SSL login without SSL admin areas is a false sense of security.

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


5 years ago

#25 @DrewAPicture
5 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Closing as fixed after [28609].

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


4 years ago

Note: See TracTickets for help on using tickets.