Make WordPress Core

Opened 9 years ago

Last modified 6 years ago

#34924 new defect (bug)

Network upgrade fails on tls 1.2 only servers

Reported by: mensmaximus's profile mensmaximus Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.7
Component: HTTP API Keywords: https
Focuses: multisite Cc:

Description

Setup:
Nginx 1.9.7 on Centos 7.1 with cURL 7.29
SSL configured with 'ssl_protocols TLSv1.2' only because Firefox does only accept TLS 1.2 for http/2

Symptom:
After upgrading from WordPress 4.3 to 4.4 the network upgrade fails with error message:
'Your server may not be able to connect to sites running on it. Error message: TCP connection reset by peer'

Test:
Setting 'curl_setopt ( $handle, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1 );' in WP_Http_Curl::request solves the issue

This is definitely an issue with older cURL versions. cURL 7.45 works well as reported by @dd32.

Suggestion:
Conditionally set CURLOPT_SSLVERSION if the first connect over https fails.

Hint:
Since WP_Http_Curl has been introduced in WordPress 2.7.0 I guess all version since 2.7.0 are affected.

Change History (22)

#1 @jeremyfelt
9 years ago

  • Component changed from General to HTTP API
  • Focuses multisite added

Hi @mensmaximus, thanks for the ticket.

The option for TLSv1.2 was added to cURL in 7.34.0 and to OpenSSL in 1.0.1c.

I think it makes sense there would be trouble connecting to a server that only accepted TLSv1.2 connections unless the libraries are more up to date. Most server configurations should probably support both TLSv1.0 and TLSv1.2 so that the client can choose. In the described scenario, Nginx can offer TLSv1.0 and TLSv1.2 and clients such as Firefox will choose TLSv1.2.

We don't explicitly set any CURLOPT_SSLVERSION in core, so cURL should be able to make the determination itself based on the protocols available to it and those offered by the server. The http_api_curl filter can be used to set more explicit versions.

I'm not sure there's anything we can do in core directly, though I think that filter should help.

#2 follow-up: @mensmaximus
9 years ago

Hi @jeremyfelt, thank you for the follow up.

The main issue is that although cURL 7.29 on Centos and probably RedHat do support TLSv1.2 it does not auto-negotiate. Setting CURLOPT_SSLVERSION to CURL_SSLVERSION_TLSv1 solves the issue. It is great to know there is a filter and I will use it for the future. Thanks for that.

However most Multisite users will struggle in the first place if they come across an error telling "TCP connection reset by peer". I help a lot at wpde.org and I see questions about error messages a lot.

In my tests Firefox did not choose TLSv1.2 if the server is set to "ssl_protocols TLSv1 TLSv1.1 TLSv1.2;" and "ssl http2". Maybe a bug in FF 42.

Yes this is a cURL issue and not something related to WordPress. Anyway helping making the web more secure should be our goal. There are thousands of recommendations to stop using SSLv2 and SSLv3. From my point of view it would not hurt if WordPress HTTP API would operate with TLS by default setting CURLOPT_SSLVERSION to CURL_SSLVERSION_TLSv1 because it will auto-negotiate between all available TLS versions and choose the highest available. In addition from cURL 7.39 on SSLv3 is disabled by default.

Setting TLS in CORE explicitly would help to avoid irritations and to make connections more secure because the HTTP API deals with remote connections as well. Using the filter to "tighten" security seems odd. Having the filter to provide a fallback for SSL makes perfect sense for me.

Maybe a discussion on slack is more appropriate than me spaming trac :-)

#3 in reply to: ↑ 2 ; follow-up: @jeremyfelt
9 years ago

Replying to mensmaximus:

From my point of view it would not hurt if WordPress HTTP API would operate with TLS by default setting CURLOPT_SSLVERSION to CURL_SSLVERSION_TLSv1 because it will auto-negotiate between all available TLS versions and choose the highest available. In addition from cURL 7.39 on SSLv3 is disabled by default.

I like this idea in general. Is there anything that wouldn't be compatible with it? These scan stats show 98.9% of servers supporting TLS1.

#4 @jeremyfelt
9 years ago

  • Version changed from 4.4 to 2.7

#5 in reply to: ↑ 3 @mensmaximus
9 years ago

Replying to jeremyfelt:

I like this idea in general. Is there anything that wouldn't be compatible with it? These scan stats show 98.9% of servers supporting TLS1.

The only issue I can think of is if you explicitly choose a weak cipher while using TLS. That would make the connect fail. But as no cipher is chosen in the WordPress HTTP API I dont see a problem. It might be best to reach out to plugin developers known for using the HTTP API as part of their code to ask for testing. Maybe some of them already use the http_api_curl filter to use TLS.

#6 follow-up: @jeremyfelt
9 years ago

It's possible the underlying cause of this is really #34935.

#7 in reply to: ↑ 6 @mensmaximus
9 years ago

Replying to jeremyfelt:

It's possible the underlying cause of this is really #34935.

I doubt this is the root cause because I discovered the same issue with the plugin mainwp while still on WP 4.3.1. Mainwp is using its own classes to perform the curl stuff rather than using the WordPress HTTP API.

#8 @johnbillion
9 years ago

  • Keywords https added

#9 @dd32
9 years ago

#36320 was marked as a duplicate.

#10 @johnbillion
9 years ago

#36320 was marked as a duplicate.

#11 @johnbillion
9 years ago

  • Keywords dev-feedback added

Thinking out loud: If libcurl < 7.34 cannot correctly negotiate a TLS1.2 connection, how about we conditionally set the default transport order based on that?

In WP_Http::_get_first_available_transport():

$curl = curl_version();
if ( 'https' === parse_url( $url, PHP_URL_SCHEME ) && version_compare( $curl['version'], '7.34', '<' ) ) {
    $transports = array( 'streams', 'curl' );
} else {
    $transports = array( 'curl', 'streams' );
}

What other compatibility issues is this likely to cause?

#12 follow-ups: @dd32
9 years ago

I've been thinking about this a bit, and I think we should probably attempt to:

  • Leave the priority order as [ curl, streams ]
  • in the cURL transport - attempt to use the most-popular SSL standard, in the event a SSL Connect failure is hit, attempt others in a fallback case. cURL not being able to negotiate a secure connection is a big enough bug to warrant a workaround

@rmccue What are your thoughts on this issue?

#13 in reply to: ↑ 12 @mensmaximus
9 years ago

Replying to dd32:

  • in the cURL transport - attempt to use the most-popular SSL standard, in the event a SSL Connect failure is hit, attempt others in a fallback case. cURL not being able to negotiate a secure connection is a big enough bug to warrant a workaround

cURL is able to negotiate. Please reread my postings: "setting CURLOPT_SSLVERSION to CURL_SSLVERSION_TLSv1 because it will auto-negotiate between all available TLS versions and choose the highest available"

To fix this TLS/SSL issue in WordPress simply set CURLOPT_SSLVERSION to CURL_SSLVERSION_TLSv1 instead of omitting this parameter at all as you do right now in WordPress. There is no need to implement a fallback that is already built in to cURL.

#14 in reply to: ↑ 12 @rmccue
9 years ago

Replying to dd32:

@rmccue What are your thoughts on this issue?

Streams can potentially also be broken, as they have their own version of OpenSSL which could be out-of-date (or worse).

I'd be +1 on trying a fallback if we hit a SSL connection issue, whether that fallback is changing the cURL option or falling back to streams.

With regards to the actual issue itself: it seems like cURL is unable to negotiate TLS instead of SSL? I'm not sure why setting the constant makes this work whereas it's unable to without it?

