Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#36320 closed defect (bug) (duplicate)

PayPal 2016 merchant security upgrades - Core defaults need to be changed

Reported by: reidbusi's profile reidbusi Owned by:
Milestone: Priority: normal
Severity: major Version: 4.4.2
Component: HTTP API Keywords:
Focuses: Cc:

Description

WordPress' functions as called by WooCoomerce can no longer use the paypal sandbox and as of 2016-06-17 it will no longer be able to talk to the production paypal systems.

See comments here:
https://gist.github.com/mikejolley/0941e0882efcad64ea40

My host's comment:

In order to utilize the TLS 1.2 cipher in accordance with Paypals new policies. You would need to contact the developer of the plugin or your site to have them update the cURLOPT to use TLS 1.2 specifically. It is defaulting to TLS 1.0, but you can use up to TLS 1.2 if you specify it in your code.

My reply to my host:

WooCoommerce uses core WordPress functions to post to the PayPal IPN url. So, then we need to get the WordPress core code modified to use the proper curl settings. I have not had the best of luck getting modifications accepted to the wordpress core in the past. Perhaps if the request comes from someone like Hostgator they will listen?

This will affect every single WooCommerce/WordPress site that is hosted on your systems. On June 17th 2016 they will all stop working. I suggest we head this off at the pass while we have the opportunity.

Thanks for any additional help and weight you can add to the case to be made to Auttomatic.

WordPress functions, as called by WooCommerce default to using curl with HTTP/1.0 and TLS 1.0. These defaults need to be changed.

I will start work on a plugin to change this behaviour if possible, though I would prefer to see this issue addressed in the core.

Please make this happen,
thanks

Change History (29)

#1 @reidbusi
8 years ago

  • Severity changed from normal to major

#2 @dd32
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Severity changed from major to normal
  • Status changed from new to closed

