New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

soap #69137 - Fix SSL verify when using a proxy #2152

Merged
merged 2 commits into from Oct 20, 2016

Conversation

3 participants
@ksmiley
Contributor

ksmiley commented Oct 3, 2016

Name verification was failing because the OpenSSL extension was picking the proxy server's address when guessing which name to compare to the SSL certificate. This scenario is already handled for stream wrappers in http_fopen_wrapper.c. This patch applies the same fix to the SOAP extension: when a proxy is used, set peer_name explicitly on the stream context.

https://bugs.php.net/bug.php?id=69137

Thanks to @arrisray for helping debug and fix a segfault in an earlier version of this patch.

soap #69137 - Fix SSL verify when using a proxy
Name verification was failing because the OpenSSL extension was picking
the proxy server's address when guessing which name to compare to the
SSL certificate. This scenario is already handled for stream wrappers
in http_fopen_wrapper.c. This patch applies the same fix to the SOAP
extension: when a proxy is used, set peer_name explicitly on the stream
context.
Show outdated Hide outdated ext/soap/php_http.c
@@ -241,6 +241,13 @@ static php_stream* http_connect(zval* this_ptr, php_url *phpurl, int use_ssl, ph
if (stream && *use_proxy && use_ssl) {
smart_str soap_headers = {0};
/* Set peer_name or name verification will try to use the proxy server name */
if (context && (tmp = php_stream_context_get_option(context, "ssl", "peer_name")) != NULL) {

This comment has been minimized.

@nikic

nikic Oct 12, 2016

Member

This condition seems to be the exact opposite of what the corresponding http_fopen_wrapper code does: https://github.com/php/php-src/blob/master/ext/standard/http_fopen_wrapper.c#L241 The http_fopen_wrapper variant looks right to me in that it still allows a manual peer_name override.

@nikic

nikic Oct 12, 2016

Member

This condition seems to be the exact opposite of what the corresponding http_fopen_wrapper code does: https://github.com/php/php-src/blob/master/ext/standard/http_fopen_wrapper.c#L241 The http_fopen_wrapper variant looks right to me in that it still allows a manual peer_name override.

This comment has been minimized.

@ksmiley

ksmiley Oct 14, 2016

Contributor

I'm glad you caught that. I definitely had it backward. The patch we've been running uses the same logic as http_fopen_wrapper, but in preparing this PR, I pulled the wrong revision from our internal git mirror. I did run the test before submitting, and I've been looking into why it passed when it shouldn't have. The request to download the WSDL file (which uses streams instead of the SOAP HTTP client) was setting peer_name on the context and masking the bug. I've updated the test to avoid this by enabling WSDL caching and warming the cache with a separate SoapClient instance.

@ksmiley

ksmiley Oct 14, 2016

Contributor

I'm glad you caught that. I definitely had it backward. The patch we've been running uses the same logic as http_fopen_wrapper, but in preparing this PR, I pulled the wrong revision from our internal git mirror. I did run the test before submitting, and I've been looking into why it passed when it shouldn't have. The request to download the WSDL file (which uses streams instead of the SOAP HTTP client) was setting peer_name on the context and masking the bug. I've updated the test to avoid this by enabling WSDL caching and warming the cache with a separate SoapClient instance.

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Oct 12, 2016

Member

Test failures are unrelated.

Member

nikic commented Oct 12, 2016

Test failures are unrelated.

@@ -2368,6 +2368,8 @@ PHP_METHOD(SoapClient, SoapClient)
Z_TYPE_P(tmp) == IS_RESOURCE) {
context = php_stream_context_from_zval(tmp, 1);
Z_ADDREF_P(tmp);
} else {
context = php_stream_context_alloc();

This comment has been minimized.

@nikic

nikic Oct 15, 2016

Member

Now that the condition includes !context || instead of context &&, is this part of the change still necessary?

@nikic

nikic Oct 15, 2016

Member

Now that the condition includes !context || instead of context &&, is this part of the change still necessary?

This comment has been minimized.

@ksmiley

ksmiley Oct 18, 2016

Contributor

Without it, the stream in http_connect is created without a context, and the call to php_stream_context_set_option segfaults with a null pointer dereference.

@ksmiley

ksmiley Oct 18, 2016

Contributor

Without it, the stream in http_connect is created without a context, and the call to php_stream_context_set_option segfaults with a null pointer dereference.

@php-pulls php-pulls merged commit 3b9ba61 into php:PHP-7.0 Oct 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

php-pulls pushed a commit that referenced this pull request Oct 20, 2016

dstogov added a commit to zendtech/php-src that referenced this pull request Oct 21, 2016

Merge branch 'master' into jit-dynasm
* master: (24 commits)
  Turn IS_TYPE_COLLECTABLE zval flag into GC_COLLECTABLE zend_refcounted flag. This simplifies checks and allows reset this flag for "acyclic" arrays and objects.
  Fixed bug #73360 Unable to work in root with unicode chars
  Add tests for PDO::getAvailableDrivers
  Remove DBDO-specific field
  Save some more calls to strlen() on the CURLFile helper methods
  Revert "Fix test, this is kinda ugly, but at least for me on Windows there seems to be some messed up line endings"
  Save a call to strlen(), since we can figure out the length of this constant value with sizeof() at compile time
  Fix test, this is kinda ugly, but at least for me on Windows there seems to be some messed up line endings
  Fix bug #71241: array_replace_recursive mutates ref params
  Do not overwrite config.nice.bat if --with-config-profile is used on Windows
  Update UPGRADING wrt. imageresolution()
  Fixed test to accept MYSQLI_OPT_READ_TIMEOUT
  remove redundant includes
  fix Windows compilation
  Add php_random_int internal API
  Fix compiler warnings, always cast to zend_long from sqlite3_int64 when converting to a zval
  Ignore the return value of sqlite3->busyTimeout() if their "API Armor" is not enabled.
  news entry for pr #2135
  news entry for pr #2152
  news entry for #pr 2152
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment