WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#25716 closed defect (bug) (fixed)

HTTP requests fails with "SSL read: error:00000000:lib(0):func(0):reason( 0), errno 0"

Reported by: ocean90 Owned by: nacin
Milestone: 3.7.1 Priority: highest omg bbq
Severity: blocker Version: 3.7
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:

Description (last modified by ocean90)

I'm having a user where the HTTP requests fails via SSL.

$request contains a WP_Error object:

object(WP_Error)#6595 (2) {
  ["errors"]=>
  array(1) {
    ["http_request_failed"]=>
    array(1) {
      [0]=>
      string(58) "SSL read: error:00000000:lib(0):func(0):reason(0), errno 0"
    }
  }
  ["error_data"]=>
  array(0) {
  }

Used transport: cURL, curl_errno() is 56.

PHP Version: 5.3.27
SSL Version: OpenSSL/0.9.8q
Curl Version: 7.31.0
Curl SSL: yes
OpenSSL support: enabled
OpenSSL Library Version: OpenSSL 0.9.8q 2 Dec 2010
OpenSSL Header Version: OpenSSL 0.9.8q 2 Dec 2010

Attachments (5)

25716.diff (1.3 KB) - added by nacin 20 months ago.
25716.2.diff (8.1 KB) - added by nacin 20 months ago.
25716.3.diff (8.3 KB) - added by nacin 20 months ago.
Only issue E_WARNING if WP_DEBUG or headers_sent(), otherwise E_NOTICE.
25716.4.diff (2.1 KB) - added by nacin 20 months ago.
Hotfix…
25716.wp_http.diff (770 bytes) - added by dd32 20 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 @nacin20 months ago

  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

This is not good, because then no updates work at all. In fact, they are trapped on 3.7 and won't be able to update. (Something in the Hotfix plugin will be necessary.)

We *need* an escape hatch for this. If necessary, we can simply double-up on the request — if the SSL one fails with http_request_failed, try again with non-SSL.

comment:2 follow-up: @rmccue20 months ago

