Be less picky about keyUsage extensions

We long recommended users to use --ns-cert-type to distinguish between
client and server certificates, but that extension is long deprecated and
now can even no longer be accurately checked in OpenSSL 1.1+.  We support
a more modern alternative, --remote-cert-tls (which expands to
--remote-cert-ku + --remote-cert-eku), but are overly strict in checking
the keyUsage.  This patch makes our implementation less picky, so that
correct-but-slightly-weird certicates will not immediately be rejected.

We currently allow users to specify a list of allowed keyUsage values, and
require that the remote certificate matches one of these values exactly.
This is for more strict than keyUsage usually requires; which is that a
certificate is okay to use if it can *at least* be used for our intended
purpose.  This patch changes the behaviour to match that, by using the
library-provided mbedtls_x509_crt_check_key_usage() function in mbed TLS
builds, and performing the 'at least bits xyz' check for OpenSSL builds
(OpenSSL unfortunately does not expose a similar function).

Furthermore, this patch adds better error messages when the checking fails;
it now explains that is expects to match either of the supplied values,
and only does so if the check actually failed.

This patch also changes --remote-cert-tls to still require a specific EKU,
but only *some* keyUsage value.  Both our supported crypto libraries will
check the keyUsage value for correctness during the handshake, but only if
it is present.  So this still enforces a correct keyUsage, but is a bit
less picky about certificates that do not exactly match expectations.

This patch should be applied together with the 'deprecate --ns-cert-type'
patch I sent earlier.

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1489612820-15284-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14265.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/Changes.rst b/Changes.rst
index 0af29e3271d31d8f04cb5b667e429db37fed9804..2a94990e5e154a4b50fb24c40e5e88b8a43617ff 100644 (file)
--- a/Changes.rst
+++ b/Changes.rst
@@ -306,6 +306,13 @@ Maintainer-visible changes
 
 Version 2.4.1
 =============
+ - ``--remote-cert-ku`` now only requires the certificate to have at least the
+   bits set of one of the values in the supplied list, instead of requiring an
+   exact match to one of the values in the list.
+ - ``--remote-cert-tls`` now only requires that a keyUsage is present in the
+   certificate, and leaves the verification of the value up to the crypto
+   library, which has more information (i.e. the key exchange method in use)
+   to verify that the keyUsage is correct.
  - ``--ns-cert-type`` is deprecated.  Use ``--remote-cert-tls`` instead.
    The nsCertType x509 extension is very old, and barely used.
    ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index f6822ec713b079a8c641af3ce1a955ac7a025f50..89229fb00161f955db59a9244ecd322e9ef18355 100644 (file)
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5344,15 +5344,25 @@ or
 .B \-\-tls\-verify.
 .\"*********************************************************
 .TP
-.B \-\-remote\-cert\-ku v...
+.B \-\-remote\-cert\-ku [v...]
 Require that peer certificate was signed with an explicit
 .B key usage.
 
-This is a useful security option for clients, to ensure that
-the host they connect to is a designated server.
+If present in the certificate, the keyUsage value is validated by the TLS
+library during the TLS handshake.  Specifying this option without arguments
+requires this extension to be present (so the TLS library will verify it).
 
-The key usage should be encoded in hex, more than one key
-usage can be specified.
+If the list
+.B v...
+is also supplied, the keyUsage field must have
+.B at least
+the same bits set as the bits in
+.B one of
+the values supplied in the list
+.B v...
+
+The key usage values in the list must be encoded in hex, e.g.
+"\-\-remote\-cert\-ku a0"
 .\"*********************************************************
 .TP
 .B \-\-remote\-cert\-eku oid
@@ -5373,24 +5383,21 @@ and
 .B extended key usage
 based on RFC3280 TLS rules.
 
-This is a useful security option for clients, to ensure that
-the host they connect to is a designated server.
+This is a useful security option for clients, to ensure that the host they
+connect to is a designated server.  Or the other way around; for a server to
+verify that only hosts with a client certificate can connect.
 
 The
 .B \-\-remote\-cert\-tls client
 option is equivalent to
 .B
