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

Comments

Projects
None yet
3 participants
@matthew1001

matthew1001 commented Oct 5, 2018

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

This comment has been minimized.

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 SSL/TLS label Oct 5, 2018

@matthew1001

This comment has been minimized.

matthew1001 commented Oct 15, 2018

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

matthew1001 pushed a commit to matthew1001/curl that referenced this issue Oct 15, 2018

Matthew Whitehead
Fix parsing of IP addresses in the subject alternative name (SAN)
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 in df54b14 Oct 16, 2018

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