WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 11 months ago

#34924 new defect (bug)

Network upgrade fails on tls 1.2 only servers

Reported by: 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 (21)

#1 @jeremyfelt
2 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
2 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
2 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
2 years ago

  • Version changed from 4.4 to 2.7

#5 in reply to: ↑ 3 @mensmaximus
2 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
2 years ago

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

#7 in reply to: ↑ 6 @mensmaximus
2 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
2 years ago

  • Keywords https added

#9 @dd32
20 months ago

#36320 was marked as a duplicate.

#10 @johnbillion
20 months ago

#36320 was marked as a duplicate.

#11 @johnbillion
20 months 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
20 months 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
20 months 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
20 months 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
20 months 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
20 months 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
20 months 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
15 months 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
15 months 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.


14 months ago

#21 @mnelson4
11 months 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

Note: See TracTickets for help on using tickets.