Hello security-conscious internals people!
I've got (what believe to be) a pretty good working solution for the
problem of insecure-by-default stream encryption. I need to do some more
thorough testing before pushing it upstream to a public fork but here's the
quick and dirty:
--- Summary ---
- All encrypted streams have peer verification enabled by default.
- Global CA path defaults may be specified via new "openssl.cafile" and
"openssl.capath" php.ini directives. This has the advantage mentioned
upthread of allowing distros to customize the .ini file to point to an
existing CA file.
- Global CA path defaults may be specified at runtime via two new functions:
+ bool openssl_set_default_cafile(string $cafile)
+ bool openssl_set_default_capath(string $capath)
- Per-stream CA paths may still be specified at call time by passing the
existing "cafile" or "capath" ssl options in the stream context (the ini
directives are a fallback)
- If none of the above methods have been used to specify the necessary CA
info you get a painfully explicit E_WARNING explaining that peer
verification is necessary to prevent Man-in-the-Middle attacks and that you
need to specify a CA or disable peer verification (gasp).
- The peer name to be verified is automagically detected from the URI
(eliminating the need for *any* configuration by the user for
secure-by-default peer verification)
- A new ssl context option, "peer_name" is added to allow manual override
of the automatically detected name (can be useful if you're connecting via
an IP address instead of a name).
- Peer certs using the subjectAltName (SAN) extension are now honored when
verifying peer names. Currently we're violating the relevant spec and not
doing this at all. With this change a common name (CN) match is only
attempted if no SAN matches were found first. This is a BC improvement that
I'll PR for inclusion in 5.4/5.5 soon regardless of what happens with peer
verification changes.
--- BC ramifications ---
The new "peer_name" context option (coupled with automatic name detection)
totally invalidates the need for "CN_match" as well as "SNI_server_name"
... IMO these values are implementation details that are only necessary
when fixing the problem of not verifying peers by default. My changes solve
this problem and as a result I think the use of these context options
should be deprecated and result in E_NOTICE for 5.6 with removal scheduled
for 5.7. In the meantime these values, if passed in the context, will
replace the "peer_name" value. Also, while I'm on the subject of the
existing context options: I don't really see the point in keeping the
"SNI_enabled" context option going forward either. If the underlying
openssl libs support SNI then we should just use it automatically. Is there
some benefit to disabling it when it's available that I'm unaware of? IMO
"SNI_enabled" should receive the same deprecation treatment as "CN_match"
and "SNI_server_name" ...
As for existing code ... with the addition of a single line in php.ini
almost all preexisting code will "just work."
// way more secure, zero user knowledge needed, just works
file_get_contents('https://www.github.com');
If you have problems you can always pass a context with "verify_peer" =>
FALSE. The difference is that now instead of being horribly insecure by
default PHP would say, "okay, I'll let you do this if you really want to
but understand that it's not safe."
--- Other considerations ---
It may be overkill to have *2* ini directives and setter functions (one for
cafile and one for capath). I'm on the fence with this and would be fine
with killing the capath directive/setter in the interest of keeping things
simple. Finally, on the subject bundling a CA file with the distribution
... I don't care if we do or we don't. It's not that difficult to have a
manual page dedicated to explaining why you need a CA file to verify peers
in your encrypted transfers and offer instructions on how to procure one.
Even if we don't bundle a cafile with the distro the hit to BC is extremely
minimal and easy to remedy. Security is always a good reason to break BC.
Alas, this turned into the neverending email, so thanks if you made it this
far. Please share any thoughts, comments or questions. Or if you just want
to tell me I'm an idiot that's fine too. I'll get the code on github in the
next couple of days, but I want to incorporate any suggestions people may
have first.
