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.

---
 Changes.rst                      |  7 ++++
 doc/openvpn.8                    | 33 ++++++++++--------
 src/openvpn/options.c            | 15 +++++----
 src/openvpn/ssl_verify.h         |  3 ++
 src/openvpn/ssl_verify_mbedtls.c | 42 ++++++++++++++---------
 src/openvpn/ssl_verify_openssl.c | 72 +++++++++++++++++++++-------------------
 6 files changed, 102 insertions(+), 70 deletions(-)

index 0af29e3..2a94990 100644
@@ -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

index f6822ec..89229fb 100644
@@ -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

index 1243db0..d0d39f6 100644
@@ -7950,14 +7950,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])
     {

@@ -7970,15 +7974,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

index ffab218..22a6190 100644
@@ -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
  */

2.7.4

ACK. While I'm certainly not the authority when it comes to crypto library innards, after the discussion we had on IRC, I'm fine with the changes - and testing against our own corp CA (which did not work with --remote-cert-tls server before, because it had "too many bits" set) it works now - that is, "remote-cert-tls server" works, ".. client" fails (set on the client).

The code change match the description, and the behaviour observed for "the SSL library will check" also matches (I have not tried creating a cert without ku/eku to see what happens then, but trust Steffan to have done so).

The default error messages *could* be a bit more helpful, though...

Sun Mar 19 17:07:17 2017 TLS_ERROR: read tls_read_plaintext error: X509 - Certificate verification failed, e.g. CRL, CA or signature check failed

.. but increasing debug to --verb 2 shows all the details:

Sun Mar 19 17:07:45 2017 Validating certificate key usage
Sun Mar 19 17:07:45 2017 VERIFY KU OK
Sun Mar 19 17:07:45 2017 Validating certificate extended key usage
Sun Mar 19 17:07:45 2017 ++ Certificate has EKU (str) TLS Web Server Authentication, expects TLS Web Client Authentication Sun Mar 19 17:07:45 2017 ++ Certificate has EKU (oid) 1.3.6.1.5.5.7.3.1, expects TLS Web Client Authentication Sun Mar 19 17:07:45 2017 VERIFY EKU ERROR

(intentional error)

Tested both for OpenSSL and mbedTLS builds.

Patch has been applied to the master and release/2.4 branch.

commit 92a5b9fb76cbb7f43a6aa86994ff559f06c55c7a (master) commit 60b23236329e6921729f51e7689042a29c794a6b (release/2.4)

Author: Steffan Karger
Date:   Wed Mar 15 22:20:20 2017 +0100

     Be less picky about keyUsage extensions

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14265.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering