WP_Http_Streams::test doesn't check enough to confirm if it can do HTTPS

[westi]

The WP_Http_Streams doesn't check for all it's prerequisites before saying it can process HTTPS request.

It seems to be common for the openssl extension to no be loaded.

Example Warnings:

[21-Feb-2011 08:06:48] PHP Warning:  fopen() [<a href='function.fopen'>function.fopen</a>]: Unable to find the wrapper &quot;https&quot; - did you forget to enable it when you configured PHP? in .../wp-includes/class-http.php on line 1063
[21-Feb-2011 08:06:48] PHP Warning:  fopen(https://example.com/feed/) [<a href='function.fopen'>function.fopen</a>]: failed to open stream: No such file or directory in .../wp-includes/class-http.php on line 1063

We need to at least check: extension_loaded( 'openssl' )

[dd32]

Should investigate ​WP_HTTP_Fsockopen() as well here, this is in the request method:

if ( ( $arrURL['scheme'] == 'ssl' !|| $arrURL['scheme'] == 'https' ) && extension_loaded('openssl') )

[hakre]

Test streams transport SSL support

[hakre]

Test streams transport HTTP and SSL support

[hakre]

Replying to dd32:

Should investigate ​WP_HTTP_Fsockopen() as well here

WP_Http_Fsockopen::test() tests for openssl library; Related: [10864]

[hakre]

Test streams transport HTTP? (correct Scheme specified by URL) and SSL support

[hakre]

Related: 3:ticket:16959

[dd32]

The Streams class only accepts ​http:// and ​https:// scheme's:

		if ( 'http' != $arrURL['scheme'] && 'https' != $arrURL['scheme'] )
			$url = preg_replace('|^' . preg_quote($arrURL['scheme'], '|') . '|', 'http', $url);

That renders most of the content of these patches moot.

Suggested:

	function test($args = array()) {
		$use = function_exists('fopen');
		if ( function_exists('ini_get') && true != ini_get('allow_url_fopen') )
			$use = false;
		
		if ( !empty($args['ssl']) && !extension_loaded('openssl') )
			$use = false;

		return apply_filters('use_streams_transport', $use, $args);
	}

[hakre]

Replying to dd32:

The Streams class only accepts ​http:// and ​https:// scheme's:

		if ( 'http' != $arrURL['scheme'] && 'https' != $arrURL['scheme'] )
			$url = preg_replace('|^' . preg_quote($arrURL['scheme'], '|') . '|', 'http', $url);

That renders most of the content of these patches moot.

Hm?

As this is the public test() function, it should return false if you test it with a URL with a scheme that is not supported, than that function should first of all return false for that tested URL.

If the class can technically not support the scheme of the URL because there is no stream wrapper for that scheme registered, it should return false. Existing wrappers are available via stream_get_wrappers().

If you want to check for SSL transport, my stance is to make use of stream_get_transports() to check for SSL. That check is more generic than checking for the openssl extension in specific.

I'll attach an updated patch that takes care of all of those.

For your suggested fopen exists + ini_get exists + allow_url_fopen check I suggest we create a function in either a base class for all transports or into WP_HTTP as this is used more and more often.

We could add some for SSL as well because until now, it's checked against the open_ssl library only. A ->has($what) like wpdb::has_cap() probably. Or on both classes, so that WP_Http can check on all supported transports. Could be added to blocking as well, just an idea.

For what reason does the http stream transport class not support other wrappers btw?

[hakre]

All checks, result truly filterable line in WP_Http_Curl::test() and WP_Http_ExtHttp::test()

[mdawaffe]

What's the use case for the filter when function_exists( 'fopen' ) returns false?

Can we turn this into a generic ticket that adds an SSL check to the ::test() methods for all the WP_HTTP transports?

Attached does so, but does not have some of the additional checks of hakre's patch (I wrote it before finding this ticket).

1. Return false if the required function doesn't exist or could never work.
2. Check for the openssl extension or CURL_VERSION_SSL as needed.
3. Return filtered boolean.

[hakre]

Replying to mdawaffe:

What's the use case for the filter when function_exists( 'fopen' ) returns false?

Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).

Can we turn this into a generic ticket that adds an SSL check to the ::test() methods for all the WP_HTTP transports?

Attached does so, but does not have some of the additional checks of hakre's patch (I wrote it before finding this ticket).

