New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't omit CN verification in DarwinSSL when an IP address is used. #93

Merged
merged 1 commit into from Feb 25, 2014

Conversation

Projects
None yet
4 participants
@d235j
Contributor

d235j commented Feb 23, 2014

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.)

@d235j

This comment has been minimized.

Contributor

d235j commented Feb 23, 2014

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.

@d235j

This comment has been minimized.

Contributor

d235j commented Feb 23, 2014

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.

@bagder

This comment has been minimized.

Member

bagder commented Feb 24, 2014

Thanks, I've asked Nick Zitzmann to review this.

@nickzman

This comment has been minimized.

Collaborator

nickzman commented Feb 24, 2014

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...

@d235j

This comment has been minimized.

Contributor

d235j commented Feb 25, 2014

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.
IP addresses SHOULD be located in the SAN field, not the CN field, but both methods should be tested.

@rmoriz

This comment has been minimized.

rmoriz commented Feb 25, 2014

FYI: https://gist.github.com/rmoriz/fb2b0a6a0ce10550ab73
CVE-2014-1263

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… 😏

@bagder

This comment has been minimized.

Member

bagder commented Feb 25, 2014

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.

nickzman added a commit that referenced this pull request Feb 25, 2014

Merge pull request #93 from d235j/darwinssl_ip_address_fix
darwinssl: don't omit CN verification when an IP address is used

@nickzman nickzman merged commit e9665e9 into curl:master Feb 25, 2014

1 check passed

default The Travis CI build passed
Details
@nickzman

This comment has been minimized.

Collaborator

nickzman commented Feb 25, 2014

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.

@d235j

This comment has been minimized.

Contributor

d235j commented Feb 25, 2014

Yep, this is CVE-2014-1263.

Interestingly, SChannel was affected by the same issue. See my other (now-closed) pull request.
I was unable to test a few of the VTLS backends..

@d235j d235j deleted the d235j:darwinssl_ip_address_fix branch Feb 25, 2014

captain-caveman2k added a commit that referenced this pull request Mar 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment