]> sjero.net Git - wget/commitdiff
Implement support for subjectAltName fields in X509 certs.
authorPetr Pisar <petr.pisar@atlas.cz>
Sat, 24 Oct 2009 23:06:44 +0000 (16:06 -0700)
committerPetr Pisar <petr.pisar@atlas.cz>
Sat, 24 Oct 2009 23:06:44 +0000 (16:06 -0700)
src/ChangeLog
src/openssl.c

index d6c9dd56f7261304798f0536c1799b1d1a99cc93..56537c8dcb819a5f9d9e07a3b792ade519cd5523 100644 (file)
@@ -1,3 +1,8 @@
+2009-10-24  Petr Pisar  <petr.pisar@atlas.cz>
+
+       * openssl.c: Implement support for (multiple) subjectAltNames in
+       X509 certificates, not just the commonName.
+
 2009-10-09  Micah Cowan  <micah@cowan.name>
 
        * main.c: Fix declaration of compiled_features.
index b55ca8bfb5add32195151d9b35b6b1ce41414ad7..4eef5c6c49c82efa3510da3ce6c005cd50068d0c 100644 (file)
@@ -39,7 +39,7 @@ as that of the covered work.  */
 #include <string.h>
 
 #include <openssl/ssl.h>
-#include <openssl/x509.h>
+#include <openssl/x509v3.h>
 #include <openssl/err.h>
 #include <openssl/rand.h>
 
