Everything

php

[VOTE] Improved TLS Defaults RFC

php.internals

Daniel Lowrey

12 years ago

Voting is now open for the Improved TLS Defaults RFC and will run through
Wednesday Feb. 19:

https://wiki.php.net/rfc/improved-tls-defaults#vote

Note that while the implementation is vote-ready at this time (and includes
several .phpt files) I'll continue adding more tests in the coming days. If
you have questions that you feel have not been addressed during the past
two weeks please feel free to ask.

Thanks for your time and have a nice day!

Stas Malyshev

12 years ago

Hi!

> Voting is now open for the Improved TLS Defaults RFC and will run through
> Wednesday Feb. 19:
>
> https://wiki.php.net/rfc/improved-tls-defaults#vote

A bit of clarification:
- For stream_socket_enable_crypto, what is the default value of
crypto_type parameter that has to be used if I want the default
behavior? Wouldn't it also be good to have a constant that has the
meaning of "every protocol you can possibly support (including TLS
protocols)"?

- What is the motivation for verify_depth default of 3? RFC does not say
anything on it.

- What is the use case for honor_cipher_order? If the client is "bad",
they won't use honor_cipher_order and thus this option doesn't add to
security. If the client is "well-behaved", they would use the correct
set of cyphers. Is this setting meant for PHP servers? Because the
example clearly uses client side.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Daniel Lowrey

12 years ago

On Tue, Feb 11, 2014 at 4:16 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

>
> Wouldn't it also be good to have a constant that has the
> meaning of "every protocol you can possibly support (including TLS
> protocols)"?
>
>

This is a good point. For this reason it probably makes to either:

1. NOT re-purpose the two STREAM_CRYPTO_METHOD_SSLv23_* constants to mean
SSLv2 or SSLv3 (retain the OpenSSL semantics of SSLv23)
2. Introduce two new constants, STREAM_CRYPTO_CLIENT and
STREAM_CRYPTO_SERVER to mean "anything we can possibly support"

I have a mild preference for #2 as it eliminates confusion that can arise
if you don't have a firm grasp of what OpenSSL means by SSLv23 but am
certainly willing to listen to the preponderance of opinion from others
here.

> - What is the motivation for verify_depth default of 3? RFC does not say
> anything on it.
>
>

I admit this one is a somewhat arbitrary limit (which explains the lack of
explanation in the wiki text). OpenSSL will default to a limit of 9 if we
don't specify one ourselves, so there's not really that much to be gained
by using a default of 3. After considering this a bit more I think it best
to eliminate the addition of a default value in this area altogether. I
will update the RFC and patch accordingly.

> - What is the use case for honor_cipher_order? If the client is "bad",
> they won't use honor_cipher_order and thus this option doesn't add to
> security. If the client is "well-behaved", they would use the correct
> set of cyphers. Is this setting meant for PHP servers? Because the
> example clearly uses client side.
>

Woops! This is a copy/paste error in my example. The honor_cipher_order
only ever has meaning for servers. I will update the example on the wiki to
reflect to correct (server) usage shortly.

Stas Malyshev

12 years ago

Hi!

> 2. Introduce two new constants, STREAM_CRYPTO_CLIENT and
> STREAM_CRYPTO_SERVER to mean "anything we can possibly support"

This I think is a good idea.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Daniel Lowrey

12 years ago

Just a quick note to say that the questions from earlier today have been
addressed in both the RFC text and the proposed patch:

- The arbitrary default "verify_depth" ssl context option is removed

- The RFC was updated to state explicitly which previously merged 5.6
features are proposed for removal in this proposal

- The tls:// wrapper no longer triggers E_WARNING and works the same as
ssl:// with regard to context-specified crypto method flags. The only
difference between tls:// and ssl:// is that the tls wrapper will not
negotiate SSLv2 or SSLv3 unless instructed to do so in the "crypto_method"
context option.

- Added STREAM_CRYPTO_CLIENT and STREAM_CRYPTO_SERVER constants to denote
"any supported protocol."


These changes are largely cosmetic and do not affect the spirit of the RFC.
However, if you feel they may influence any votes previously cast, please
voice your concerns so I can address them :)

