attempt to verify SSL/TLS connections by default

[AdamWill]

This is an alternative approach to issue #196 (compared to
tosiara's PR #197). It attempts a compromise between security
and backward compatibility.

It introduces two new variables, SMTPTLSRequireValidation and
SMTPTLSTrustStore. The former indicates whether certificate
validation is required, and has three settings - null, true,
and false - with null indicating 'not explicitly chosen'. If
set to false, all validation checks are explicitly turned off.
If null or true, we try to find a certificate store, either
from the setting of SMTPTLSTrustStore (if set), from a couple
of well-known locations for the two major distribution groups
(PHP < 5.6), or by simply accepting PHP's default behaviour
which will use OpenSSL's default trust store if possible (PHP >=
5.6). If we cannot find one, secure connections will fail
if RequireValidation is true, or certificate validation will
be disabled and a debug message sent if RequireValidation is
null.

Whenever RequireValidation is true or null, secure connections
will fail if the server host name does not match CN in the
certificate.

The new default behaviour is somewhat tighter than before the
change for PHP versions earlier than 5.6, as previously the
only check was for self-signed certificates (PHP rejects these
by default, at least PHP 5.5 does); the hostname vs. CN check
is new in this case. However, it's a much less major change
than tosiara's approach.

[AdamWill]

For the record: it'd be nice to catch cases where the connection fails because of validation errors and return a good error, but it doesn't look entirely straightforward. I believe we'd have to add an error handling function which basically did string matching on the error messages to class.smtp.php with set_error_handler(). At present when a check fails, what you get is the rather unhelpfully generic "Failed to connect to server" error, because the SMTP class' connect() calls stream_socket_client with @ to suppress errors, then just sets that generic error message if the connection fails.

[AdamWill]

Couple of other notes: though it may not be entirely obvious from the code (I did try to mention it in comments), on PHP 5.6, if neither of the variables is set, you're basically getting PHP's default behaviour. verify_peer and allow_self_signed are set explicitly rather than relying on defaults, but that's about the only change. The big block hits the comment:

// Here, RequireValidation is null or true, PHP ver is >=5.6

where we do...nothing. This means that for >= 5.6 (unless SMTPTLSRequireValidation is explicitly set false), if a trust store is not specified or cannot be found, secure connections will fail - the behaviour is different (and stricter) than < 5.6. I decided to leave it that way because it's what upstream PHP wants, and it's what the current code does - if you use an unmodified master PHPMailer with PHP 5.6 and nerf it so it can't find a trust store, secure connections will fail.

I was also in a few minds about where to implement this, but in the end I think smtpConnect() is probably best. I don't like @tosiara 's approach of doing it in smtpSend() because it doesn't cover any case where smtpConnect() is called directly for some reason (but if you'd prefer to have it in smtpSend(), it's trivial to move it there). I thought about doing it in the SMTP class' connect() even, but there's a couple of problems with that - we don't know whether the connection is 'secure' at that point so we either have to go to the trouble of guessing or fail even non-secure connections if the trust store is invalid or it can't be found and RequireValidation is true. It also wouldn't help anyone switching out SMTP classes, whereas doing it in the Mailer class' smtpConnect() does, so long as they properly pass on $options.

One more thing - note that this still doesn't do any revocation checks. PHP doesn't do any by default (AFAICS) and neither does OpenSSL. I'm not too beat up about that, because revocation checking is a) really hard and b) of questionable benefit (there are other perspectives, and some later back and forth, but that one's pretty well argued).

[Synchro]

Thanks for all the effort you've put in here. This all sounds very sane. I'm still in xmas mode, so I'll get back on this soon! Have a great new year!

[w3d]

... I believe we'd have to add an error handling function which basically did string matching on the error messages to class.smtp.php with set_error_handler(). At present when a check fails, what you get is the rather unhelpfully generic "Failed to connect to server" error, because the SMTP class' connect() calls stream_socket_client with @ to suppress errors, then just sets that generic error message if the connection fails.

Would this be optional? Only thought being that if the app already used a custom error handler that suitably logged suppressed errors. (?) At least be sure to restore_error_handler() afterwards.

[AdamWill]

@w3d I honestly just browsed the doc page for like five minutes before figuring 'eh, this looks like a lot of work' and skipping it. but note custom error handlers don't have to do everything; at any point they can return 'false' and the error falls through to the previous error handler. so my thought was one which simply looks out for SSL/TLS-y errors and does something appropriate, and falls through to the stock handling for everything else. it's one of those things where I'd have to start building it to see how much trouble it is and whether there's any hidden bits I didn't understand. (I think you can also check if there's already a custom error handler, so maybe only do this if there isn't?)

the alternative is just to half-ass it and rewrite the generic error to say something like "this may be caused by an SSL/TLS validation error", of course.

edit: on the other hand, http://php.net/manual/en/function.openssl-error-string.php is apparently a thing, which...yeah, why is that a thing? but it might be useful here? I don't know if @ suppresses those errors or not. I'll check it out.

[AdamWill]

So I poked about at better error handling for a bit but didn't really get anywhere :/

For SSL connections, stream_socket_client fails (returns false), but gives us errno 0 and an empty errstr. So we hit the if (!is_resource($this->smtp_conn)) block and produce that fairly useless error. openssl-error-string() doesn't seem to produce anything in this case. I tried setting a stream context callback function, but that doesn't seem to get called at all. So, I can't see a way to get the actual error for SSL connections other than dropping the error suppression in the call to stream_socket_client and writing a custom error handler, as I first thought.

For TLS connections, we wind up on the expected failure path - STARTTLS fails - so I think that's being handled the way you want it to be handled.

I've no idea if you find the SSL failure behaviour acceptable; if not, let me know what you'd like done about it. Note stream_socket_client actively sets errno to 0; if you change its initial value in SMTP's connect() to any other value then trigger an SSL validation error, it comes out as 0 (not the different initial value you set). So we could possibly set the initial value to -1 or something and then check for 0 and set an appropriate $this->error and debug message in that case, something like 'this may be caused by SSL certification validation failure'?

[AdamWill]

Ping?

[Synchro]

Hi - I'm guessing you saw I closed the other TLS-related ticket as that's now handled with the SMTPOptions property to set stream context options. Now that we have that, it should make what you've been suggesting a bit easier. We also have opportunistic auto-TLS now too.

All you've said sounds very sane, and I don't have a problem causing SSL errors where it's appropriate. There have been quite a few changes recently and your patch won't apply cleanly - can you have a look at it and see how all this can fit together?

I was planning on releasing 5.2.10 as-is tomorrow, but if you have some time to spend on this, I can postpone that.

...and apologies for not getting back to you before.

[AdamWill]

Thanks! I'll see if I can find some time this week - if you don't hear from me by Friday, though, assume I failed :)

[AdamWill]

I've re-diffed the patch. It didn't really need much changing.

The SMTPOptions thing doesn't really help here AFAICS. Really, neither the SMTPOptions change nor the opportunistic-TLS change affects things hugely. Only thing is that I made the option setting unconditional so that the options are set if TLS gets enabled opportunistically - I tested a bit, and having the ssl options set when initializing a non-SSL/TLS connection seems not to cause any problems.

[Synchro]

This is all a bit academic now. PHP 5.6 is the only 5.x version stil current, and it solves the verification problem - it's easier to upgrade PHP than to try to resolve this for older versions.

[AdamWill]

And of course, the world has an unblemished track record of only using 'current' versions of PHP ;)

but, I don't really care any more; I decided a couple of years back to just stop dealing with anything at all to do with PHP as much as possible.

[Synchro]

Perl, Python and Ruby all did exactly the same as PHP; All started out not verifying certs, and all had exactly the same set of problems when they changed, so this isn't a PHP-specific thing at all. Those that are not upgrading PHP are unlikely to be updating PHPMailer either, so fixing things for legacy issues doesn't really help.

[AdamWill]

It wasn't particularly to do with this specific issue.