Unfortunately this is a case where the PHP inbuilt cURL cannot auto-negotiate the correct TLS version (although it supports 1.2, it doesn't negotiate it correctly).
Forcing all connections to 1.2 isn't a viable option for WordPress, as not all servers support it.

This is already being discussed in #34924

#3 @reidbusi
8 years ago

So, WooCommerce/WordPress can no longer use PayPal standard? That is the answer? This will not go over well.

I am not suggesting WP force connections to 1.2 but at least try it first.

I guess, like everything else that is broken in the WordPress ecosystem I will have to fix it myself.

Last edited 8 years ago by reidbusi (previous) (diff)

#4 @SergeyBiryukov
8 years ago

WordPress functions, as called by WooCommerce default to using curl with HTTP/1.0 and TLS 1.0. These defaults need to be changed.

As far as I can see, WooCommerce does set httpversion to 1.1 for PayPal requests.

It does not set CURLOPT_SSLVERSION at the moment, but the http_api_curl filter can be used for that.

This appears to be a WooCommerce issue, I don't see why WordPress core defaults need to be changed.

#5 @reidbusi
8 years ago

Yes, I am looking at

<?php
do_action_ref_array( 'http_api_curl', array( &$handle, $r, $url ) );

in class-wp-http-curl.php right now, to create a plugin to fix it.

I guess you are correct, WooCommerce needs to address this, but they hand it off to WP core:
https://docs.woothemes.com/document/paypal-update-for-sha-256/
https://gist.github.com/mikejolley/0941e0882efcad64ea40

While you guys all argue over who has to get off their ass and fix it. I will just do it myself. I might release the plugin, but given the attitude I see on this subject I may not and just let the zillions of WooCommerce sites affected by this fail.

#6 @mikejolley
8 years ago

@reidbusi as I mentioned this is also something controllable at host level. You can disable old protocols. http://disablessl3.com/

More info on insecure protocols https://www.howsmyssl.com/s/about.html

Sure patching the calls in PPS might be a good workaround and viable for WC core, but other requests (not PPS) will still potentially be insecure.

I will just do it myself. I might release the plugin, but given the attitude I see on this subject I may not and just let the zillions of WooCommerce sites affected by this fail.

You are welcome to submit a PR/raise this on WooCommerce Github if you find a workaround. No need to take this kind of attitude...

#7 @reidbusi
8 years ago

Sorry Mike, but you are just wrong in this case.

"CURL_SSLVERSION_TLSv1_0 (4), CURL_SSLVERSION_TLSv1_1 (5) or CURL_SSLVERSION_TLSv1_2 (6) only work for PHP versions using curl 7.34 or newer."
http://php.net/manual/en/function.curl-setopt.php

So my host can disable old protocols all they like, unless they upgrade curl and php to use a later version of curl/libcurl, it is not going to to work (as dd32 implied). My host is currently using curl 7.19.7. So they would need to update curl and php, and once they do they still cannot disable old protocols because that may break other applications on their many thousands of hosting accounts. There are other things running on servers besides WooCommerce/WordPress.

I have found a likely solution however (appears to be working for me), and since I am feeling generous (smug, more accurately), it looks like this:

<?php
function rbst_http_api_transports() {
        return array( 'streams', 'curl' );
}
add_filter( 'http_api_transports', 'rbst_http_api_transports', 9999 );

Which should be the default transport order anyway. So somebody should fix that. I will not bother to release a plugin with only this in it, I will just slap it on our sites and watch for a WP or Woo changelog mention of the issue.

#8 @reidbusi
8 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

#9 @reidbusi
8 years ago

  • Severity changed from normal to major

#10 @SergeyBiryukov
8 years ago

  • Component changed from General to HTTP API
  • Milestone set to Awaiting Review

#11 follow-up: @mozzak
8 years ago

Hi there reidbusi,

I also run into this problem and cannot fix.

your suggestion:

function rbst_http_api_transports() {
        return array( 'streams', 'curl' );
}
add_filter( 'http_api_transports', 'rbst_http_api_transports', 9999 );

where do put this code ? I would like to try this out.

thanks,
Mike

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#12 in reply to: ↑ 11 @reidbusi
8 years ago

Replying to mozzak:

Here is a plugin that contains this "fix":
https://drive.google.com/file/d/0BzyS7-wCVjeOdVBrUHB6ZVZUNzg/view?usp=sharing

DISCLAIMER: I take no responsibility and assume no liability if this causes your server to explode, catch on fire and burn down the data centre, instantiate a singularity and cause the universe to implode (or anything else bad that might happen).

I have only tested this on one of my development sites and it is working for me on my host and does not appear to have broken anything else. There is no guarantee it will work for you (though I expect it will). There is no guarantee it will not break something else on your site (though I expect it will not).

If you do use it, please use it on a development/testing site and backup your database and files before installing it.

I have not submitted it to the WordPress plugin repository and I'm not sure I will bother since it is so small (big effect though). Though if a fix with equivalent results is not included in the next WordPress release I may do so (with an improved version that modifies the transports array instead of replacing it), since many thousands of eCommerce sites will still be broken.

Last edited 8 years ago by reidbusi (previous) (diff)

#13 @johnbillion
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

Thanks for the feedback everyone. This remains a duplicate of #34924 as the core issue is that libcurl older than 7.34 is unable to correctly negotiate a TLS1.2 connection. Let's keep the discussion in one place.

@reidbusi A reminder, we all have the same objective here, to make WordPress better for everyone. Please remember to keep things civil.

#14 follow-up: @reidbusi
8 years ago

@johnbillion I refer you to the issue of scalable vector graphics (the perfect image format for the web) which the platform that drives ~25% of the web refuses to allow (a simple permissions check is all that is required). I have been civil. Sometimes strong words are required to prod people out of complacency.

This is not a duplicate of #34924 - it is a completely separate issue. PHP "streams" should be used before curl. External dependencies are bad, this is an example of why.

You can't make every host out there update curl and rebuild php. It is just not going to happen, so it has to be accounted for, and using php's stream contexts and functions does that. Further, hosts are not going to disable older protocols for curl.

But hey, fine, I give up. Close it, ignore the facts, throw it all out (svg included) with a condescending and superior attitude. It is always "somebody else's problem" here, this crew needs to get over that destructive attitude.

You get back what you put out.

Last edited 8 years ago by reidbusi (previous) (diff)

#15 follow-up: @mozzak
8 years ago

Hi @reidbusi

thanks so much for the efforts.

installed the plugin but now receive the following error:

FAIL - The SSL certificate for the host could not be verified

Does that ring a bell somehow ?

thanks,
Moz

Last edited 8 years ago by mozzak (previous) (diff)

#16 in reply to: ↑ 15 @reidbusi
8 years ago

Replying to mozzak:

Hi @mozzak

I did not see that error in testing on my host.

However, I have produced a new plugin that tells WordPress to use TLS 1.2 for https curl requests to paypal urls (live and sandbox). Instead of the previous plugin which told WordPress to use streams before curl.

Note that this plugin requires that TLS 1.2 and HTTP/1.1 are working in PHP/curl on your server. It does not check for the curl version. The same disclaimers as the previous plugin apply. Uninstall and delete the previous plugin before installing and activating this one.

https://drive.google.com/file/d/0BzyS7-wCVjeOQTNERzFERm9paHc/view?usp=sharing

It is interesting to note that this works fine on my hosts server where curl is version 7.19.7, where previous comments led to believe it would not work unless curl was version 7.34 or later.

I will think about submitting this one to the plugin repository.

Last edited 8 years ago by reidbusi (previous) (diff)

#17 in reply to: ↑ 14 ; follow-up: @rmccue
8 years ago

Replying to reidbusi:

PHP "streams" should be used before curl. External dependencies are bad, this is an example of why.

This is not necessarily a broadly-applicable change, unfortunately. The streams transport uses PHP's built-in version of OpenSSL that it was compiled with, which also has the ability to be broken (in separate and only semi-related ways). cURL in addition tends to have better performance characteristics and more solid handling of edge cases, as it's a very mature project.

I understand your frustration with this issue, but the problem is nuanced and we need to consider the side effects of making a change like this. Changing the default from cURL to streams may break unrelated things, and may not fix the core issue (TLS negotiation) depending on the PHP install configuration.

The core issue here appears to be that cURL's TLS negotiation isn't behaving as expected, which is why #34924 has been suggested as the fix. This is a much smaller change with less potential side-effects than switching the HTTP transport.

I'm investigating exactly why the TLS negotiation is failing, but please bear with us. We don't want WP to be broken any more than you do, but we don't want to break other things in the process of fixing this.

#18 in reply to: ↑ 17 @reidbusi
8 years ago

Replying to rmccue:

I agree @rmccue, after having noted @mozzak's results with the streams transport. Thus my current solution looks like this (only affecting connections to paypal):

<?php
/*
Plugin Name: Merchant Security Upgrades 2016 PayPal Fix for WordPress
Description: Corrects WordPress functions to allow the PayPal Standard gateway for WooCommerce to work with PayPal's new security restrictions. Requires TLS 1.2 and HTTP/1.1 to be working in PHP/curl on your server.
Author: reidbusi
Version: 1.1
*/

if ( ! defined( 'ABSPATH' ) ) {
        exit;
}

function rbst_http_api_curl( &$cr ) {

        $cr_url = curl_getinfo( $cr, CURLINFO_EFFECTIVE_URL );
        $cr_url_parts = parse_url( $cr_url );
        if ( $cr_url_parts['scheme'] == 'https' ) {
                if ( $cr_url_parts['host'] == 'www.paypal.com' || $cr_url_parts['host'] == 'www.sandbox.paypal.com' ) {
                        curl_setopt( $cr, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1 );
                        curl_setopt( $cr, CURLOPT_SSLVERSION, 6);
                }
        }

}
add_action( 'http_api_curl', 'rbst_http_api_curl', 9999, 1 );

I would have passed and used the $r and $url params as provided by:

<?php
do_action_ref_array( 'http_api_curl', array( &$handle, $r, $url ) );

... but they don't really work - it appears wp-cron uses curl to talk to itself and can run at any time on any request and interferes with using the $url parameter to detect the url, where using curl_getinfo() to get the url works consistently.

The most reliable way around this that I can envision is to somehow re-implement TLS 1.2 in php to make WordPress independent of OpenSSL and/or cURL. Since we cannot know what the status of OpenSSL or cURL is on any particular host, the only reliable solution is to DIY. Not sure how to achieve this, or if it would even be possible, but it is the right way as I see it. fsockopen() depends on OpenSSL for tls, so that's out...

PHP sockets might be workable, but again, we cannot know if any particular host as built php with --enable-sockets.

I'll dig around a bit and see if it might be possible to create a host independent TLS 1.2 solution for php.

Version 0, edited 8 years ago by reidbusi (next)

#19 follow-up: @mikejolley
8 years ago

We added this to WooCommerce master a few days ago: https://github.com/woothemes/woocommerce/commit/ae36ff71fae112d08f0caf7f894e221254345328

Setting HTTP to 1.1 is not needed - you can do that when using wp_remote_post

#20 in reply to: ↑ 19 @reidbusi
8 years ago

Replying to mikejolley:

Glad to see that Mike, but in my testing I found that wp-cron will interfere with the $url parameter provided by the http_api_curl action. So one needs to examine the curl handle itself to determine which connection is currently being made by curl.

Also my host's current setup cannot negotiate TLS connections (as well in testing on my host I found the defines for CURLOPT_SSLVERSION were not available and the integer values must be used [i.e. 6 instead of CURL_SSLVERSION_TLSv1_2]), so I'm pretty confident that your commit will not work on my host (Hostgator).

The PHP binaries on our shared hosting servers are linked against the system libraries and utilize an older library that does not auto-negotiate SSL connections with other servers. This is expected to be fixed in CentOS version 6.8, which our servers will automatically update to once the release is pushed. You can see more about the bug report here: https://bugzilla.redhat.com/show_bug.cgi?id=1289205 . We're hopeful that CentOS 6.8 should be released within the next few months.

#21 @mikejolley
8 years ago

@reidbusi #34924 which is related, explains more; defining v1, not v1_2 which is not available on some versions, will negotiate the highest version. Thats why the fix there, and what I did, should work fine. I don't see how wp_cron is relevant here.

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#22 @reidbusi
8 years ago

Replying to mikejolley:

Hi Mike, thanks for the reply. Yes I have read the other ticket and the discussion about negotiation of TLS versions, which is why I quoted a reply from my host (here it is again):

The PHP binaries on our shared hosting servers are linked against the system libraries and utilize an older library that does not auto-negotiate SSL connections with other servers. This is expected to be fixed in CentOS version 6.8, which our servers will automatically update to once the release is pushed. You can see more about the bug report here: https://bugzilla.redhat.com/show_bug.cgi?id=1289205 . We're hopeful that CentOS 6.8 should be released within the next few months.

Also, the defines for the values of CURLOPT_SSLVERSION are missing on my hosts setup. The integer values must be used.

wp_cron is related, because when you test the paypal ipn connection, wordpress may decide to run cron which it appears it does through curl, and when you look at the contents of the $url variable it contains the cron url (scope issue?). It will do this inconsistently (not on every request). Thus the need to use curl_getinfo() to determine the url currently being requested by curl. This was revealed in my testing where I was dumping variables with print_r() to observe what was going on. If I have time later I can dig out the code I was using to test that and demonstrate it for you. Right now I am working on site content for a client and don't have time to repeat this at the moment.

So while we are making progress here, the current commit to the woo master branch is still not quite there in my view.

Last edited 8 years ago by reidbusi (previous) (diff)

#23 follow-up: @mikejolley
8 years ago

@reidbusi not using constants is kinda hacky. Our code does check CURL_SSLVERSION_TLSv1 is defined before using it. You could set this constant to avoid needing to change core.

#24 in reply to: ↑ 23 ; follow-up: @reidbusi
8 years ago

Replying to mikejolley:

@reidbusi not using constants is kinda hacky. Our code does check CURL_SSLVERSION_TLSv1 is defined before using it. You could set this constant to avoid needing to change core.

Hacky or not, it is required on Hostgator shared hosting. I suppose I could define CURL_SSLVERSION_TLSv1_2 (there would be no point in defining CURL_SSLVERSION_TLSv1 for us, since our servers cannot negotiate a TLS connection via cURL). I don't see much point in defining CURL_SSLVERSION_TLSv1_2 in my plugin to use it three lines later either (I'd need to check if the define exists too). Thanks for the suggestion anyway.

Based on this, I guess I will have to publish my plugin for the thousands of other WooCommerce users on Hostgator shared hosting so that they can still use Paypal.

I still think the right answer is to re-implement TLS 1.2 in php somehow to remove all external dependencies that we just cannot assume are present. Nuke it from orbit - it's the only way to be sure.

Or maybe it's time to try to make Paypal do this work, nobody is paying me to do it. If they want it, they can make it happen. They should at least know about this huge problem. They have lots of people on staff who are paid to work on such things. They can contribute to CentOS and get it done. Or re-implement TLS 1.2 in php (or maybe wordpress could include a perl module to do it?).

Me, I'm still working on completely unrelated website content for a paying client...

#25 in reply to: ↑ 24 @rmccue
8 years ago

Replying to mikejolley:

@reidbusi not using constants is kinda hacky. Our code does check CURL_SSLVERSION_TLSv1 is defined before using it. You could set this constant to avoid needing to change core.

In cURL-land, it's not terribly hacky, as the constant values are defined as part of the cURL API. The corresponding integer for TLS negotiation is 1, for reference.

Replying to reidbusi:

Hacky or not, it is required on Hostgator shared hosting. I suppose I could define CURL_SSLVERSION_TLSv1_2 (there would be no point in defining CURL_SSLVERSION_TLSv1 for us, since our servers cannot negotiate a TLS connection via cURL). I don't see much point in defining CURL_SSLVERSION_TLSv1_2 in my plugin to use it three lines later either (I'd need to check if the define exists too). Thanks for the suggestion anyway.

