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

[Antonio Quartulli]

Hi,

This patch was msising some hunks. To be resent as v2.

Cheers,

On 18/09/2022 01:32, Antonio Quartulli wrote:

From: Mateusz Markowicz 

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 
---
  doc/man-sections/tls-options.rst |  9 ---
  src/openvpn/options.c|  4 +++
  src/openvpn/ssl_verify.c | 46 ++--
  src/openvpn/ssl_verify.h |  1 +
  src/openvpn/ssl_verify_backend.h |  8 --
  src/openvpn/ssl_verify_mbedtls.c |  7 -
  src/openvpn/ssl_verify_openssl.c | 16 +++
  7 files changed, 59 insertions(+), 32 deletions(-)

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 76c09a0a..bde7ec98 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8894,6 +8894,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 45545c83..786d23ba 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -377,31 +377,37 @@ 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,
+_alt_names);
+if (!match && !have_alt_names)
+{
+match = (strcmp(opt->verify_x509_name, common_name) == 0);
+}
  }
  else
  {
-bool verfified_with_alt_names = opt->verify_x509_type == 
VERIFY_X509_SUBJECT_RDN &&
-x509v3_is_host_in_alternative_names(peer_cert, 
opt->verify_x509_name);
+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 (verfified_with_alt_names)

-{
-msg(D_HANDSHAKE, "VERIFY X509NAME OK (ALTERNATIVE): %s", 
opt->verify_x509_name);
-}
-else
-{
-msg(D_HANDSHAKE, "VERIFY X509NAME ERROR: %s, must be %s",
-subject, opt->verify_x509_name);
-

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

[David Sommerseth]

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:


One starting point for this can probably found here:



-- 
kind regards,

David Sommerseth
OpenVPN Inc



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[Arne Schwabe]

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  >


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, _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
>