1. Return false if the required function doesn't exist or could never work.
2. Check for the openssl extension or CURL_VERSION_SSL as needed.
3. Return filtered boolean.

The additional checks in my last patch are mostly as a "complete" check, so to pick from those as needed.

I think it's good to streamline all tests. I think it's worth to let them all run after a common principle (do one check after the other unless false) then then the filter at the end of the function, one return statement at the end only. So all test() functions have the same level of quality.

[hakre]

16606.6.patch​ containted the $scheme check in each concrete ::test() function.

In 16606.7.patch​ I reduced the code now by only checking the $scheme where applicable (only Streams so far). Next to that there were some minor formatting / comment / coding issues in 16606.6.patch​ I fixed in this new patch.

The WP_Http_ExtHttp::test() function was lacking check of request and SSL support. The WP_Http_ExtHttp::request() made use of http_inflate which is only available if the http extension supports encodings.

Looks like that now each of the four WP_Http_Concrete::test() is checking for SSL support for the first time.

[mdawaffe]

Replying to hakre:

Replying to mdawaffe:

What's the use case for the filter when function_exists( 'fopen' ) returns false?

Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).

OK.

The curl_version() (with falllback to extension_loaded( 'openssl' )) check in 16606.5.diff​ is a better check for the ability to make an SSL request in WP_Http_Curl.

PS: for the inflate code, rather than changing WP_Http_ExtHttp::request(), should WP_Http_Encoding::decompress() just use http_inflate() if it's available and supported?

[dd32]

What's the use case for the filter when function_exists( 'fopen' ) returns false?

Hooks can still filter it. E.g. reporting, enable / creating functions etc. (dl etc.).

For what it's worth here, I personally believe only a true response should be filterable. If our test rules say the transport can't be used, it can't be used, it's that simple. The test rules are nothing extravagant, basic cap checks..

[mdawaffe]

Fix streams context (but don't bother)

[mdawaffe]

fsockopen() can't verify SSL certificates so its ::test() should reflect that.

Streams can verify SSL certificates, though the code is currently broken (see 16606.8.diff​). Unfortunately, setting verify_peer is useless unless cafile or capath is also set, and we can't know what to set those to (also verify_host doesn't exist). So the streams ::test() should also bail if sslverify is true.

Only cURL and the HTTP extension can verify SSL certs.

[sivel]

See #9078 and #8702

[sivel]

For streams, perhaps allow_self_signed can be of use. As far as requiring cafile or capath, I don't remember that being the case from my testing.

[sivel]

So it basically looks like the only transport we have that really supports SSL verification is cURL. Since the http extension is really just a wrapper for cURL and is being considered for removal from the HTTP API (See #16978)

[mdawaffe]

Replying to sivel:

For streams, perhaps allow_self_signed can be of use.

We can't enable allow_self_signed by default. That would allow MITM attacks. There's currently no filter on the options array, but there should be.

Replying to sivel:

As far as requiring cafile or capath, I don't remember that being the case from my testing.

Were you testing with the faulty code addressed in 16606.8.diff​? Try this test case​.

[dd32]

(In [17598]) Streamline WP_Http_*::test() methods: Check basic SSL requirements, only allow filters to disable transports, not enable them after ::test() has failed. Props mdawaffe for the initial patch. See #16606

[dd32]

before anyone says something, That commit was designed to be a basic check for SSL capability and streamlining the current test methods, not to cover all the SSL issues brought up here.

[ryan]

Done for 3.2?

[dd32]

Some patching was done for 3.2, but there's a bigger question of SSL handling in requests, specifically SSL signing left, Punting the signing to 3.3

[kurtpayne]

Is this still needed? Is it still considered "severity major" ?

[dd32]

Most of this has been rendered fixed, or invalid by #25007

[dd32]

In 25224:

WP_HTTP: Replacing the Fsockopen & Streams Transports with a new Streams transport which fully supports HTTPS communication.
This changeset also bundles ca-bundle.crt from the Mozilla project to allow for us to verify SSL certificates on hosts which have an incomplete, outdated, or invalid local SSL configuration.
Props rmccue for major assistance getting this this far. See #25007 for discussion, also Fixes #16606

[dd32]

In 25225:

WP_HTTP: Re-enable curl, it was accidentally left out of [25224]. See #25007 See #16606