-\-\-remote\-cert\-ku 80 08 88 \-\-remote\-cert\-eku "TLS Web Client Authentication"
-
-The key usage is digitalSignature and/or keyAgreement.
+\-\-remote\-cert\-ku \-\-remote\-cert\-eku "TLS Web Client Authentication"
 
 The
 .B \-\-remote\-cert\-tls server
 option is equivalent to
 .B
-\-\-remote\-cert\-ku a0 88 \-\-remote\-cert\-eku "TLS Web Server Authentication"
-
-The key usage is digitalSignature and ( keyEncipherment or keyAgreement ).
+\-\-remote\-cert\-ku \-\-remote\-cert\-eku "TLS Web Server Authentication"
 
 This is an important security precaution to protect against
 a man-in-the-middle attack where an authorized client

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 58cb10e3226e39a3bbb0265a76455c09ac6417bb..dcb6ecfaaf86208bf76ca521f7cef836a5edc135 100644 (file)
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7914,14 +7914,18 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "remote-cert-ku"))
     {
-        int j;
-
         VERIFY_PERMISSION(OPT_P_GENERAL);
 
+        size_t j;
         for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
         {
             sscanf(p[j], "%x", &(options->remote_cert_ku[j-1]));
         }
+        if (j == 1)
+        {
+            /* No specific KU required, but require KU to be present */
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
+        }
     }
     else if (streq(p[0], "remote-cert-eku") && p[1] && !p[2])
     {
@@ -7934,15 +7938,12 @@ add_option(struct options *options,
 
         if (streq(p[1], "server"))
         {
-            options->remote_cert_ku[0] = 0xa0;
-            options->remote_cert_ku[1] = 0x88;
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
             options->remote_cert_eku = "TLS Web Server Authentication";
         }
         else if (streq(p[1], "client"))
         {
-            options->remote_cert_ku[0] = 0x80;
-            options->remote_cert_ku[1] = 0x08;
-            options->remote_cert_ku[2] = 0x88;
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
             options->remote_cert_eku = "TLS Web Client Authentication";
         }
         else

diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index d91799e113e02d320ce0e4a143798a76a2d2a9d5..278c5cbeac047c06940749717423b16f1f24693e 100644 (file)
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -218,6 +218,9 @@ struct x509_track
 /** Do not perform Netscape certificate type verification */
 #define NS_CERT_CHECK_CLIENT (1<<1)
 
+/** Require keyUsage to be present in cert (0xFFFF is an invalid KU value) */
+#define OPENVPN_KU_REQUIRED (0xFFFF)
+
 /*
  * TODO: document
  */

diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index 4068dd30eaa85e5bd42aa7dbe0da928e95e045d8..c32e48154e37f59e8b145ccaffef93a4dd6f7851 100644 (file)
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -437,32 +437,42 @@ result_t
 x509_verify_cert_ku(mbedtls_x509_crt *cert, const unsigned *const expected_ku,
                     int expected_len)
 {
-    result_t fFound = FAILURE;
+    msg(D_HANDSHAKE, "Validating certificate key usage");
 
     if (!(cert->ext_types & MBEDTLS_X509_EXT_KEY_USAGE))
     {
-        msg(D_HANDSHAKE, "Certificate does not have key usage extension");
+        msg(D_TLS_ERRORS,
+            "ERROR: Certificate does not have key usage extension");
+        return FAILURE;
     }
-    else
+
+    if (expected_ku[0] == OPENVPN_KU_REQUIRED)
     {
-        int i;
-        unsigned nku = cert->key_usage;
+        /* Extension required, value checked by TLS library */
+        return SUCCESS;
+    }
 
-        msg(D_HANDSHAKE, "Validating certificate key usage");
-        for (i = 0; SUCCESS != fFound && i<expected_len; i++)
+    result_t fFound = FAILURE;
+    for (size_t i = 0; SUCCESS != fFound && i<expected_len; i++)
+    {
+        if (expected_ku[i] != 0
+            && 0 == mbedtls_x509_crt_check_key_usage(cert, expected_ku[i]))
         {
-            if (expected_ku[i] != 0)
-            {
-                msg(D_HANDSHAKE, "++ Certificate has key usage  %04x, expects "
-                    "%04x", nku, expected_ku[i]);
+            fFound = SUCCESS;
+        }
+    }
 
-                if (nku == expected_ku[i])
-                {
-                    fFound = SUCCESS;
-                }
-            }
+    if (fFound != SUCCESS)
+    {
+        msg(D_TLS_ERRORS,
+            "ERROR: Certificate has key usage %04x, expected one of:",
+            cert->key_usage);
+        for (size_t i = 0; i < expected_len && expected_ku[i]; i++)
+        {
+            msg(D_TLS_ERRORS, " * %04x", expected_ku[i]);
         }
     }
+
     return fFound;
 }
 

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 5c2c5b7520800ece5e15748a38cc3daebf210d9b..5624daac59eff9d8815463aa17fa556439dee6ec 100644 (file)
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -590,55 +590,59 @@ result_t
 x509_verify_cert_ku(X509 *x509, const unsigned *const expected_ku,
                     int expected_len)
 {
-    ASN1_BIT_STRING *ku = NULL;
-    result_t fFound = FAILURE;
+    ASN1_BIT_STRING *ku = X509_get_ext_d2i(x509, NID_key_usage, NULL, NULL);
 
-    if ((ku = (ASN1_BIT_STRING *) X509_get_ext_d2i(x509, NID_key_usage, NULL,
-                                                   NULL)) == NULL)
+    if (ku == NULL)
     {
-        msg(D_HANDSHAKE, "Certificate does not have key usage extension");
+        msg(D_TLS_ERRORS, "Certificate does not have key usage extension");
+        return FAILURE;
     }
-    else
+
+    if (expected_ku[0] == OPENVPN_KU_REQUIRED)
     {
-        unsigned nku = 0;
-        int i;
-        for (i = 0; i < 8; i++)
-        {
-            if (ASN1_BIT_STRING_get_bit(ku, i))
-            {
-                nku |= 1 << (7 - i);
-            }
-        }
+        /* Extension required, value checked by TLS library */
+        return SUCCESS;
+    }
 
-        /*
-         * Fixup if no LSB bits
-         */
-        if ((nku & 0xff) == 0)
+    unsigned nku = 0;
+    for (size_t i = 0; i < 8; i++)
+    {
+        if (ASN1_BIT_STRING_get_bit(ku, i))
         {
-            nku >>= 8;
+            nku |= 1 << (7 - i);
         }
+    }
 
-        msg(D_HANDSHAKE, "Validating certificate key usage");
-        for (i = 0; fFound != SUCCESS && i < expected_len; i++)
-        {
-            if (expected_ku[i] != 0)
-            {
-                msg(D_HANDSHAKE, "++ Certificate has key usage  %04x, expects "
-                    "%04x", nku, expected_ku[i]);
+    /*
+     * Fixup if no LSB bits
+     */
+    if ((nku & 0xff) == 0)
+    {
+        nku >>= 8;
+    }
 
-                if (nku == expected_ku[i])
-                {
-                    fFound = SUCCESS;
-                }
-            }
+    msg(D_HANDSHAKE, "Validating certificate key usage");
+    result_t fFound = FAILURE;
+    for (size_t i = 0; fFound != SUCCESS && i < expected_len; i++)
+    {
+        if (expected_ku[i] != 0 && (nku & expected_ku[i]) == expected_ku[i])
+        {
+            fFound = SUCCESS;
         }
     }
 
-    if (ku != NULL)
+    if (fFound != SUCCESS)
     {
-        ASN1_BIT_STRING_free(ku);
+        msg(D_TLS_ERRORS,
+            "ERROR: Certificate has key usage %04x, expected one of:", nku);
+        for (size_t i = 0; i < expected_len && expected_ku[i]; i++)
+        {
+            msg(D_TLS_ERRORS, " * %04x", expected_ku[i]);
+        }
     }
 
+    ASN1_BIT_STRING_free(ku);
+
     return fFound;
 }