Thanks to Stas, Peter and Adam specifically for their questions today.
Also, special thanks to Pádraic for his feedback over the past couple of
weeks.

-  Daniel

Stas Malyshev

12 years ago

Hi!

Thanks for your changes! The RFC looking very good now. One minor note,
tls is still listed as deprecated in Stream Wrapper Creep/Proposal.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Daniel Lowrey

12 years ago

On Wed, Feb 12, 2014 at 1:33 AM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> Thanks for your changes! The RFC looking very good now. One minor note,
> tls is still listed as deprecated in Stream Wrapper Creep/Proposal.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>

Okay, I think I've removed any remaining references to tls:// wrapper
deprecation in the RFC text. More importantly, the code in the patch is
correct :)

Thanks for the heads-up

Pádraic Brady

12 years ago

Hi,

On 11 February 2014 21:53, Daniel Lowrey <rdlowrey@gmail.com> wrote:

> On Tue, Feb 11, 2014 at 4:16 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:
>> - What is the motivation for verify_depth default of 3? RFC does not say
>> anything on it.
>>
>>
> I admit this one is a somewhat arbitrary limit (which explains the lack of
> explanation in the wiki text). OpenSSL will default to a limit of 9 if we
> don't specify one ourselves, so there's not really that much to be gained
> by using a default of 3. After considering this a bit more I think it best
> to eliminate the addition of a default value in this area altogether. I
> will update the RFC and patch accordingly.

It’s partly arbitrary. The main reason for minimising depth, as far as
I’m aware, is to minimise the cost of TLS. It’s an expensive operation
(even for the client) and having an infinite depth may, as far as I
know, be a potential DOS risk. The general values used are 3-6
reflecting real world use. A requirement for a value above this would
indicate potential issues with the server. This may be something of a
historical artifact since I cannot remember where I heard it. I do
know that DOS against SSL servers used to justify limiting this value
for certain.

Paddy may need a memory upgrade…

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Zend Framework PHP-FIG Representative

Daniel Lowrey

12 years ago

On Wed, Feb 12, 2014 at 7:17 AM, Pádraic Brady <padraic.brady@gmail.com>wrote:

> Hi,
>
> On 11 February 2014 21:53, Daniel Lowrey <rdlowrey@gmail.com> wrote:
> > On Tue, Feb 11, 2014 at 4:16 PM, Stas Malyshev <smalyshev@sugarcrm.com
> >wrote:
> >> - What is the motivation for verify_depth default of 3? RFC does not say
> >> anything on it.
> >>
> >>
> > I admit this one is a somewhat arbitrary limit (which explains the lack
> of
> > explanation in the wiki text). OpenSSL will default to a limit of 9 if we
> > don't specify one ourselves, so there's not really that much to be gained
> > by using a default of 3. After considering this a bit more I think it
> best
> > to eliminate the addition of a default value in this area altogether. I
> > will update the RFC and patch accordingly.
>
> It's partly arbitrary. The main reason for minimising depth, as far as
> I'm aware, is to minimise the cost of TLS. It's an expensive operation
> (even for the client) and having an infinite depth may, as far as I
> know, be a potential DOS risk. The general values used are 3-6
> reflecting real world use. A requirement for a value above this would
> indicate potential issues with the server. This may be something of a
> historical artifact since I cannot remember where I heard it. I do
> know that DOS against SSL servers used to justify limiting this value
> for certain.
>
> Paddy may need a memory upgrade...
>
> --
> Pádraic Brady
>
> http://blog.astrumfutura.com
> http://www.survivethedeepend.com
> Zend Framework Community Review Team
> Zend Framework PHP-FIG Representative
>

This was my original line of thinking. However in my research I was unable
to find any real discussion centering around this issue as a significant
DoS vector. A couple of things to consider ...

1. Infinite descent is not an issue because, if unspecified, OpenSSL will
default to a verify depth of 9 as documented here:

https://www.openssl.org/docs/ssl/SSL_CTX_set_verify.html

2. If you're a client (far and away the more frequent use case in PHP) this
is really a non-issue. Servers aren't in the business of DoS'ing clients
and if they are doing something nefarious the solution is to simply not
connect to those servers.

3. I think it's generally safe to expect a bit more knowledge from
developers writing servers than those writing clients.

4. Servers verifying client certificates is only common in very specific
environments (e.g. corporate intranets). In these sorts of implementations
the verify depth will almost always be 0 for self-signed or 1 for an
internal/organizational CA. For these cases a feasible low default verify
depth setting is still not useful.

For these reasons I think it's best to leave out the additional default
setting for now. It would likely only result in failed transfers that would
otherwise have succeeded without adding very much real tangible security
benefit.

Daniel

Chris Wright

12 years ago

On 12 February 2014 12:50, Daniel Lowrey <rdlowrey@gmail.com> wrote:

> 1. Infinite descent is not an issue because, if unspecified, OpenSSL will
> default to a verify depth of 9 as documented here:
>
> https://www.openssl.org/docs/ssl/SSL_CTX_set_verify.html

I would suggest that we set a default of 9 at the PHP level. I would
prefer not to rely on OpenSSL always having a sane default. What with
the docs (for OpenSSL) being updated so infrequently and people just
generally configuring systems in idiotic ways it makes sense to me to
accept OpenSSL's stated default value, but to impose it manually
ourselves.

I personally feel that more control we have over these settings the
better, I'd rather not rely on any 3rd party doing anything sensibly.

Thanks, Chris

Daniel Lowrey

12 years ago

On Wed, Feb 12, 2014 at 8:08 AM, Chris Wright <daverandom@php.net> wrote:

> On 12 February 2014 12:50, Daniel Lowrey <rdlowrey@gmail.com> wrote:
> > 1. Infinite descent is not an issue because, if unspecified, OpenSSL will
> > default to a verify depth of 9 as documented here:
> >
> > https://www.openssl.org/docs/ssl/SSL_CTX_set_verify.html
>
> I would suggest that we set a default of 9 at the PHP level. I would
> prefer not to rely on OpenSSL always having a sane default. What with
> the docs (for OpenSSL) being updated so infrequently and people just
> generally configuring systems in idiotic ways it makes sense to me to
> accept OpenSSL's stated default value, but to impose it manually
> ourselves.
>
> I personally feel that more control we have over these settings the
> better, I'd rather not rely on any 3rd party doing anything sensibly.
>
> Thanks, Chris
>

Fair enough. Do we see value in exposing an
OPENSSL_DEFAULT_STREAM_VERIFY_DEPTH constant to userland?

Chris Wright

12 years ago

On 12 February 2014 13:19, Daniel Lowrey <rdlowrey@gmail.com> wrote:

> Fair enough. Do we see value in exposing an
> OPENSSL_DEFAULT_STREAM_VERIFY_DEPTH constant to userland?

Not really. Applications needing to adjust this are likely to know
specifically what they need for their use case, they won't care what
the default is because they will be overriding it anyway i.e.

// You wouldn't bother with the check, you'd just set it
// to the value you require
if (OPENSSL_DEFAULT_STREAM_VERIFY_DEPTH < 12) {
stream_context_set_option($ctx, 'verify_depth', 12);
}

Peter Cowburn

12 years ago

On 11 February 2014 20:08, Daniel Lowrey <rdlowrey@gmail.com> wrote:

> Voting is now open for the Improved TLS Defaults RFC and will run through
> Wednesday Feb. 19:
>
> https://wiki.php.net/rfc/improved-tls-defaults#vote
>
> Note that while the implementation is vote-ready at this time (and includes
> several .phpt files) I'll continue adding more tests in the coming days. If
> you have questions that you feel have not been addressed during the past
> two weeks please feel free to ask.
>

Overall, I would be very happy to see these changes sooner rather than
later. That said, there is a *super* minor thing that could do with being
sorted one way or another.

Deprecation of "tls://". Please can I raise a dissenting voice just for
that particular transport; it currently serves its purpose very well (I can
has TLS, kthxbye) and asking us to change code to use "ssl://" and add some
extra context configuration to get the same result seems a little out
there.  This particular change was not discussed in the other thread, in
point of fact it read to me like "tls://" would not be deprecated [1]. What
changed? In summary: I'd really rather "tls://" not issue an E_DEPRECATED.

Oh, another super minor thing; could you make it 100% clear (I *may* be the
only one not crystal clear!) about how this RFC affects changes that have
previously been pushed to 5.6 that you wish to undo, and not undo.

>
> Thanks for your time and have a nice day!
>

Thanks so much for your time in getting this together. As I started off by
saying, I'm looking forward to these changing being introduced.

[1] http://markmail.org/message/x46g74zaphdsf5jg

Adam Harvey

12 years ago

On 11 February 2014 13:27, Peter Cowburn <petercowburn@gmail.com> wrote:

> Deprecation of "tls://". Please can I raise a dissenting voice just for
> that particular transport; it currently serves its purpose very well (I can
> has TLS, kthxbye) and asking us to change code to use "ssl://" and add some
> extra context configuration to get the same result seems a little out
> there.  This particular change was not discussed in the other thread, in
> point of fact it read to me like "tls://" would not be deprecated [1]. What
> changed? In summary: I'd really rather "tls://" not issue an E_DEPRECATED.

I missed that the first time reading it myself — I'd definitely prefer
tls:// to still exist too, presumably as a direct alias of ssl://. We
might as well let pedantic people like myself who'd rather use the
proper name be happy!

As for the versioned protocols, though: kill them with fire.

> Thanks so much for your time in getting this together. As I started off by
> saying, I'm looking forward to these changing being introduced.

Seconded. I'm really excited about how much better our SSL/TLS
functionality is going to be in PHP 5.6.

Adam

Daniel Lowrey

12 years ago

On Tue, Feb 11, 2014 at 4:27 PM, Peter Cowburn <petercowburn@gmail.com>wrote:

>
>
>
> On 11 February 2014 20:08, Daniel Lowrey <rdlowrey@gmail.com> wrote:
>
>> Voting is now open for the Improved TLS Defaults RFC and will run through
>> Wednesday Feb. 19:
>>
>> https://wiki.php.net/rfc/improved-tls-defaults#vote
>>
>> Note that while the implementation is vote-ready at this time (and
>> includes
>> several .phpt files) I'll continue adding more tests in the coming days.
>> If
>> you have questions that you feel have not been addressed during the past
>> two weeks please feel free to ask.
>>
>
> Overall, I would be very happy to see these changes sooner rather than
> later. That said, there is a *super* minor thing that could do with being
> sorted one way or another.
>
> Deprecation of "tls://". Please can I raise a dissenting voice just for
> that particular transport; it currently serves its purpose very well (I can
> has TLS, kthxbye) and asking us to change code to use "ssl://" and add some
> extra context configuration to get the same result seems a little out
> there.  This particular change was not discussed in the other thread, in
> point of fact it read to me like "tls://" would not be deprecated [1]. What
> changed? In summary: I'd really rather "tls://" not issue an E_DEPRECATED.
>
>

I agree, this one is an oversight on my part. The tls:// wrapper "just
makes sense" and deprecating it will likely result in little more than
annoyance. I am going to remove the E_DEPRECATED in these cases.

Oh, another super minor thing; could you make it 100% clear (I *may* be the

> only one not crystal clear!) about how this RFC affects changes that have
> previously been pushed to 5.6 that you wish to undo, and not undo.
>
>

Certainly, sorry for not making it clear in the RFC. The only changes that
have previously been merged for 5.6 that will be rolled-back are the
following stream wrappers:

- tlsv1.1://
- tlsv1.2://

I originally submitted these new wrappers, so hopefully I'm not hurting
anyone's feelings by shuttering them before they ever make it to a release
:)

Going forward the primary tls:// wrapper can be translated as "TLSv1,
TLSv1.1 or TLSv1.2." If code needs to use one of the version-specific TLS
protocols they can specify the relevant constant in the "crypto_method"
context option. Prior to 5.6 the tls:// wrapper translated to "only
TLSv1.0."