diff mbox series

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

Message ID NaAsPRnb2g-raEjxjv15C_odADru2NIyXCs5wvZO609b_-4IqW2Do2sVbBull-FE9myUOZ7lNV6gdAx7MGugGUOBbdYOFO8AsoFHu35BIs8=@protonmail.com
State Changes Requested
Headers
Series [Openvpn-devel] openssl: alternative names support for --verify-x509-name CN checks |

Commit Message

haixiao.yan.cn--- via Openvpn-devel Feb. 10, 2020, 6:59 a.m. UTC 
  when using "--verify-x509-name [hostname] 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).

see also: https://github.com/OpenVPN/openvpn/pull/136/

Signed-off-by: Mateusz Markowicz <poniekad@protonmail.com>
---
src/openvpn/options.c            |  4 +++
src/openvpn/ssl_verify.c         | 18 +++++++++++---
src/openvpn/ssl_verify.h         |  1 +
src/openvpn/ssl_verify_backend.h |  7 ++++++
src/openvpn/ssl_verify_mbedtls.c | 11 +++++++++
src/openvpn/ssl_verify_openssl.c | 42 ++++++++++++++++++++++++++++++++
6 files changed, 80 insertions(+), 3 deletions(-)

--
2.20.1

Comments

Arne Schwabe Feb. 12, 2020, 3:39 a.m. UTC | #1 
Am 10.02.20 um 18:59 schrieb Mateusz Markowicz via Openvpn-devel:
> when using "--verify-x509-name [hostname] 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).
> 
> see also: https://github.com/OpenVPN/openvpn/pull/136/
> 
> Signed-off-by: Mateusz Markowicz <poniekad@protonmail.com
> <mailto:poniekad@protonmail.com>>


If this should have a chance of being included it needs to cover mbed
TLS as well.


Also documentation in the man page is missing.

> ---
> src/openvpn/options.c            |  4 +++
> src/openvpn/ssl_verify.c         | 18 +++++++++++---
> src/openvpn/ssl_verify.h         |  1 +
> src/openvpn/ssl_verify_backend.h |  7 ++++++
> src/openvpn/ssl_verify_mbedtls.c | 11 +++++++++
> src/openvpn/ssl_verify_openssl.c | 42 ++++++++++++++++++++++++++++++++
> 6 files changed, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 173a1eea..438dfff0 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -8144,6 +8144,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 65188d23..6480b5eb 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -390,15 +390,27 @@ 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
> +        bool match;
> +        if (opt->verify_x509_type == VERIFY_X509_SAN)
> +        {
> +            bool have_alt_names;
> +            match = x509v3_is_host_in_alternative_names(peer_cert,
> opt->verify_x509_name, &have_alt_names)
> +                    || (!have_alt_names &&
> strcmp(opt->verify_x509_name, common_name) == 0);

I know that this technically correct C but setting a variable in the
first part of via & and then using it in the second part feels like not
good style. I would rather like too a bit more verbose and less clever
code that is easier to understand.


> +        }
> +        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) )
> +                            strlen(opt->verify_x509_name)) == 0);
> +        }
> +
> +        if (match)
>          {
> -            msg(D_HANDSHAKE, "VERIFY X509NAME OK: %s", subject);
> +            msg(D_HANDSHAKE, "VERIFY X509NAME OK: %s",
> opt->verify_x509_name);

This changes the log message. If you want to verify that the cert prefix
with OVPN-client or something you don't get to see what certificate you
accepted anymore. If you want to log opt->verify_x509_name that needs to
be in addition.

>          }
>          else
>          {
> diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
> index c54b89a6..1295e76b 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
> 
> #define TLS_AUTHENTICATION_SUCCEEDED  0
> #define TLS_AUTHENTICATION_FAILED     1
> diff --git a/src/openvpn/ssl_verify_backend.h
> b/src/openvpn/ssl_verify_backend.h
> index d6b31bfa..927a5a29 100644
> --- a/src/openvpn/ssl_verify_backend.h
> +++ b/src/openvpn/ssl_verify_backend.h
> @@ -268,4 +268,11 @@ 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} != NULL it'll return 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_names);
> +
> #endif /* SSL_VERIFY_BACKEND_H_ */
> diff --git a/src/openvpn/ssl_verify_mbedtls.c
> b/src/openvpn/ssl_verify_mbedtls.c
> index fd31bbbd..2f2e04be 100644
> --- a/src/openvpn/ssl_verify_mbedtls.c
> +++ b/src/openvpn/ssl_verify_mbedtls.c
> @@ -245,6 +245,17 @@ 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 != NULL)
> +    {
> +        *has_alt_names = false;
> +    }
> +    return false;
> +}


*has_alt_names = (has_alt_names != NULL);
David Sommerseth Feb. 12, 2020, 4:01 a.m. UTC | #2 
On 12/02/2020 15:39, Arne Schwabe wrote:
>> +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.");

I'm not happy about this at all.  This should be possible to achieve with
mbed TLS as well:
<https://tls.mbed.org/api/structmbedtls__x509__crt.html#a1f148e8fb52e03e2604e716386a07df4>

One starting point for this can probably found here:
<https://tls.mbed.org/api/group__x509__module.html#ga033567483649030f7f859db4f4cb7e14>
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 173a1eea..438dfff0 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8144,6 +8144,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 65188d23..6480b5eb 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -390,15 +390,27 @@  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
+        bool match;
+        if (opt->verify_x509_type == VERIFY_X509_SAN)
+        {
+            bool have_alt_names;
+            match = x509v3_is_host_in_alternative_names(peer_cert, opt->verify_x509_name, &have_alt_names)
+                    || (!have_alt_names && 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) )
+                            strlen(opt->verify_x509_name)) == 0);
+        }
+
+        if (match)
         {
-            msg(D_HANDSHAKE, "VERIFY X509NAME OK: %s", subject);
+            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 c54b89a6..1295e76b 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

#define TLS_AUTHENTICATION_SUCCEEDED  0
#define TLS_AUTHENTICATION_FAILED     1
diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index d6b31bfa..927a5a29 100644
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -268,4 +268,11 @@  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} != NULL it'll return 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_names);
+
#endif /* SSL_VERIFY_BACKEND_H_ */
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index fd31bbbd..2f2e04be 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -245,6 +245,17 @@  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 != NULL)
+    {
+        *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 ff14db23..bb639abc 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -364,6 +364,48 @@  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 != NULL)
+    {
+        *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,