Make WordPress Core

Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#16606 closed defect (bug) (fixed)

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

Reported by: westi's profile westi Owned by: dd32's profile dd32
Milestone: 3.7 Priority: high
Severity: major Version: 3.0.5
Component: HTTP API Keywords:
Focuses: Cc:

Description

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' )

Attachments (9)

16606.patch (676 bytes) - added by hakre 13 years ago.
Test streams transport SSL support
16606.2.patch (721 bytes) - added by hakre 13 years ago.
Test streams transport HTTP and SSL support
16606.3.patch (1.0 KB) - added by hakre 13 years ago.
Test streams transport HTTP? (correct Scheme specified by URL) and SSL support
16606.4.patch (1.6 KB) - added by hakre 13 years ago.
All checks, result truly filterable line in WP_Http_Curl::test() and WP_Http_ExtHttp::test()
16606.5.diff (3.8 KB) - added by mdawaffe 13 years ago.
16606.6.patch (4.8 KB) - added by hakre 13 years ago.
16606.7.patch (4.6 KB) - added by hakre 13 years ago.
16606.8.diff (591 bytes) - added by mdawaffe 13 years ago.
Fix streams context (but don't bother)
test-https-request-methods.php (782 bytes) - added by mdawaffe 13 years ago.

Download all attachments as: .zip

Change History (32)

#1 follow-up: @dd32
13 years ago

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
13 years ago

Test streams transport SSL support

@hakre
13 years ago

Test streams transport HTTP and SSL support

#2 in reply to: ↑ 1 @hakre
13 years ago

  • Keywords has-patch added; needs-patch removed

Replying to dd32:

Should investigate WP_HTTP_Fsockopen() as well here

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

Last edited 13 years ago by hakre (previous) (diff)

@hakre
13 years ago

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

#4 follow-up: @dd32
13 years ago

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);
	}

#5 in reply to: ↑ 4 @hakre
13 years ago

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?

Last edited 13 years ago by hakre (previous) (diff)

@hakre
13 years ago

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

#6 follow-up: @mdawaffe
13 years ago

  • Cc mdawaffe added

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.

@mdawaffe
13 years ago

#7 in reply to: ↑ 6 @hakre
13 years ago

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.).

And this is for streamline reasons: Not all test() functions were filterable in full.

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.

Last edited 13 years ago by hakre (previous) (diff)

@hakre
13 years ago

#8 follow-up: @hakre
13 years ago

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.

@hakre
13 years ago

#9 in reply to: ↑ 8 @mdawaffe
13 years ago

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?

Last edited 13 years ago by mdawaffe (previous) (diff)

#10 @dd32
13 years ago

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
13 years ago

Fix streams context (but don't bother)

#11 @mdawaffe
13 years ago

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.

#12 @sivel
13 years ago

See #9078 and #8702

Last edited 13 years ago by sivel (previous) (diff)

#13 follow-up: @sivel
13 years ago

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.

#14 @sivel
13 years ago

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)

#15 in reply to: ↑ 13 @mdawaffe
13 years ago

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.

#16 @dd32
13 years ago

(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

#17 @dd32
13 years ago

  • Milestone changed from Future Release to 3.2

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.

#18 @ryan
13 years ago

Done for 3.2?

#19 @dd32
13 years ago

  • Keywords 3.3-early added; 3.2-early removed
  • Milestone changed from 3.2 to Future Release

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

#20 @kurtpayne
11 years ago

  • Cc kurtpayne added

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

#21 @dd32
11 years ago

  • Keywords 3.3-early has-patch removed
  • Milestone changed from Future Release to 3.7

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

#22 @dd32
11 years ago

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

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

#23 @dd32
11 years ago

In 25225:

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

Note: See TracTickets for help on using tickets.