@@ -486,9 +486,11 @@ bool
 ssl_check_certificate (int fd, const char *host)
 {
   X509 *cert;
+  GENERAL_NAMES *subjectAltNames;
   char common_name[256];
   long vresult;
   bool success = true;
+  bool alt_name_checked = false;
 
   /* If the user has specified --no-check-cert, we still want to warn
      him about problems with the server's certificate.  */
@@ -536,7 +538,8 @@ ssl_check_certificate (int fd, const char *host)
           break;
         case X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN:
         case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
-          logprintf (LOG_NOTQUIET, _("  Self-signed certificate encountered.\n"));
+          logprintf (LOG_NOTQUIET,
+                     _("  Self-signed certificate encountered.\n"));
           break;
         case X509_V_ERR_CERT_NOT_YET_VALID:
           logprintf (LOG_NOTQUIET, _("  Issued certificate not yet valid.\n"));
@@ -558,10 +561,6 @@ ssl_check_certificate (int fd, const char *host)
   /* Check that HOST matches the common name in the certificate.
      #### The following remains to be done:
 
-     - It should use dNSName/ipAddress subjectAltName extensions if
-       available; according to rfc2818: "If a subjectAltName extension
-       of type dNSName is present, that MUST be used as the identity."
-
      - When matching against common names, it should loop over all
        common names and choose the most specific one, i.e. the last
        one, not the first one, which the current code picks.
@@ -569,51 +568,124 @@ ssl_check_certificate (int fd, const char *host)
      - Ensure that ASN1 strings from the certificate are encoded as
        UTF-8 which can be meaningfully compared to HOST.  */
 
-  X509_NAME *xname = X509_get_subject_name(cert);
-  common_name[0] = '\0';
-  X509_NAME_get_text_by_NID (xname, NID_commonName, common_name,
-                             sizeof (common_name));
+  subjectAltNames = X509_get_ext_d2i (cert, NID_subject_alt_name, NULL, NULL);
 
-  if (!pattern_match (common_name, host))
+  if (subjectAltNames)
     {
-      logprintf (LOG_NOTQUIET, _("\
-%s: certificate common name %s doesn't match requested host name %s.\n"),
-                 severity, quote_n (0, common_name), quote_n (1, host));
-      success = false;
+      /* Test subject alternative names */
+
+      /* Do we want to check for dNSNAmes or ipAddresses (see RFC 2818)?
+       * Signal it by host_in_octet_string. */
+      ASN1_OCTET_STRING *host_in_octet_string = NULL;
+      host_in_octet_string = a2i_IPADDRESS (host);
+
+      int numaltnames = sk_GENERAL_NAME_num (subjectAltNames);
+      int i;
+      for (i=0; i < numaltnames; i++)
+        {
+          const GENERAL_NAME *name =
+            sk_GENERAL_NAME_value (subjectAltNames, i);
+          if (name)
+            {
+              if (host_in_octet_string)
+                {
+                  if (name->type == GEN_IPADD)
+                    {
+                      /* Check for ipAddress */
+                      /* TODO: Should we convert between IPv4-mapped IPv6
+                       * addresses and IPv4 addresses? */
+                      alt_name_checked = true;
+                      if (!ASN1_STRING_cmp (host_in_octet_string,
+                            name->d.iPAddress))
+                        break;
+                    }
+                }
+              else if (name->type == GEN_DNS)
+                {
+                  /* Check for dNSName */
+                  alt_name_checked = true;
+                  /* dNSName should be IA5String (i.e. ASCII), however who
+                   * does trust CA? Convert it into UTF-8 for sure. */
+                  unsigned char *name_in_utf8 = NULL;
+                  if (0 <= ASN1_STRING_to_UTF8 (&name_in_utf8, name->d.dNSName))
+                    {
+                      /* Compare and check for NULL attack in ASN1_STRING */
+                      if (pattern_match ((char *)name_in_utf8, host) &&
+                            (strlen ((char *)name_in_utf8) ==
+                                ASN1_STRING_length (name->d.dNSName)))
+                        {
+                          OPENSSL_free (name_in_utf8);
+                          break;
+                        }
+                      OPENSSL_free (name_in_utf8);
+                    }
+                }
+            }
+        }
+      sk_GENERAL_NAME_free (subjectAltNames);
+      if (host_in_octet_string)
+        ASN1_OCTET_STRING_free(host_in_octet_string);
+
+      if (alt_name_checked == true && i >= numaltnames)
+        {
+          logprintf (LOG_NOTQUIET,
+              _("%s: no certificate subject alternative name matches\n"
+                "\trequested host name %s.\n"),
+                     severity, quote_n (1, host));
+          success = false;
+        }
     }
-  else
+  
+  if (alt_name_checked == false)
     {
-      /* We now determine the length of the ASN1 string. If it differs from
-       * common_name's length, then there is a \0 before the string terminates.
-       * This can be an instance of a null-prefix attack.
-       *
-       * https://www.blackhat.com/html/bh-usa-09/bh-usa-09-archives.html#Marlinspike
-       * */
-
-      int i = -1, j;
-      X509_NAME_ENTRY *xentry;
-      ASN1_STRING *sdata;
-
-      if (xname) {
-        for (;;)
-          {
-            j = X509_NAME_get_index_by_NID (xname, NID_commonName, i);
-            if (j == -1) break;
-            i = j;
-          }
-      }
+      /* Test commomName */
+      X509_NAME *xname = X509_get_subject_name(cert);
+      common_name[0] = '\0';
+      X509_NAME_get_text_by_NID (xname, NID_commonName, common_name,
+                                 sizeof (common_name));
 
-      xentry = X509_NAME_get_entry(xname,i);
-      sdata = X509_NAME_ENTRY_get_data(xentry);
-      if (strlen (common_name) != ASN1_STRING_length (sdata))
+      if (!pattern_match (common_name, host))
         {
           logprintf (LOG_NOTQUIET, _("\
-%s: certificate common name is invalid (contains a NUL character).\n\
-This may be an indication that the host is not who it claims to be\n\
-(that is, it is not the real %s).\n"),
-                     severity, quote (host));
+    %s: certificate common name %s doesn't match requested host name %s.\n"),
+                     severity, quote_n (0, common_name), quote_n (1, host));
           success = false;
         }
+      else
+        {
+          /* We now determine the length of the ASN1 string. If it
+           * differs from common_name's length, then there is a \0
+           * before the string terminates.  This can be an instance of a
+           * null-prefix attack.
+           *
+           * https://www.blackhat.com/html/bh-usa-09/bh-usa-09-archives.html#Marlinspike
+           * */
+
+          int i = -1, j;
+          X509_NAME_ENTRY *xentry;
+          ASN1_STRING *sdata;
+
+          if (xname) {
+            for (;;)
+              {
+                j = X509_NAME_get_index_by_NID (xname, NID_commonName, i);
+                if (j == -1) break;
+                i = j;
+              }
+          }
+
+          xentry = X509_NAME_get_entry(xname,i);
+          sdata = X509_NAME_ENTRY_get_data(xentry);
+          if (strlen (common_name) != ASN1_STRING_length (sdata))
+            {
+              logprintf (LOG_NOTQUIET, _("\
+    %s: certificate common name is invalid (contains a NUL character).\n\
+    This may be an indication that the host is not who it claims to be\n\
+    (that is, it is not the real %s).\n"),
+                         severity, quote (host));
+              success = false;
+            }
+        }
     }
 
 
@@ -631,3 +703,7 @@ To connect to %s insecurely, use `--no-check-certificate'.\n"),
   /* Allow --no-check-cert to disable certificate checking. */
   return opt.check_cert ? success : true;
 }
+
+/*
+ * vim: tabstop=2 shiftwidth=2 softtabstop=2
+ */