This makes me sad. :(

I'll try and replicate ASAP, but agreed with nacin, we should try without SSL if SSL fails, but not always. The only way to detect if it's from SSL is to search the error string for "SSL" (booo; I'm working on a way of improving this for both cURL and sockets).

Note that if we silently send a non-SSL request, we lose any benefit that SSL gives us. IMO, the best option is to warn with an AYS.

(This also reaffirms the need for a HTTP Tester plugin; I'll get cracking on that this week.)

Last edited 20 months ago by rmccue (previous) (diff)

comment:3 in reply to: ↑ 2 ; follow-up: @nacin20 months ago

Replying to rmccue:

Note that if we silently send a non-SSL request, we lose any benefit that SSL gives us. IMO, the best option is to warn with an AYS.

Well, not entirely. We still only serve auto-updates from the WP.org API if the request doesn't come through SSL. For 3.7, we need to contain the damage by always trying non-SSL when a request fails.

comment:4 in reply to: ↑ 3 @rmccue20 months ago

Replying to nacin:

Well, not entirely. We still only serve auto-updates from the WP.org API if the request doesn't come through SSL. For 3.7, we need to contain the damage by always trying non-SSL when a request fails.

If you're on a site without SSL support and you attempt an update, without an AYS you'll never know if you got MITM'd. That's a huge security issue, which is why IMO we should at least issue a warning.

@nacin20 months ago

@nacin20 months ago

comment:5 @nacin20 months ago

25716.2.diff introduces a new string (we may just keep it in English for 3.7.1, as core language packs are not up-and-running yet) and reuses an existing one.

The only problem with a warning is that it could cause headers-already-sent issues. Probably a few situations in updates and such where that can be a problem.

Issuing just a notice when headers_sent() || WP_DEBUG ? E_USER_WARNING : E_USER_NOTICE is cheap but it works.

This patch was lightly tested, but I probably screwed something up. Each piece must be tested.

My testing code:

add_filter( 'pre_http_request', function( $result, $args, $url ) {
	if ( 0 === strpos( $url, 'https://' ) )
		return new WP_Error( 'ssl_failed' );
	return $result;
}, 10, 3 );

Also, here's some code someone can use to fix their site:

add_filter( 'use_curl_transport', 'wp_ticket_25716_ssl_not_supported', 10, 2 );
add_filter( 'use_streams_transport', 'wp_ticket_25716_ssl_not_supported', 10, 2 );
function wp_ticket_25716_ssl_not_supported( $test, $args ) {
    if ( ! empty( $args['ssl'] ) )
        return false;
    return $test;
}
Last edited 20 months ago by nacin (previous) (diff)

comment:6 @nacin20 months ago

  • Keywords has-patch added

rmccue approved this approach before he went off to bed.

@nacin20 months ago

Only issue E_WARNING if WP_DEBUG or headers_sent(), otherwise E_NOTICE.

comment:7 @nacin20 months ago

  • Keywords commit added; needs-testing removed

Could use some more eyes and testing, but 25716.3.diff works for me. This patch falls back to E_USER_NOTICE if headers aren't set (and ! WP_DEBUG).

comment:8 @nacin20 months ago

You'll not that 25716.3.diff is not DRY. We can make it DRY in 3.8. In a point release, I want to limit cross-file dependencies. If a critical update error occurs and not all files copy, I don't want it to end up having a fatal error because the new function's file was what wasn't copied over. Ideally, *any* file can not update and everything will still be OK.

@nacin20 months ago

Hotfix...

comment:9 @ocean9020 months ago

  • Description modified (diff)

comment:10 @ocean9020 months ago

Back to the original error code:
It seems to be related to a specific curl version, 7.31.0, see http://sourceforge.net/p/curl/bugs/1249/. The related commit is https://github.com/bagder/curl/commit/8a7a277c086199b3 which should be in cURL 7.32.0.

25716.3.diff looks good so far and works on the broken site.

Hotfix: The broken site drives me crazy.

  • wp_remote_get( 'https://api.wordpress.org/' ) works
  • wp_remote_post('https://api.wordpress.org/core/checksums/1.0/?version=3.7') works
  • wp_remote_post( 'https://api.wordpress.org/', array( 'body' => array( 'foo' => 'bar' ) ) ) doesn't work

Because of this, wp_http_supports( array( 'ssl' ) ) && is_wp_error( wp_remote_get( 'https://api.wordpress.org/' ) ) is false.

comment:11 follow-ups: @dd3220 months ago

As a quick fix, 25716.3.diff looks OK.

There's a thread in the support forums for something similar:
http://wordpress.org/support/topic/37-search-for-plugins-in-cp-gives-unexpected-error?replies=15#post-4807261

I had thrown together a quick plugin to output some info about HTTPS connections too:
http://dd32.id.au/uploads/2013/10/https-debugger.zip

cURL 7.31.0 appears affected from that thread, but potentially also 7.33.0 (I'm about to ask for clarification)

comment:12 in reply to: ↑ 11 @ocean9020 months ago

Replying to dd32:

I had thrown together a quick plugin to output some info about HTTPS connections too:
http://dd32.id.au/uploads/2013/10/https-debugger.zip

cURL 7.31.0 appears affected from that thread, but potentially also 7.33.0 (I'm about to ask for clarification)

That's cool! All tests passed. Maybe we/you should add a test for each method with and without a body.

comment:13 @dd3220 months ago

That's cool! All tests passed. Maybe we/you should add a test for each method with and without a body.

Now that we know that's a issue, I've added a check.
If I can get homebrew to play nicely with OSX 10.9 I'll be able to test those curl versions directly.

comment:14 @dd3220 months ago

If I can get homebrew to play nicely with OSX 10.9 I'll be able to test those curl versions directly.

7.30.0, 7.31.0, 7.32.0, and, 7.33.0 CLI doesn't appear to trigger it for me with a simple payload of foo=bar, it might have something to do with the version of OpenSSL it's compiled against too.

I think the good news here, is that limited environments will be affected, as it's requires a version of cURL released between 3 and 4 months ago, so it'll probably only affect a limited set of PHP 5.5 users (and only those who use a distro that actually keeps up to date with packages, none of my systems that are up to date have curl version more recent than 3 years old..)

@dd3220 months ago

comment:15 @dd3220 months ago

A few alternative approaches:

If we just want to blacklist that version of cURL for HTTPS requests, we can do that easily with 25716.wp_http.diff.

Of course that doesn't protect against future (or still unknown, past) bugs. To cover that we could put a counter in WP_HTTP that increments on SSL failures, 3 or more within a 24hr period (core,plugin,theme update checks twice daily, or a user hitting a failure page) to a known good domain *.wordpress.org, disables that transports SSL abilities for a larger-than-24hr period (1 week?) - If it was caused by cURL, hopefully then the Streams transport would be able to pick up the slack for HTTPS requests. If it can't, then the update checks will revert to HTTP anyway.

Now that we only have 2 HTTP transports, we could also make WP_HTTP::_dispatch_request() failover to the 2nd transport in the case where the first request fails, potentially only for a whitelisted set of domains (ie. *.wordpress.org). That is also not ideal though as it doubles the length of the HTTP timeouts which users might see.

comment:16 in reply to: ↑ 11 @dd3220 months ago

Replying to dd32:

cURL 7.31.0 appears affected from that thread, but potentially also 7.33.0 (I'm about to ask for clarification)

Good news from the thread is that 7.33.0 fixes it, so it's most likely just 7.31.0 that's affected, which confirms ocean90's comments in comment:10

comment:17 @dd3220 months ago

Also appears to affect Ubuntu versions of PHP where cURL is compiled with certain versions of GnuTLS (instead of OpenSSL), one again simple HTTPS requests succeed, post with a body fails:
https://bugs.launchpad.net/ubuntu/+source/gnutls26/+bug/1111882

[PASS]: cURL is installed and supports SSL communication, cURL Details: version_number=464384; age=3; features=50749; ssl_version_number=0; version=7.22.0; host=x86_64-pc-linux-gnu; ssl_version=GnuTLS/2.12.14; libz_version=1.2.3.4; protocols=dict,file,ftp,ftps,gopher,http,https,imap,imaps,ldap,pop3,pop3s,rtmp,rtsp,smtp,smtps,telnet,tftp
[PASS]: [cURL] Communication with WordPress.org suceeded, it took 0.908 seconds
[FAIL]: [cURL with a POST body] Communication with WordPress.org failed with error: [http_request_failed]: GnuTLS recv error (-9): A TLS packet with unexpected length was received.

It appears that GnuTLS will emit that failure when it encounters a server that it doesn't believe adhere to the SSL spec, so the above patch that tests for the known-bad cURL version won't help.

comment:18 @dd3220 months ago

FWIW I'm not against 25716.3.diff either, I think that's a good choice for patching over this for now in a .1 release.

What I don't like is that the cURL handler claims to support SSL when it can't make successful requests, yet, we have a streams handler which most likely could've (in some environments at least).

comment:19 @nacin20 months ago

In 25956:

When an HTTPS request to api.wordpress.org fails, try an insecure HTTP request and issue a warning.

Certain versions of cURL appear to claim OpenSSL support but fail to work. We need to not trap users on older versions while we work this out, and instead fall back to an insecure request.

see #25716 for trunk.

comment:20 @nacin20 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25957:

When an HTTPS request to api.wordpress.org fails, try an insecure HTTP request and issue a warning.

Certain versions of cURL appear to claim OpenSSL support but fail to work. We need to not trap users on older versions while we work this out, and instead fall back to an insecure request.

Merges [25956] to the 3.7 branch.
fixes #25716 for the 3.7 branch.

comment:21 @nacin20 months ago

This is still better than 3.6. And, we still only serve auto-updates when an API request works over SSL.

Let's open a new ticket to track adjustments to this for 3.8.

comment:22 follow-up: @scribu20 months ago

Certain versions of cURL appear to claim OpenSSL support but fail to work.

I don't think that's the whole story. At least for environments with GnuTLS, requests made by WP 3.6 work, while requests made by WP 3.7 fail. More context: https://github.com/wp-cli/wp-cli/issues/846

comment:23 in reply to: ↑ 22 @dd3220 months ago

Replying to scribu:

Certain versions of cURL appear to claim OpenSSL support but fail to work.

I don't think that's the whole story. At least for environments with GnuTLS, requests made by WP 3.6 work, while requests made by WP 3.7 fail. More context: https://github.com/wp-cli/wp-cli/issues/846

GnuTLS refuses to talk to a standard-configured nginx server over SSL, that's the problem with cURL on the travis system. Put basically, when cURL is used with OpenSSL it ignores non-fatal SSL errors that nginx can trigger, when compiled with GnuTLS however, it cannot determine if it's a fatal ssl error or not, and just fails 100% of the time.
(If you have cURL compiled with GnuTLS on the command line, I doubt that can make a POST to https://api.wordpress.org either)

The context that you're missing, is that in 3.6 all API requests went over HTTP, in 3.7 all API requests go over HTTPS if supported else HTTP, and in 3.7.1 all API requests first go over HTTPS if supported, and fall back to HTTP in event of failure (but triggers a PHP Notice/Warning).


The tech details:
The cause problem with cURL 7.31.0 w/ OpenSSL, and with cURL w/ GnuTLS in general is caused by the connection shutting down before the SSL connection has been properly shutdown, this isn't a fatal issue as the data has already been received and sent by both parties - it's simply like your great aunt saying goodbye and hugging you another dozen times, nginx cuts her off and says get in the car.

The root cause is that nginx applies a set of heuristics to open connections to determine when it believes it can close the connection, and closes it before that SSL shutdown handshake is done, so really, it's that nginx takes a shortcut that works with every client except GnuTLS (which sticks to the standard to a dot - which can be the wrong thing to do when dealing with 3rd party software (web servers) ).

nginx can be configured to not close connections early by using it's lingering_close off; directive, the problem is that it can affect performance in a few unknown cases since it's not documented very well.. but we're looking into it.

One can also say it's a bug in nginx's heuristics, and I'd argue that's true, since standard HTTPS GET's work, it's just HTTPS POST's that fail.

comment:24 follow-ups: @barry20 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

lingering_close disabled on WordPress.org. Can you please re-test? Thanks.

comment:25 in reply to: ↑ 24 @dd3220 months ago

Replying to barry:

lingering_close disabled on WordPress.org. Can you please re-test? Thanks.

FWIW, my local PHP+Curl-GnuTLS install now works.

comment:26 in reply to: ↑ 24 @ocean9020 months ago

Replying to barry:

lingering_close disabled on WordPress.org. Can you please re-test? Thanks.

On users host it works now too.

comment:27 @nacin20 months ago

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

Thanks barry! Closing this out for 3.7.1. Related: #25751. New problems can move into new tickets.

comment:28 in reply to: ↑ description @jewelry120 months ago

[spam]

Last edited 20 months ago by dd32 (previous) (diff)
Note: See TracTickets for help on using tickets.