Skip to content
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

Streams: Configure crypto method for ssl:// transports #469

Merged
merged 2 commits into from
Oct 8, 2013
Merged

Streams: Configure crypto method for ssl:// transports #469

merged 2 commits into from
Oct 8, 2013

Conversation

mj
Copy link
Member

@mj mj commented Sep 21, 2013

Streams for ssl:// transports can now be configured to use a specific crypto method (SSLv3, SSLv2 etc.) by calling

stream_context_set_option($ctx, "ssl", "crypto_method", $crypto_method)

where $crypto_method can be one of

  • STREAM_CRYPTO_METHOD_SSLv2_CLIENT
  • STREAM_CRYPTO_METHOD_SSLv3_CLIENT
  • STREAM_CRYPTO_METHOD_SSLv23_CLIENT
  • STREAM_CRYPTO_METHOD_TLS_CLIENT

SSLv23 remains the default crypto method.

Among other situations, this change makes it possible to fopen() URLs on an SSL server that only supports SSL v3: Previously the streams behind this always used STREAM_CRYPTO_METHOD_SSLv23_CLIENT which can now be overridden using the context option.

crypto method (SSLv3, SSLv2 etc.) by calling

stream_context_set_option($ctx, "ssl", "crypto_method", $crypto_method)

where $crypto_method can be one of STREAM_CRYPTO_METHOD_SSLv2_CLIENT,
STREAM_CRYPTO_METHOD_SSLv3_CLIENT, STREAM_CRYPTO_METHOD_SSLv23_CLIENT
or STREAM_CRYPTO_METHOD_TLS_CLIENT. SSLv23 remains the default crypto
method.

This change makes it possible to fopen() SSL URLs that are only
provided using SSL v3.
@rdlowrey
Copy link
Contributor

Actually, after further thought I don't believe this PR is necessary because if you need SSLv3 you can simply use the sslv3:// scheme in your URI, no? Am I missing something here?

@rdlowrey
Copy link
Contributor

Another concern I have with this is ...

SSL v3 is very old and obsolete. Because it lacks some key features and because virtually all clients support TLS 1.0 and better, you should not support SSL v3 unless you have a very good reason.

(source)

I'm not sure making it easier to implement insecure encryption protocols is really a good idea.

@m6w6
Copy link
Contributor

m6w6 commented Oct 3, 2013

@rdlowrey There are plenty of (dumb) SSLv3 services. sslv3:// is for fsockopen() and this PR for fopen() using a stream context.

I'd like to see a test coming with this patch, though.

@m6w6
Copy link
Contributor

m6w6 commented Oct 3, 2013

The test regards @mj though :)

@mj
Copy link
Member Author

mj commented Oct 3, 2013

@m6w6

I'd like to see a test coming with this patch, though.

I'll look into it.

@rdlowrey
Copy link
Contributor

rdlowrey commented Oct 3, 2013

There are plenty of (dumb) SSLv3 services.

@m6w6 I get that. I just wanted to raise the point because SSLv3 is hopelessly insecure and TLSv1.0 has been around since January 1999. If a service requires SSLv3 you should (1) demand the service upgrade its capabilities or (2) not use the service because it might as well not be encrypted at all.

TBH, I'm not sure we should be subsidizing this sort of negligence on the part of service providers.

The bigger issue here is that STREAM_CRYPTO_METHOD_SSLv23_CLIENT (the default) connects just fine to SSLv3 services -- it's like a wildcard telling the underlying OpenSSL libs to essentially negotiate the best protocol possible given the capabilities of the client and the server. There is no hole for SSLv3 in the functionality as it exists currently.

@m6w6
Copy link
Contributor

m6w6 commented Oct 4, 2013

I understand your concerns, but we provide a tool for software development for people to get their job done. We're not a religion or a law making entitity :)

@rdlowrey
Copy link
Contributor

rdlowrey commented Oct 4, 2013

Regardless of how you feel about subsidizing insecure behavior SSLv3 is already supported. Without a substantive use-case I don't see how this addition is justifiable. The only thing this patch does is add overhead without adding additional support. Once you understand how OpenSSL interprets the STREAM_CRYPTO_METHOD_SSLv23_CLIENT constant the patch makes no sense.

@m6w6
Copy link
Contributor

m6w6 commented Oct 4, 2013

Ok well, then show us how you fetch from e.g. https://tst.eboekhuis.nl with fopen or file_get_contents.
Following your argument we may remove support for non-ecrypted transfers, too.

@rdlowrey
Copy link
Contributor

rdlowrey commented Oct 4, 2013

I stand corrected. You can count me in for support on this feature :)

I was working on assumptions from the OpenSSL docs here:

A TLS/SSL connection established with these methods will understand the SSLv2, SSLv3, and TLSv1 protocol. A client will send out SSLv2 client hello messages and will indicate that it also understands SSLv3 and TLSv1. A server will understand SSLv2, SSLv3, and TLSv1 client hello messages. This is the best choice when compatibility is a concern.

With that in mind the following connects successfully (but fails with the SSLv23 method):

<?php
$client = stream_socket_client('tcp://194.229.60.172:443');
if (!stream_socket_enable_crypto($client, TRUE, STREAM_CRYPTO_METHOD_SSLv3_CLIENT)) {
    die("Failed enabling crypto!\n");
}
printf("wrote %d bytes to socket\n", fwrite($client, "GET / HTTP/1.0\r\n\r\n"));
while (!feof($client)) {
    echo fread($client, 8192);
}

Interestingly the server immediately terminates the connection after you write data and the same behavior is seen when connecting via the openssl binary:

openssl s_client -connect 194.229.60.172:443 -ssl3

Recently distros have started disabling SSLv2 (at least in their packaged openssl libs). I suspect that the gradual phasing out of SSLv2 will cause more breakage going forward as the SSLv2 hello messages sent with the SSLv23 client method are no longer understood.

FWIW the different browsers I've tried also fail to successfully resolve the encrypted version of the link you provided. I'm also curious as to why the remote server immediately severs the connection as soon as data is written by the client.

@m6w6
Copy link
Contributor

m6w6 commented Oct 4, 2013

You see, the real world is broken and we have to support those poor souls working with it :)

Anyway for this particular broken service, you've to know that it additionally wants RC4 cipher suite.

openssl  s_client -ssl3 -cipher RC4 -connect tst.eboekhuis.nl:443

@rdlowrey
Copy link
Contributor

rdlowrey commented Oct 4, 2013

Yes :)

Another thing we should look at in relation to this issue is changing the default encryption wrapper from ssl:// to tls:// for use with https:// streams.

The problem of missing SSLv2 support is likely to grow more prevalent as the SSLv2 protocol dies off. Allowing context-level specification for the specific protocol is fine, but using tls:// under the hood would solve this specific case and make things "just work" for most people without having to pass a context.

In any case I'll soon be posting an RFC with lots of ext/openssl improvements. Among other things it adds support for both TLSv1.1 and TLSv1.2 when built against OpenSSL >= 1.0.1. With this change the STREAM_CRYPTO_METHOD_TLS_CLIENT (and its server counterpart) work for all three (1.0/1.1/1.2).

@m6w6
Copy link
Contributor

m6w6 commented Oct 4, 2013

Alright then, I want to merge it into PHP-5.5 when there's a test from @mj

@mj
Copy link
Member Author

mj commented Oct 4, 2013

@m6w6, there's a test case available now.

@php-pulls php-pulls merged commit 047877e into php:master Oct 8, 2013
@mj mj deleted the ssl-streams-crypto-method branch October 12, 2013 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants