Don't omit CN verification in DarwinSSL when an IP address is used. #93
Conversation
|
Again, I'd suggest discussing this before merging and deciding which behaviour is preferred. This fix only outputs a warning, but allowing an IP address in the CN field doesn't seem right either. It's likely best to reject all attempts to connect via SSL to an IP address unless -k is used. |
|
I researched this more and determined that the behaviour of the backends other than the SChannel one is appropriate here. The patch has been corrected accordingly. |
|
Thanks, I've asked Nick Zitzmann to review this. |
|
I'm okay with this (and the equivalent Windows change), though I don't have any way to test this since I don't know of any TLS hosts with static IP addresses and legit certificates issued for the address. Does anyone? It ought to work in theory, though... |
|
This can be tested by creating a test CA which signs the certificate with static IP address, and using it with the --cacert CLI option. |
|
FYI: https://gist.github.com/rmoriz/fb2b0a6a0ce10550ab73 Sorry guys for not contacting you first. When I disocvered the issue I found http://curl.haxx.se/mail/archive-2013-10/0036.html and thought it's probably something that Apple broke. Maybe that got Apple on the way to fix the way more important CVE-2014-1266 issue… 😏 |
|
Right, this bug is indeed completely independent from Apple's CVE-2014-1263. I don't think CAs are supposed to sell certificates for IP addresses so they should be rare in the public, but of course you can self-sign and make your own CA cert etc. |
darwinssl: don't omit CN verification when an IP address is used
|
Okay, I merged the pull request. And this does sound just like CVE-2014-1263; CVE-2014-1266 is Apple's catastrophic #gotofail hole, which was patched earlier today. |
|
Yep, this is CVE-2014-1263. Interestingly, SChannel was affected by the same issue. See my other (now-closed) pull request. |
On OS X, curl does not output any sort of warning or error if an https request is made with an IP address rather than a hostname, no matter what the CN field in the SSL certificate is. This is because the SSLSetPeerDomainName() call is required even for basic hostname checking. It also performs SNI checking if available.
This fix prints a warning but allows the connection to go through, which matches curl's behaviour when compiled on Windows with SChannel. Most of the other vtls backends do verify the IP address against the CN field. If this is preferred, changing the behaviour here to be consistent would be trivial. Ideally, all of the vtls backends would be consistent — this would be fairly easy to fix, but I am unable to test some of the platform-specific ones. I'm unsure which approach to take, as having an IP address in the CN field does not appear to be proper.
(This appears to be the cause of the issue mentioned at the bottom of https://www.imperialviolet.org/2014/02/22/applebug.html which threw people off for a while.)