libcurl+openssl does not accept https certificate iPAddress if dNSName is present

[jay]

@wmsch reported in #875 that IP address verifications are failing when both IP addresses and hostnames are present in the SAN:

If an IP address is specified in the request instead of a name, and it matches an address in the iPAddress field, and a dNSName field exists, shouldn't this still work? It does not work for me in 7.50.1 if both iPAddress and dNSName are provided. If dNSName does not exist, it works.

RFC 2818 says this:

If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used. Although the use of the Common Name is existing practice, it is deprecated and Certification Authorities are encouraged to use the dNSName instead.

...but also:

In some cases, the URI is specified as an IP address rather than a hostname. In this case, the iPAddress subjectAltName must be present in the certificate and must exactly match the IP in the URI.

seems unclear. if you have an iPAddress and dNSName in there I would think the intention is either... thoughts?

tested master 2c8ccda 2016-08-11, returns:
curl: (51) SSL: no alternative certificate subject name matches target host name '127.0.0.1'

Tested Firefox and Chrome and they accept iPAddress even when dNSName is present.

To reproduce:

Create a file server.cnf:

[req]
distinguished_name = req_distinguished_name
req_extensions = v3_req
prompt = no

[req_distinguished_name]
CN = FOOBAR

[v3_req]
basicConstraints = CA:FALSE
keyUsage = digitalSignature, keyEncipherment
subjectAltName = @alt_names

[alt_names]
DNS.1 = DOESNOTEXIST
IP.1 = 127.0.0.1

Create a file ca.cnf:

[req]
distinguished_name = req_distinguished_name
prompt = no

[req_distinguished_name]
CN = CA_FOOBAR

Create the certificates:

openssl req -config ca.cnf -x509 -nodes -days 1095 -newkey rsa:2048 -x509 -out ca.crt -keyout ca.key
cat ca.key ca.crt > ca.pem

openssl req -config server.cnf -new -nodes -days 1095 -newkey rsa:2048 -out server.req -keyout server.key
openssl x509 -extfile server.cnf -sha256 -req -in server.req -out server.crt -CA ca.crt -CAkey ca.key -CAcreateserial -days 1095 -extensions v3_req
cat server.key server.crt > server.pem

Listen:

socat openssl-listen:4433,reuseaddr,cert=server.pem,verify=0,fork -

Connect:

curl -v --cacert ca.crt https://127.0.0.1:4433/

Output:

* Server certificate:
*  subject: CN=FOOBAR
*  start date: Aug 12 06:08:21 2016 GMT
*  expire date: Aug 12 06:08:21 2019 GMT
*  subjectAltName: host "127.0.0.1" matched cert's IP address!
*  subjectAltName does not match 127.0.0.1
* SSL: no alternative certificate subject name matches target host name '127.0.0.1'
* multi_done
* Closing connection 0
* The cache now contains 0 members
* TLSv1.2 (OUT), TLS alert, Client hello (1):
curl: (51) SSL: no alternative certificate subject name matches target host name '127.0.0.1'

Version:

curl 7.50.2-DEV (i386-pc-win32) libcurl/7.50.2-DEV OpenSSL/1.0.2h nghttp2/1.11.1
Protocols: dict file ftp ftps gopher http https imap imaps ldap pop3 pop3s rtsp
smb smbs smtp smtps telnet tftp
Features: AsynchDNS Debug Largefile NTLM SSL HTTP2

Possible solution if we want to allow either:

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 3027ca3..18c878e 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1115,8 +1115,6 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
       /* get a handle to alternative name number i */
       const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i);

-      /* If a subjectAltName extension of type dNSName is present, that MUST
-         be used as the identity. / RFC2818 section 3.1 */
       if(check->type == GEN_DNS)
         dNSName = TRUE;

@@ -1164,11 +1162,8 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
     }
     GENERAL_NAMES_free(altnames);

-    if(dnsmatched || (!dNSName && ipmatched)) {
-      /* count as a match if the dnsname matched or if there was no dnsname
-         fields at all AND there was an IP field match */
+    if(dnsmatched || ipmatched)
       matched = TRUE;
-    }
   }

   if(matched)

[bagder]

