WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 11 months ago

#12609 closed defect (bug) (fixed)

Enabling FORCE_SSL_ADMIN breaks wp-cron.php

Reported by: dphiffer Owned by: johnbillion
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch
Focuses: Cc:

Description

I just completed a server migration and discovered that scheduled posting broke during the process. I tried a few things, including the suggestions from ticket #8923 with no luck. I realized that one change I made, on the new server, was a requirement that the admin uses SSL. After disabling FORCE_SSL_ADMIN I discovered that scheduled posts work again. It seems that I can leave FORCE_SSL_LOGIN turned on and it doesn't cause any problems. This is good enough for our security needs, but I'm sure others might want to have both enabled.

Attachments (3)

12609.diff (2.2 KB) - added by sivel 5 years ago.
Use wp_remote_head instead of wp_remote_post with empty body and switch default for sslverify to false
12609.2.diff (611 bytes) - added by sivel 5 years ago.
Per dd32 there are problems with some hosts where head requests are not processed properly. Just relax the ssl verification for cron requests.
12609.3.diff (477 bytes) - added by rhurling 11 months ago.
default sslverify arg of cron to false

Download all attachments as: .zip

Change History (18)

comment:1 @johnbillion5 years ago

  • Cc johnbillion@… added

comment:2 @nacin5 years ago

Relevant code:

wp_remote_post( $cron_url, array('timeout' => 0.01, 'blocking' => false, 'sslverify' => apply_filters('https_local_ssl_verify', true)) );

If you're using a self-signed or invalid cert, then sslverify=true will prevent the request. You can try this:

add_filter( 'https_local_ssl_verify', '__return_false' ); // __return_false() is 3.0

comment:3 @nacin5 years ago

  • Component changed from General to HTTP
  • Owner set to dd32

comment:4 @dphiffer5 years ago

  • Cc dphiffer added

comment:5 @dd325 years ago

  • Component changed from HTTP to Cron
  • Owner changed from dd32 to westi

IMO, the security check should be relaxed there for cron, It should allow a loopback connection without having to verify the certificate is correct, its not like a man in the middle attack could occur here?

comment:6 @dd325 years ago

  • Milestone changed from Unassigned to 3.0

@sivel5 years ago

Use wp_remote_head instead of wp_remote_post with empty body and switch default for sslverify to false

comment:7 @sivel5 years ago

  • Cc matt@… added
  • Keywords has-patch dev-feedback added

So nacin, Viper007Bond and myself discussed that we should switch sslverify to false as a default and keep the filter so if it is needed we can force verification.

Also we were using wp_remote_post with no body, which gets converted to a GET request and we make sure in wp-cron.php that $_POST is empty before continuing.

We may still want to look at the following logic in wp-cron.php as it seems a little strange that we are making sure $_POST is empty:

if ( !empty($_POST) || defined('DOING_AJAX') || defined('DOING_CRON') )
        die();

@sivel5 years ago

Per dd32 there are problems with some hosts where head requests are not processed properly. Just relax the ssl verification for cron requests.

comment:8 @nacin5 years ago

  • Milestone changed from 3.0 to 3.1

There is currently a filter in place that can fix this, this isn't a regression. Let's get this right in 3.1.

comment:9 @nacin5 years ago

  • Milestone changed from Awaiting Triage to Future Release

Still needs feedback on proper course here.

comment:10 @johnbillion12 months ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 4.0
  • Owner changed from westi to johnbillion
  • Status changed from new to accepted

After some thought I've decided we should change the default value of sslverify to false for local requests, in order to play nicely with hosts that can't verify their own certificates for whatever reason. The https_local_ssl_verify filter will still be applied.

Local requests will loopback, meaning the request isn't exposed to the network and there's no chance of a MITM. If the server doesn't support loopbacks then the connection will fail regardless.

Any objections?

comment:11 @johnbillion12 months ago

  • Component changed from Cron API to HTTP API

@rhurling11 months ago

default sslverify arg of cron to false

comment:12 @rhurling11 months ago

  • Keywords has-patch added; needs-patch removed

just ran into this myself and since @johnbillion said it should default to false i created a new patch

edit:
just saw that johnbillion was asking for objections so maybe i was a bit too fast?
(and i'm not sure if it's okay to create a patch if an owner is set, since i'm not sure what exactly that means)

Last edited 11 months ago by rhurling (previous) (diff)

comment:13 @nacin11 months ago

I'm not actually sure why we care if cron is SSL verified. It's a public URL, anyone can hit it, and if the MITM directs this request elsewhere, then so be it. I suspect ryan or sivel may have some prior knowledge here.

comment:14 @ircbot11 months ago

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

comment:15 @johnbillion11 months ago

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

Fixed in [28781].

Note: See TracTickets for help on using tickets.