When your host says their SSL client code (presumably OpenSSL) doesn't support SSL negotiation, that sounds to me like it doesn't support v2/v3/TLS negotiation specifically, however I think it should correctly support TLS v1 subversion negotiation, based on my reading of the code, and also as mentioned in the RHEL bug:

With el6 libcurl, CURL_SSLVERSION_TLSv1_2 means "TLS 1.2 only" whereas CURL_SSLVERSION_TLSv1 means "TLS 1.x". Both of them are known to work.

Is it possible to try this set to 1 specifically (the value of CURL_SSLVERSION_TLSv1 if it existed)? If that fixes the issue, we should be fine to ship this in core per #34924.

I still think the right answer is to re-implement TLS 1.2 in php somehow to remove all external dependencies that we just cannot assume are present. Nuke it from orbit - it's the only way to be sure.

TLS is mostly not possible in PHP itself without reimplementing something like OpenSSL in userland, which is just a terrible idea, as it'd mean we need to maintain crypto code ourselves in WordPress. (I've attempted this before just to see if it were possible, and it's also extremely hard to get right.)

At some point, there are times where we need to say that we don't support server configurations. If this is one of them, so be it. Let's make sure we've exhausted all of our options first.

#26 follow-up: @mikejolley
8 years ago

@rmccue Thanks Ryan, found http://stackoverflow.com/questions/31266501/use-of-undefined-constant-curl-sslversion-tlsv1-assumed-curl-sslversion-tlsv1 So you're right, constants don't exist in all environments. + 1 to setting to '1'.