(I'm happy to also drop support for SSL in favour of TLS-only.)

#15 follow-up: @rmccue
9 years ago

Took a look into this with @dd32. For newer versions of cURL, the default setting is equivalent to setting to TLSv1, but older versions have the distinction and default to SSL 2/3 negotiation. This means that the default is broken for (what appears to be) cURL versions <7.33.0.

CURL_SSLVERSION_TLSv1 also isn't available in PHP <5.5, but we can just set the setting to the integer value of the constant instead (1)

For consistency, seems like the best course of action is to set CURLOPT_SSLVERSION to 1. Don't think we need a version switch for this really.

#16 in reply to: ↑ 15 @rmccue
9 years ago

Replying to rmccue:

This means that the default is broken for (what appears to be) cURL versions <7.33.0.

Looks like the default changed in 7.39.

#17 @dd32
9 years ago

  • Keywords dev-feedback removed

After looking at the raw requests made by cURL and the cURL source, I also agree that setting the constant seems like the best way forward.
There's a slightly possibility to break a server somewhere which doesn't handle TLS connections well, but I'm not overly concerned as they're going to be broken already for other clients.

I don't really think that this is something we should change during the RC period (even though I don't think it'll break anything) - I'd suggest that plugins who need to ensure they're making TLSv1.2 connections should probably hook into cURL and make that happen directly

#18 follow-up: @mnelson4
8 years ago

I don't really think that this is something we should change during the RC period

Now that 4.6 is out, is now a good time to have another look at getting this into core?

I think the main motivator to get this done is that in June 2017 PayPal will ONLY be accepting TLS1.2 connections (see https://devblog.paypal.com/upcoming-security-changes-notice/ in section "TLS 1.2 Upgrade"), so it would be nice to have this change in WP core well before then, otherwise every plugin/theme that uses PayPal will need to hook into curl etc themselves (and hope their users upgrade on time).

#19 in reply to: ↑ 18 @mensmaximus
8 years ago

Replying to mnelson4:

Now that 4.6 is out, is now a good time to have another look at getting this into core?

+1

There should be no discussion about TLS 1.2 beeing definitely the protocol to use until TLS 1.3 is launched. WordPress with a 26% market share should do the same as google, firefox, letsencrypt and many others do: start with the highest and strongest encryption available and fallback if needed. However I guess this is not an argument because WordPress development cares more about backward compatibility.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


8 years ago

#21 @mnelson4
8 years ago

FYI this is what woocommerce is doing to resolve the issue until this happens in WP core:

...
                add_action( 'http_api_curl', array( $this, 'force_ssl_version' ) );
        }
        /**
         * Force SSL version to TLS1.2 when using cURL
         *
         * Thanks olivierbellone (Stripe Engineer)
         * @param resource $curl the handle
         * @return null
         */
        public function force_ssl_version( $curl ) {
                if ( ! $curl ) {
                        return;
                }
                if ( OPENSSL_VERSION_NUMBER >= 0x1000100f ) {
                        if ( ! defined( 'CURL_SSLVERSION_TLSv1_2' ) ) {
                                // Note the value 6 comes from its position in the enum that
                                // defines it in cURL's source code.
                                define( 'CURL_SSLVERSION_TLSv1_2', 6 ); // constant not defined in PHP < 5.5
                        }
                
                        curl_setopt( $curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2 );
                } else {
                        if ( ! defined( 'CURL_SSLVERSION_TLSv1' ) ) {
                                define( 'CURL_SSLVERSION_TLSv1', 1 ); // constant not defined in PHP < 5.5
                        }
                        curl_setopt( $curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1 );
                }
        }

see https://github.com/woocommerce/woocommerce-gateway-stripe/pull/67/commits/877dcabf5c60ef6e41f770ecf654274fa1fc0ce5?diff=unified

#22 @mnelson4
6 years ago

an update:

WooCommerce has stopped doing what I pasted previous, as is now always setting CURLOPT_SSLVERSION to 6 (CURL_SSLVERSION_TLSv1_2). See https://github.com/woocommerce/woocommerce/blob/bfebd305654919629d5d8e32782ef0d249416991/includes/class-wc-https.php#L133
Here's some rationale for it: https://github.com/woocommerce/woocommerce-gateway-stripe/issues/45#issuecomment-248846989

We found on https://github.com/eventespresso/event-espresso-core/pull/581 when CURL on 7.29 and openSSL on 1.0.1e, setting CURLOPT_SSLVERSION to 1 actually CAUSED TLS/SSL handshake to fail. (I think that's because that version of CURL was instructing openSSL to use TLS1.0 by providing the argument -tls1, which would fail when communicating with servers requiring TLS1.2 or higher).

On the other hand, leaving CURLOPT_SSLVERSION as the default, or setting it to 6, resolved the issue. (I think ommitting CURLOPT_SSLVERSION worked because it allowed openSSL to negotiate the TLS/SSL version. Using 6 also worked because that version of CURL didn't know how to handle it, and so passed nothing and was equivalent to not specifying CURLOPT_SSLVERSION at all.)

So in summary: setting CURLOPT_SSLVERSION to 1 seems to help for some versions (Eg 7.29) of CURL when communicating with servers only supporting TLS 1.2, but can cause problems for other versions (eg 7.24).

Note: See TracTickets for help on using tickets.