Bug 5363: Handle IP-based X.509 SANs better

src/security/cert_generators/file/Makefile.am

@@ -24,6 +24,7 @@ security_file_certgen_SOURCES = \
 
 security_file_certgen_LDADD = \
 	$(top_builddir)/src/ssl/libsslutil.la \
+	$(top_builddir)/src/ip/libip.la \
 	$(top_builddir)/src/sbuf/libsbuf.la \
 	$(top_builddir)/src/debug/libdebug.la \
 	$(top_builddir)/src/error/liberror.la \

src/ssl/gadgets.cc

@@ -5,10 +5,13 @@
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
-
+#include <arpa/inet.h>
+#include <cstdio>
+#include <iostream>
 #include "squid.h"
 #include "base/IoManip.h"
 #include "error/SysErrorDetail.h"
+#include "ip/Address.h"
 #include "sbuf/Stream.h"
 #include "security/Io.h"
 #include "ssl/gadgets.h"
@@ -484,8 +487,13 @@ addAltNameWithSubjectCn(Security::CertPointer &cert)
     if (!cn_data)
         return false;
 
+    Ip::Address ipAddress;
+    // Extract the CN string from cn_data and use it to determine its type
+    // using Ip::Address::operator "="
+    const char* cn_cstr = (const char*) ASN1_STRING_get0_data(cn_data);
+    const char* altname_type = (ipAddress = cn_cstr) ? "IP" : "DNS";
     char dnsName[1024]; // DNS names are limited to 256 characters
-    const int res = snprintf(dnsName, sizeof(dnsName), "DNS:%*s", cn_data->length, cn_data->data);
+    const int res = snprintf(dnsName, sizeof(dnsName), "%s:%.*s", altname_type, cn_data->length, cn_data->data);
     if (res <= 0 || res >= static_cast<int>(sizeof(dnsName)))
         return false;
 

src/ssl/gadgets.h

@@ -292,7 +292,6 @@ bool CertificatesCmp(const Security::CertPointer &cert1, const Security::CertPoi
 /// wrapper for OpenSSL X509_get0_signature() which takes care of
 /// portability issues with older OpenSSL versions
 const ASN1_BIT_STRING *X509_get_signature(const Security::CertPointer &);
-
 } // namespace Ssl
 
 #endif // USE_OPENSSL

src/ssl/support.cc

@@ -15,13 +15,16 @@
  */
 #if USE_OPENSSL
 
+#include <openssl/asn1.h>
 #include "acl/FilledChecklist.h"
 #include "anyp/PortCfg.h"
 #include "anyp/Uri.h"
+#include <arpa/inet.h>
 #include "fatal.h"
 #include "fd.h"
 #include "fde.h"
 #include "globals.h"
+#include "ip/Address.h"
 #include "ipc/MemMap.h"
 #include "security/CertError.h"
 #include "security/Certificate.h"
@@ -55,6 +58,79 @@ std::vector<const char *> Ssl::BumpModeStr = {
     /*,"err"*/
 };
 
