TLS: allow any protocol version

[Slamdunk]

Since PHP 5.6.7 the constant STREAM_CRYPTO_METHOD_TLS_CLIENT doesn't refer to any TLS version, but only to TLS 1.0.

As of yet every zend-mail users are forced to use TLS 1.0 version, and TLS 1.1 and TLS 1.2 are excluded.
We need to readd them manually.

1. https://3v4l.org/llTT8
2. stream_socket_enable_crypto manual page
3. PHP 5.6.7 revert commit
4. PHPMailer evidence
5. New RFC for PHP 7.2

I consider this a very urgent fix.

[Slamdunk]

@weierophinney ping

[ezimuel]

@Slamdunk instead of copy the $cryptoMethod code in all the classes, we can refactor it using a Trait. Moreover, we need a unit test for that.

[Slamdunk]

@ezimuel Ok for centralizing the code, but how to test it?

1. The code contains no logic
2. The test would equal the code itself, I'm not sure something like this would be useful:

class StreamSocketTest extends PHPUnit_Framework_TestCase
{
    public function testTlsVersion()
    {
        $cryptoMethod = STREAM_CRYPTO_METHOD_TLS_CLIENT;
        if (defined('STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT')) {
            $cryptoMethod |= STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT;
            $cryptoMethod |= STREAM_CRYPTO_METHOD_TLSv1_1_CLIENT;
        }
        
        $this->assertSame($cryptoMethod, StreamSocket::getCryptoMethod());
    }
}

[ezimuel]

@Slamdunk for the unit test we should check that using PHP 5.6.7+ the crypto constants are ok. We can use the PHPUnit annotation @requires PHP 5.6.7 for this test.

Moreover, I suggested to use a Trait and not a static function of a new class. You can use a name like ProtocolTrait. Can you change this? Thanks!

[Slamdunk]

@ezimuel A test was added in the way that I found the most worthwhile.

I left the code in a new class because traits can easily get bypassed: https://3v4l.org/3DrmE .
The new class express directly the intent, and its behaviour cannot be modified: if the choise of the crypto method must be the same for all the protocols, no protocol should have the chance to alter it.

Do you have particular reasons for using them in this case?

[ezimuel]

@Slamdunk the unit test looks perfect! Regarding the Trait I think this is a perfect fit because we just need to share the same code across different classes that does not extends or implements a shared interface.
Regarding your point of the bypass, this is perfectly fine, it's not an issue. If in the future, for any reason, a protocol needs specific configurations it can override it. Moreover, we are providing a Trait for an internal components of a library, that means you should always change the code of the library to override it.

[Slamdunk]

@ezimuel I believe the code should protect itself in every scenario: you should be able to change the internal code of a library only if you become aware of every reason behide the already written lines.

With a Trait you are able to modify/override the code and get the tests pass without alteration: this would be a security hole.

Instead, if in the future some evidence will require this code to be altered, you can't skip reading and understanding the current reasons.

[ezimuel]

@Slamdunk IMHO I don't see any security issue using a Trait. Again, if someone wants to override the getCryptoMethod() he/she needs to change always the source code of the library.
If this is your argument against Trait, when you will use it? I guess never :)

Anyway, I would like to have a different feedback on this, @weierophinney what do you think?

[Slamdunk]

If this is your argument against Trait, when you will use it? I guess never :)

Indeed 👍

[weierophinney]

@Slamdunk I'm failing to see how this would present a security issue. Composing a trait is equivalent to compile-time copy-and-paste, and any changes to the trait would be reflected in changes in behavior, and thus captured in the test suite already. Using a final static class places a hard dependency within the classes using this instead, and a change to that is also reflected in the test suite results.

My point is: It's not a security issue; it's an issue of preference. As a project, we've generally preferred usage of traits vs static methods for code since we updated to PHP 5.6 as our minimum supported version.

[Slamdunk]

@weierophinney The issue here is that we are not testing this bug+bugfix.

In order to test this bug we should create a (mock) socket listener and try to connect to it in different cases with different parameters for every Zend\Mail\Protocol available.
IF we could do this, I would agree with you.

Since we can't do this, suppose the Trait is written and used; it's passed a year, and someone creates a PR like this:

// class Smtp
+private function getCryptoMethod()
+{
+    return STREAM_CRYPTO_METHOD_TLS_CLIENT;
+}
+
+public function createTlsSocket($resource)
+{
+    return stream_socket_enable_crypto($resource, true, $this->getCryptoMethod());
+}

This would actually break Smtp::helo method without notice; all tests will go green because there are no test as of yet that verify that Smtp::helo is handling TLSv1.2.
How are you going to spot the error if you don't remember each and every line of the project you are maintaining?

I see only two choises:

1. We create real tests for each stream_socket_enable_crypto usage and use-case
2. We enforce with code to think what we are changing

[weierophinney]

@Slamdunk Again, in that case, we'd see the addition of the new method during review, giving us time to consider its impact. It's not much different than changing the argument to stream_socket_enable_crypto(), which could also happen via a PR, and which would also spawn a serious review. If we have tests in place to verify what arguments are provided to that method, such changes would require accompanying test changes, which would spawn red flags during the review process. Without tests, they will not, even if you have a separate class and method for generating the values.

Which brings me to what I feel is your most important point — that tests do not exist yet to verify that these calls are happening appropriately. This is the crux of the matter, and is not solved with just creating a static class method that you've tested; we need to test that the adapters are passing these flags.

You can mock functions with tools like Mockery and/or by recreating the functions within the same namespace within which they are invoked in order to determine what arguments are passed. While these take some effort, they also ensure that no breaking changes are introduced later.

[Slamdunk]

Again, in that case, we'd see the addition of the new method during review, giving us time to consider its impact. It's not much different than changing the argument to stream_socket_enable_crypto(), which could also happen via a PR, and which would also spawn a serious review

Maybe I wasn't clear in my example.

1. With a Trait and with Protocol/Smtp.php#L179 using $this->getCryptoMethod() notation you can change its behaviour just by adding new coding and you can't notice it
2. With a static call in Protocol/Smtp.php#L179 you cannot change its behaviour without changing it and thus noticing it

By the way, I have to admit that I'm a bit confused: stated that I am a TDD-nazi, I feel like you are saying that a stronger and more defensive solution I am proposing isn't good, and that I have to changing it to make it weaker or I have to implement all the tests that this library is missing from the day it was created, and all this for a security issue that usually need to be merged as soon as possible, since PFS and GCM offered in TLSv1.2 is now negated.

I would like to implement all the tests, but I can't in the next few days; likely not before mid june I'll be available for such a huge step.

[weierophinney]

@Slamdunk — Regarding this:

With a Trait and with Protocol/Smtp.php#L179 using $this->getCryptoMethod() notation you can change its behaviour just by adding new coding and you can't notice it

This is simply not true: a pull request would be introducing a new method in the class, which would be noticeable. Moreover, it would only affect the one class, not every class composing the trait, which limits the impact to only consumers of that single adapter, should we choose to merge such a patch.

I feel like you are saying that a stronger and more defensive solution I am proposing isn't good, and that I have to changing it to make it weaker or I have to implement all the tests that this library is missing from the day it was created, and all this for a security issue that usually need to be merged as soon as possible, since PFS and GCM offered in TLSv1.2 is now negated.

We're not saying that the solution you propose is not good, or proposing you make it weaker; we're proposing that you modify it to be more consistent with styles and approaches we use throughout the ZF project. What your patch has demonstrated is that we were missing tests, which was highlighted by the fact that making the changes you did did not result in any test breakage. The proper approach is to add those tests to ensure when a change like this occurs in the future, we can evaluate it properly for backwards compatibility impact.

The approach you suggest has the benefit of being simpler to test. However, if somebody were to modify any of the calls to stream_socket_enable_crypto() to no longer call the static method, we'd still have no test failures related to this, which could lead to breakage. That's why we're trying to evaluate whether or not to take a more robust testing approach now.

[Slamdunk]

I've rewritten the PR to accomplish your wish.

I have to note that I'm against this very implementation, and I've followed what you requested only because it's a right point to have project standards.
Apart from that, I can't consider Traits anything but brittle design code, mainly for this reason https://3v4l.org/3DrmE

About testing stream_socket_enable_crypto, the work is big: if it would be up to me, I would wrap stream_* functions in a separate Stream class, make it an external dependancy for current protocol classes, and then mock it for protocol classes tests. What would you do?

[ezimuel]

@Slamdunk thanks for your work! I think we can definitely improve the unit tests of zend-mail and a possible solution is the one that you suggested. I'll let you know if will be the case.
Thanks again for your time! I'm going to merge this in a while.