Skip to content
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

Certificate SAN ip addresses not checked correctly for GSKit TLS provider #3102

Closed
matthew1001 opened this issue Oct 5, 2018 · 2 comments
Closed
Labels

Comments

@matthew1001
Copy link

I did this

Ran an HTTPS application using libcurl + GSKit TLS provider, connecting to a server at https://192.168.0.2:9443 who's certificate contains a SAN of:

IP Address: 127.0.0.1
DNS Name: localhost
IP Address: 192.168.0.2
DNS Name: my.local.server

I expected the following

For the connection to succeed

curl/libcurl version

curl 7.61.0 (x86_64-pc-linux-gnu) libcurl/7.61.0
Release-Date: 2018-07-11
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile SSL UnixSockets

operating system

Fedora 28

problem

It appears that the implementation of Curl_verifyhost in x509asn1.c does a memcmp of &addr and q (where q = Curl_getASN1Element(...)). Running with a debugger appears to show that (name.end - q) = 0, which when compared to addrlen fails, preventing the memcmp even happening.

I wonder if it should it be comparing addrlen to (name.end - name.beg) & memcmp-ing &addr with name.beg?

@jay
Copy link
Member

jay commented Oct 5, 2018

Yeah that doesn't look right. On success the pointer assigned to q should always be the same as what was assigned to name.end which means it would always be 0. I almost can't believe nobody's ever caught that though it must not be a heavily used function since it's gskit. I agree your solution sounds correct. Are you able to test it out and write up a PR for it?

@bagder bagder added the TLS label Oct 5, 2018
@matthew1001
Copy link
Author

I believe my local patch is working correctly. I'll look at submitting a PR.

matthew1001 added a commit to matthew1001/curl that referenced this issue Oct 15, 2018
field of a certificate.

For IP addresses in the subject alternative name field, the length
of the IP address (and hence the number of bytes to perform a
memcmp on) is incorrectly calculated to be zero. The code previously
subtracted q from name.end. where in a successful case q = name.end
and therefore addrlen equalled 0. The change modifies the code to
subtract name.beg from name.end to calculate the length correctly.

Fixes curl#3102
@jay jay closed this as completed in df54b14 Oct 16, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

3 participants