+// Methods for the VerifyAddress class
+//
+// The public match method can be called with an ASN1_STRING which
+// could either be an X509_NAME or GENERAL_NAME
+int VerifyAddress::match(ASN1_STRING *asn1_data) {
+    // declare an empty server var which will be updated later
+    const char *server = nullptr;
+    if (ASN1_STRING_type(asn1_data) == V_ASN1_OCTET_STRING) {
+        // We've been passed an IP address from a GENERAL_NAME struct
+        // Attempt to safely convert this to a string
+        const unsigned char *ip = ASN1_STRING_get0_data(asn1_data);
+        char buffer[INET6_ADDRSTRLEN];
+        if (ASN1_STRING_length(asn1_data) == 4) {
+            // IPv4
+            inet_ntop(AF_INET, ip, buffer, sizeof(buffer));
+        } else if (ASN1_STRING_length(asn1_data) == 16) {
+            // IPv6
+            inet_ntop(AF_INET6, ip, buffer, sizeof(buffer));
+        } else {
+            debugs(83, 4, "Tried to get an IP from ASN1_OCTET_STRING but failed");
+            return 1;
+        }
+        server = strdup(buffer);
+    } else {
+        // Otherwise, assume we have the cn_data from a typical ASN1_STRING
+        // This could be a domainname or IP address
+        server = (const char *)asn1_data->data;
+    }
+    // Attempt to convert server into an IP and use matchIP if it's valid
+    Ip::Address ipAddress;
+    if (ipAddress = server) {
+        debugs(83, 4, "Verifying server IP " << private_check_data << " to certificate name/subjectAltName " << server);
+        return matchIp(ipAddress);
+    }
+    // Otherwise, match the original address as a domain name
+    return matchDomainNameData(asn1_data);
+}
+
+int VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) {
+    char cn[1024];
+
+    if (cn_data->length == 0)
+        return 1; // zero length cn, ignore
+
+    if (cn_data->length > (int)sizeof(cn) - 1)
+        return 1; //if does not fit our buffer just ignore
+
+    char *s = reinterpret_cast<char*>(cn_data->data);
+    char *d = cn;
+    for (int i = 0; i < cn_data->length; ++i, ++d, ++s) {
+        if (*s == '\0')
+            return 1; // always a domain mismatch. contains 0x00
+        *d = *s;
+    }
+    cn[cn_data->length] = '\0';
+    debugs(83, 4, "Verifying server domain " << private_check_data << " to certificate name/subjectAltName " << cn);
+    return matchDomainName(private_check_data, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains);
+}
+
+int VerifyAddress::matchIp(Ip::Address &iaddr) {
+    // Attempt to convert private_check_data to an IP address
+    // and compare it to iaddr
+    Ip::Address check_ip;
+    if ((check_ip = private_check_data) && (iaddr == check_ip)) {
+        return 0;
+    }
+    return 1;
+}
+
+const char* VerifyAddress::processCheckData(void *check_data) {
+    return reinterpret_cast<const char*>(check_data);
+}
+
 /**
  \defgroup ServerProtocolSSLInternal Server-Side SSL Internals
  \ingroup ServerProtocolSSLAPI
@@ -213,14 +289,24 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun
         int numalts = sk_GENERAL_NAME_num(altnames);
         for (int i = 0; i < numalts; ++i) {
             const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i);
-            if (check->type != GEN_DNS) {
-                continue;
-            }
-            ASN1_STRING *cn_data = check->d.dNSName;
-
-            if ( (*check_func)(check_data, cn_data) == 0) {
-                sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free);
-                return 1;
+            switch(check->type) {
+                case GEN_DNS:
+                    // If the type is GEN_DNS, call check_func with the dNSName data
+                    if ( (*check_func)(check_data, check->d.dNSName) == 0) {
+                        sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free);
+                        return 1;
+                    }
+                    break;
+                case GEN_IPADD:
+                    debugs(83, 4, "Check type is GEN_IPADD, verifying...");
+                    // If it's an IP address, call check_func with the iPAddress data
+                    if ( (*check_func)(check_data, check->d.iPAddress) == 0) {
+                        sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free);
+                        return 1;
+                    }
+                    break;
+                default:
+                    continue;
             }
         }
         sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free);
@@ -230,25 +316,8 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun
 
 static int check_domain( void *check_data, ASN1_STRING *cn_data)
 {
-    char cn[1024];
-    const char *server = (const char *)check_data;
-
-    if (cn_data->length == 0)
-        return 1; // zero length cn, ignore
-
-    if (cn_data->length > (int)sizeof(cn) - 1)
-        return 1; //if does not fit our buffer just ignore
-
-    char *s = reinterpret_cast<char*>(cn_data->data);
-    char *d = cn;
-    for (int i = 0; i < cn_data->length; ++i, ++d, ++s) {
-        if (*s == '\0')
-            return 1; // always a domain mismatch. contains 0x00
-        *d = *s;
-    }
-    cn[cn_data->length] = '\0';
-    debugs(83, 4, "Verifying server domain " << server << " to certificate name/subjectAltName " << cn);
-    return matchDomainName(server, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains);
+    VerifyAddress verify(check_data);
+    return verify.match(cn_data);
 }
 
 bool Ssl::checkX509ServerValidity(X509 *cert, const char *server)

src/ssl/support.h

@@ -16,6 +16,7 @@
 #include "base/CbDataList.h"
 #include "comm/forward.h"
 #include "compat/openssl.h"
+#include "ip/Address.h"
 #include "sbuf/SBuf.h"
 #include "security/Session.h"
 #include "ssl/gadgets.h"
@@ -366,6 +367,21 @@ class VerifyCallbackParameters {
 
 } //namespace Ssl
 
+class VerifyAddress {
+public:
+    // Constructor
+    VerifyAddress(void *check_data)
+    : private_check_data(processCheckData(check_data))
+    {}
+    int match(ASN1_STRING *cn_data);
+private:
+    int matchDomainNameData(ASN1_STRING *cn_data);
+    int matchIp(Ip::Address &iaddr);
+
+    const char* processCheckData(void *check_data);
+    const char* private_check_data;
+};
+
 #if _SQUID_WINDOWS_
 
 #if defined(__cplusplus)