#27 in reply to: ↑ 26 @reidbusi
8 years ago

Replying to mikejolley:

I see the current code in Woo master tree is using the integer value of the constant now, but you might as well just set it to 6, since it is only affecting paypal the way you have it written, and paypal will not allow anything other than TLS 1.2 (currently on sandbox and also on live as of June 17th).

So there is no sense in allowing negotiation, it is detrimental, as some hosts (like mine or any other on CentOS 6.x) cannot do the negotiation anyway.

Basically, paypal is only allowing TLS 1.2 so there is no sense setting it to 1 which is more likely to fail, when you can just set it to six which is required anyway.

Just my thoughts.

Last edited 8 years ago by reidbusi (previous) (diff)

#28 @reidbusi
8 years ago

I went ahead and created a tool that should be able to deal with this in most situations and be helpful for diagnostics. It is not a plugin for non-technical users. As mentioned in the description it is intended to be for "advanced users, site administrators and developers", though a clever user could be instructed how to use it. I may add further instruction to the description intended for average users, such as example options for services like Paypal and Moneris or perhaps pre-made rule-sets to be configured at the press of a button.

https://wordpress.org/plugins/reid-plugins-curl-options/

The most interesting thing that I learned in the production of this plugin is that cURL can be built with NSS instead of OpenSSL (as is the case on the server I am using at my current host): PHP/5.4.45 - cURL/ 7.19.7 - NSS/3.19.1.

I suspect that this fact may explain a lot of the confusion about this issue and the behaviour observed on various servers as discussed above and in the other ticket.

The other interesting thing to note is that some constant defines such as CURL_SSLVERSION_TLSv1_2 has
only been available since PHP 5.5.19 and 5.6.3, though the integer value does work on my servers setup. This is all mostly described in the plugin description.

The other complication is cipher names, cipher suite strings and their formats, it will be difficult to automate the determination of available ciphers on a server, if possible at all.

#29 @reidbusi
8 years ago

It appears PayPal realised they were going to break large numbers of websites and they have backed off on the changes till June 30th 2017 (previously June 17, 2016):

PayPal is upgrading the protocols used to secure all external connections made to our systems. Transport Layer Security version 1.2 (TLS 1.2) and Hypertext Transfer Protocol version 1.1 (HTTP/1.1) will become mandatory for communication with PayPal in 2017. You will need to verify that your environment supports TLS 1.2 and HTTP/1.1, and if necessary make appropriate updates. DATE CHANGE - Act by June 30, 2017

https://www.paypal-knowledge.com/infocenter/index?page=content&id=FAQ1914

Though CentOS 6.8 was released today, so it is ready now:

"PHP cURL module now supports TLS 1.1 and TLS 1.2" and "NSS now enables the TLS version 1.2 protocol by default"

https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/6.8_Release_Notes/new_features_security.html

So hosts now have a whole year to roll it out.

Note: See TracTickets for help on using tickets.