diff mbox series

[Openvpn-devel,v2] openssl: alternative names support for --verify-x509-name CN checks

Message ID 20220918220740.32549-1-a@unstable.cc
State New
Headers show
Series [Openvpn-devel,v2] openssl: alternative names support for --verify-x509-name CN checks | expand

Commit Message

Antonio Quartulli Sept. 18, 2022, 12:07 p.m. UTC 
From: Mateusz Markowicz <poniekad@protonmail.com>

When using "--verify-x509-name [hostname] subject-alt-name" hostname
will now be accepted also when matched against one of the
X509v3 Subject Alternative Name IP or DNS entries (instead of just
Subject's CN).

While at it, fix a few uncrustify complaints to allow committing this
change.

Signed-off-by: Mateusz Markowicz <poniekad@protonmail.com>
---

Changes from v1:
* added missing hunks
---
 doc/man-sections/tls-options.rst |  9 ++++---
 src/openvpn/options.c            |  4 +++
 src/openvpn/ssl_verify.c         | 32 +++++++++++++++++------
 src/openvpn/ssl_verify.h         |  1 +
 src/openvpn/ssl_verify_backend.h |  9 +++++++
 src/openvpn/ssl_verify_mbedtls.c | 12 +++++++++
 src/openvpn/ssl_verify_openssl.c | 45 ++++++++++++++++++++++++++++++++
 7 files changed, 100 insertions(+), 12 deletions(-)

Comments

Frank Lichtenheld Sept. 19, 2022, 1:41 a.m. UTC | #1 
On Mon, Sep 19, 2022 at 12:07:40AM +0200, Antonio Quartulli wrote:
> From: Mateusz Markowicz <poniekad@protonmail.com>
> 
> When using "--verify-x509-name [hostname] subject-alt-name" hostname
> will now be accepted also when matched against one of the
> X509v3 Subject Alternative Name IP or DNS entries (instead of just
> Subject's CN).

The code looks like it would do what it says, but the options are then
very confusing, I think.

So if you want to check the name against a SAN of type EMAIL you
need to specify "x509-username-field ext:subjectAltName" but that
will only match against the last SAN. But if you want to check
against a SAN of type DNS or IP you need to specify
"verify-x509-name [hostname] subject-alt-name", which does not
allow the combination with other fields like x509-username-field
but matches against all SANs. Huh?

The design choices that went into the existing x509-username-field
seem questionable in hindsight and don't give a good way of
integrating this functionality. But having two completely separate
implementations of "extract a name from SAN" seems wasteful as well.

Don't have any good solutions to offer atm. Just noting the mess.

Regards,
diff mbox series

Patch

diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
index d51aff77..257c779a 100644
--- a/doc/man-sections/tls-options.rst
+++ b/doc/man-sections/tls-options.rst
@@ -647,10 +647,11 @@  If the option is inlined, ``algo`` is always :code:`SHA256`.
 
   Which X.509 name is compared to ``name`` depends on the setting of type.
   ``type`` can be :code:`subject` to match the complete subject DN
-  (default), :code:`name` to match a subject RDN or :code:`name-prefix` to
-  match a subject RDN prefix. Which RDN is verified as name depends on the
-  ``--x509-username-field`` option. But it defaults to the common name
-  (CN), e.g. a certificate with a subject DN
+  (default), :code:`name` to match a subject RDN, :code:`name-prefix` to
+  match a subject RDN prefix or :code:`subject-alt-name` to match the subject
+  SAN (or the CN if SAN is not specified). Which RDN is verified as name
+  depends on the ``--x509-username-field`` option. But it defaults to the
+  common name (CN), e.g. a certificate with a subject DN
   ::
 
      C=KG, ST=NA, L=Bishkek, CN=Server-1
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4566172b..02f91259 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8935,6 +8935,10 @@  add_option(struct options *options,
             {
                 type = VERIFY_X509_SUBJECT_RDN_PREFIX;
             }
+            else if (streq(p[2], "subject-alt-name"))
+            {
+                type = VERIFY_X509_SAN;
+            }
             else
             {
                 msg(msglevel, "unknown X.509 name type: %s", p[2]);
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 76cb9f19..c13f1c45 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -377,15 +377,31 @@  verify_peer_cert(const struct tls_options *opt, openvpn_x509_cert_t *peer_cert,
     /* verify X509 name or username against --verify-x509-[user]name */
     if (opt->verify_x509_type != VERIFY_X509_NONE)
     {
-        if ( (opt->verify_x509_type == VERIFY_X509_SUBJECT_DN
-              && strcmp(opt->verify_x509_name, subject) == 0)
-             || (opt->verify_x509_type == VERIFY_X509_SUBJECT_RDN
-                 && strcmp(opt->verify_x509_name, common_name) == 0)
-             || (opt->verify_x509_type == VERIFY_X509_SUBJECT_RDN_PREFIX
-                 && strncmp(opt->verify_x509_name, common_name,
-                            strlen(opt->verify_x509_name)) == 0) )
+        bool match;
+        if (opt->verify_x509_type == VERIFY_X509_SAN)
         {
-            msg(D_HANDSHAKE, "VERIFY X509NAME OK: %s", subject);
+            bool have_alt_names;
+            match = x509v3_is_host_in_alternative_names(peer_cert, opt->verify_x509_name,
+                                                        &have_alt_names);
+            if (!match && !have_alt_names)
+            {
+                match = (strcmp(opt->verify_x509_name, common_name) == 0);
+            }
+        }
+        else
+        {
+            match = (opt->verify_x509_type == VERIFY_X509_SUBJECT_DN
+                     && strcmp(opt->verify_x509_name, subject) == 0)
+                    || (opt->verify_x509_type == VERIFY_X509_SUBJECT_RDN
+                        && strcmp(opt->verify_x509_name, common_name) == 0)
+                    || (opt->verify_x509_type == VERIFY_X509_SUBJECT_RDN_PREFIX
+                        && strncmp(opt->verify_x509_name, common_name,
+                                   strlen(opt->verify_x509_name)) == 0);
+        }
+
+        if (match)
+        {
+            msg(D_HANDSHAKE, "VERIFY X509NAME OK: %s", opt->verify_x509_name);
         }
         else
         {
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index 30dfc9bc..214243d8 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -64,6 +64,7 @@  struct cert_hash_set {
 #define VERIFY_X509_SUBJECT_DN          1
 #define VERIFY_X509_SUBJECT_RDN         2
 #define VERIFY_X509_SUBJECT_RDN_PREFIX  3
+#define VERIFY_X509_SAN                 4
 
 enum tls_auth_status
 {
diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index be857960..948daae2 100644
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -268,4 +268,13 @@  result_t x509_write_pem(FILE *peercert_file, openvpn_x509_cert_t *peercert);
  */
 bool tls_verify_crl_missing(const struct tls_options *opt);
 
+/**
+ * Return true iff {host} was found in {cert} Subject Alternative Names DNS or
+ * IP entries.
+ * If has_alt_names is not NULL it'll be set to true iff Subject Alternative
+ * Names were defined for cert.
+ */
+bool x509v3_is_host_in_alternative_names(openvpn_x509_cert_t *cert,
+                                         const char *host, bool *has_alt_name);
+
 #endif /* SSL_VERIFY_BACKEND_H_ */
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index 5463c8da..228e7fb6 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -262,6 +262,18 @@  x509_get_subject(mbedtls_x509_crt *cert, struct gc_arena *gc)
     return subject;
 }
 
+bool
+x509v3_is_host_in_alternative_names(mbedtls_x509_crt *cert, const char *host,
+                                    bool *has_alt_names)
+{
+    msg(M_WARN, "Missing support for subject alternative names in mbedtls.");
+    if (has_alt_names)
+    {
+        *has_alt_names = false;
+    }
+    return false;
+}
+
 static void
 do_setenv_x509(struct env_set *es, const char *name, char *value, int depth)
 {
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 78efa70a..3c6f629b 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -376,6 +376,51 @@  err:
     return subject;
 }
 
+bool
+x509v3_is_host_in_alternative_names(X509 *cert, const char *host,
+                                    bool *has_alt_names)
+{
+    GENERAL_NAMES *altnames = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
+    if (has_alt_names)
+    {
+        *has_alt_names = (altnames != NULL);
+    }
+    if (altnames == NULL)
+    {
+        return false;
+    }
+
+    int n = sk_GENERAL_NAME_num(altnames);
+    for (int i = 0; i < n; i++)
+    {
+        GENERAL_NAME *altname = sk_GENERAL_NAME_value(altnames, i);
+        ASN1_STRING *altname_asn1 = NULL;
+        if (altname->type == GEN_DNS)
+        {
+            altname_asn1 = altname->d.dNSName;
+        }
+        else if (altname->type == GEN_IPADD)
+        {
+            altname_asn1 = altname->d.iPAddress;
+        }
+
+        if (altname_asn1 != NULL)
+        {
+            char *altname_cstr = NULL;
+            if (ASN1_STRING_to_UTF8((unsigned char **)&altname_cstr, altname_asn1) >= 0)
+            {
+                bool match = strcmp(host, altname_cstr) == 0;
+                OPENSSL_free(altname_cstr);
+                if (match)
+                {
+                    return true;
+                }
+            }
+        }
+    }
+    return false;
+}
+
 
 /*
  * x509-track implementation -- save X509 fields to environment,