Yeah, I was clearly mislead by that first quote and didn't do the full check. I think we should go for compatibility with how the browsers treat this and allow the IP match. Thanks @jay !

[jay]

Ok. Another thing, if we are verifying an IP and there is at least one iPAddress field but no dNSName is it correct that if the match fails to default to CN like we are now? I haven't found much but this a python issue.

[bagder]

That's a very good question, but I would guess so. I also guess that as there are very few certs in use using iPAddress, there will be even less that actually put an IP address in the CN field so we might simply not hit this case in real life.

[jay]

Firefox and Chrome don't default to CN when iPAddress is present but dNSName is not. To repro follow above but use this server.cnf:

[req]
distinguished_name = req_distinguished_name
req_extensions = v3_req
prompt = no

[req_distinguished_name]
CN = 127.0.0.1

[v3_req]
basicConstraints = CA:FALSE
keyUsage = digitalSignature, keyEncipherment
subjectAltName = @alt_names

[alt_names]
IP.1 = 8.8.8.8

Chrome shows This server could not prove that it is 127.0.0.1; its security certificate is from 127.0.0.1. NET::ERR_CERT_COMMON_NAME_INVALID

Firefox shows The certificate is only valid for 8.8.8.8 Error code: SSL_ERROR_BAD_CERT_DOMAIN

(Edit: Note the connection is also rejected under the same premises if I change the CN and URL from 127.0.0.1 to a hostname (eg CN foo and I connect to https://foo:4433/).)

So assuming that's correct the fix looks more like this:

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 3027ca3..eedba0d 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1083,6 +1083,7 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
 #endif
   CURLcode result = CURLE_OK;
   bool dNSName = FALSE; /* if a dNSName field exists in the cert */
+  bool iPAddress = FALSE; /* if a iPAddress field exists in the cert */

 #ifdef ENABLE_IPV6
   if(conn->bits.ipv6_ip &&
@@ -1115,10 +1116,10 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
       /* get a handle to alternative name number i */
       const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i);

-      /* If a subjectAltName extension of type dNSName is present, that MUST
-         be used as the identity. / RFC2818 section 3.1 */
       if(check->type == GEN_DNS)
         dNSName = TRUE;
+      else if(check->type == GEN_IPADD)
+        iPAddress = TRUE;

       /* only check alternatives of the same type the target is */
       if(check->type == target) {
@@ -1164,18 +1165,14 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
     }
     GENERAL_NAMES_free(altnames);

-    if(dnsmatched || (!dNSName && ipmatched)) {
-      /* count as a match if the dnsname matched or if there was no dnsname
-         fields at all AND there was an IP field match */
+    if(dnsmatched || ipmatched)
       matched = TRUE;
-    }
   }

   if(matched)
     /* an alternative name matched */
     ;
-  else if(dNSName) {
-    /* an dNSName field existed, but didn't match and then we MUST fail */
+  else if(dNSName || (iPAddress && target == GEN_IPADD)) {
     infof(data, " subjectAltName does not match %s\n", conn->host.dispname);
     failf(data, "SSL: no alternative certificate subject name matches "
           "target host name '%s'", conn->host.dispname);

[bagder]

Great. And yes, going with mimicking the browsers' behavior seems like a good guidance here. 👍

[jay]

I turned this into a PR with the final version. I made one more change that needs review, I have it fail now in the case of iPAddress present but no-match regardless of what type of target we have. I did some tests and that is consistent with the way Firefox and Chrome do it. If I set the CN to localhost but I have v3 iPAddress then it ignores the CN even if iPAddress doesn't match.

[bagder]

👍 excellent and yes, I think that update makes sense. LGTM as they say.

[jay]

Landed in b6fcdc3

[wmsch]

I have used curl and the regression seems to have been fixed; however, an original comment from databus23 in issue #875 is still not addressed. databus23 wrote "Newer versions of curl don't work with our certificates anymore which only contain the hostname in the CN." This is an issue for us as well - perfectly good server certificates with the server hostname in CN no longer work - in 7.50.2.

I would suggest that CN be used as a fallback match on hostname if dNSName is missing from subjectAltName. I do not see any security issues with this approach. Additionally, wget matches on hostname in CN.