Browse code

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>
(cherry picked from commit 92a5b9fb76cbb7f43a6aa86994ff559f06c55c7a)

Steffan Karger authored on 2017/03/16 06:20:20
Showing 6 changed files
... ...
@@ -306,6 +306,13 @@ Maintainer-visible changes
306 306
 
307 307
 Version 2.4.1
308 308
 =============
309
+ - ``--remote-cert-ku`` now only requires the certificate to have at least the
310
+   bits set of one of the values in the supplied list, instead of requiring an
311
+   exact match to one of the values in the list.
312
+ - ``--remote-cert-tls`` now only requires that a keyUsage is present in the
313
+   certificate, and leaves the verification of the value up to the crypto
314
+   library, which has more information (i.e. the key exchange method in use)
315
+   to verify that the keyUsage is correct.
309 316
  - ``--ns-cert-type`` is deprecated.  Use ``--remote-cert-tls`` instead.
310 317
    The nsCertType x509 extension is very old, and barely used.
311 318
    ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage
... ...
@@ -5344,15 +5344,25 @@ or
5344 5344
 .B \-\-tls\-verify.
5345 5345
 .\"*********************************************************
5346 5346
 .TP
5347
-.B \-\-remote\-cert\-ku v...
5347
+.B \-\-remote\-cert\-ku [v...]
5348 5348
 Require that peer certificate was signed with an explicit
5349 5349
 .B key usage.
5350 5350
 
5351
-This is a useful security option for clients, to ensure that
5352
-the host they connect to is a designated server.
5351
+If present in the certificate, the keyUsage value is validated by the TLS
5352
+library during the TLS handshake.  Specifying this option without arguments
5353
+requires this extension to be present (so the TLS library will verify it).
5353 5354
 
5354
-The key usage should be encoded in hex, more than one key
5355
-usage can be specified.
5355
+If the list
5356
+.B v...
5357
+is also supplied, the keyUsage field must have
5358
+.B at least
5359
+the same bits set as the bits in
5360
+.B one of
5361
+the values supplied in the list
5362
+.B v...
5363
+
5364
+The key usage values in the list must be encoded in hex, e.g.
5365
+"\-\-remote\-cert\-ku a0"
5356 5366
 .\"*********************************************************
5357 5367
 .TP
5358 5368
 .B \-\-remote\-cert\-eku oid
... ...
@@ -5373,24 +5383,21 @@ and
5373 5373
 .B extended key usage
5374 5374
 based on RFC3280 TLS rules.
5375 5375
 
5376
-This is a useful security option for clients, to ensure that
5377
-the host they connect to is a designated server.
5376
+This is a useful security option for clients, to ensure that the host they
5377
+connect to is a designated server.  Or the other way around; for a server to
5378
+verify that only hosts with a client certificate can connect.
5378 5379
 
5379 5380
 The
5380 5381
 .B \-\-remote\-cert\-tls client
5381 5382
 option is equivalent to
5382 5383
 .B
5383
-\-\-remote\-cert\-ku 80 08 88 \-\-remote\-cert\-eku "TLS Web Client Authentication"
5384
-
5385
-The key usage is digitalSignature and/or keyAgreement.
5384
+\-\-remote\-cert\-ku \-\-remote\-cert\-eku "TLS Web Client Authentication"
5386 5385
 
5387 5386
 The
5388 5387
 .B \-\-remote\-cert\-tls server
5389 5388
 option is equivalent to
5390 5389
 .B
5391
-\-\-remote\-cert\-ku a0 88 \-\-remote\-cert\-eku "TLS Web Server Authentication"
5392
-
5393
-The key usage is digitalSignature and ( keyEncipherment or keyAgreement ).
5390
+\-\-remote\-cert\-ku \-\-remote\-cert\-eku "TLS Web Server Authentication"
5394 5391
 
5395 5392
 This is an important security precaution to protect against
5396 5393
 a man-in-the-middle attack where an authorized client
... ...
@@ -7930,14 +7930,18 @@ add_option(struct options *options,
7930 7930
     }
7931 7931
     else if (streq(p[0], "remote-cert-ku"))
7932 7932
     {
7933
-        int j;
7934
-
7935 7933
         VERIFY_PERMISSION(OPT_P_GENERAL);
7936 7934
 
7935
+        size_t j;
7937 7936
         for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
7938 7937
         {
7939 7938
             sscanf(p[j], "%x", &(options->remote_cert_ku[j-1]));
7940 7939
         }
7940
+        if (j == 1)
7941
+        {
7942
+            /* No specific KU required, but require KU to be present */
7943
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
7944
+        }
7941 7945
     }
7942 7946
     else if (streq(p[0], "remote-cert-eku") && p[1] && !p[2])
7943 7947
     {
... ...
@@ -7950,15 +7954,12 @@ add_option(struct options *options,
7950 7950
 
7951 7951
         if (streq(p[1], "server"))
7952 7952
         {
7953
-            options->remote_cert_ku[0] = 0xa0;
7954
-            options->remote_cert_ku[1] = 0x88;
7953
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
7955 7954
             options->remote_cert_eku = "TLS Web Server Authentication";
7956 7955
         }
7957 7956
         else if (streq(p[1], "client"))
7958 7957
         {
7959
-            options->remote_cert_ku[0] = 0x80;
7960
-            options->remote_cert_ku[1] = 0x08;
7961
-            options->remote_cert_ku[2] = 0x88;
7958
+            options->remote_cert_ku[0] = OPENVPN_KU_REQUIRED;
7962 7959
             options->remote_cert_eku = "TLS Web Client Authentication";
7963 7960
         }
7964 7961
         else
... ...
@@ -218,6 +218,9 @@ struct x509_track
218 218
 /** Do not perform Netscape certificate type verification */
219 219
 #define NS_CERT_CHECK_CLIENT (1<<1)
220 220
 
221
+/** Require keyUsage to be present in cert (0xFFFF is an invalid KU value) */
222
+#define OPENVPN_KU_REQUIRED (0xFFFF)
223
+
221 224
 /*
222 225
  * TODO: document
223 226
  */
... ...
@@ -437,32 +437,42 @@ result_t
437 437
 x509_verify_cert_ku(mbedtls_x509_crt *cert, const unsigned *const expected_ku,
438 438
                     int expected_len)
439 439
 {
440
-    result_t fFound = FAILURE;
440
+    msg(D_HANDSHAKE, "Validating certificate key usage");
441 441
 
442 442
     if (!(cert->ext_types & MBEDTLS_X509_EXT_KEY_USAGE))
443 443
     {
444
-        msg(D_HANDSHAKE, "Certificate does not have key usage extension");
444
+        msg(D_TLS_ERRORS,
445
+            "ERROR: Certificate does not have key usage extension");
446
+        return FAILURE;
445 447
     }
446
-    else
448
+
449
+    if (expected_ku[0] == OPENVPN_KU_REQUIRED)
447 450
     {
448
-        int i;
449
-        unsigned nku = cert->key_usage;
451
+        /* Extension required, value checked by TLS library */
452
+        return SUCCESS;
453
+    }
450 454
 
451
-        msg(D_HANDSHAKE, "Validating certificate key usage");
452
-        for (i = 0; SUCCESS != fFound && i<expected_len; i++)
455
+    result_t fFound = FAILURE;
456
+    for (size_t i = 0; SUCCESS != fFound && i<expected_len; i++)
457
+    {
458
+        if (expected_ku[i] != 0
459
+            && 0 == mbedtls_x509_crt_check_key_usage(cert, expected_ku[i]))
453 460
         {
454
-            if (expected_ku[i] != 0)
455
-            {
456
-                msg(D_HANDSHAKE, "++ Certificate has key usage  %04x, expects "
457
-                    "%04x", nku, expected_ku[i]);
461
+            fFound = SUCCESS;
462
+        }
463
+    }
458 464
 
459
-                if (nku == expected_ku[i])
460
-                {
461
-                    fFound = SUCCESS;
462
-                }
463
-            }
465
+    if (fFound != SUCCESS)
466
+    {
467
+        msg(D_TLS_ERRORS,
468
+            "ERROR: Certificate has key usage %04x, expected one of:",
469
+            cert->key_usage);
470
+        for (size_t i = 0; i < expected_len && expected_ku[i]; i++)
471
+        {
472
+            msg(D_TLS_ERRORS, " * %04x", expected_ku[i]);
464 473
         }
465 474
     }
475
+
466 476
     return fFound;
467 477
 }
468 478
 
... ...
@@ -590,55 +590,59 @@ result_t
590 590
 x509_verify_cert_ku(X509 *x509, const unsigned *const expected_ku,
591 591
                     int expected_len)
592 592
 {
593
-    ASN1_BIT_STRING *ku = NULL;
594
-    result_t fFound = FAILURE;
593
+    ASN1_BIT_STRING *ku = X509_get_ext_d2i(x509, NID_key_usage, NULL, NULL);
595 594
 
596
-    if ((ku = (ASN1_BIT_STRING *) X509_get_ext_d2i(x509, NID_key_usage, NULL,
597
-                                                   NULL)) == NULL)
595
+    if (ku == NULL)
598 596
     {
599
-        msg(D_HANDSHAKE, "Certificate does not have key usage extension");
597
+        msg(D_TLS_ERRORS, "Certificate does not have key usage extension");
598
+        return FAILURE;
600 599
     }
601
-    else
600
+
601
+    if (expected_ku[0] == OPENVPN_KU_REQUIRED)
602 602
     {
603
-        unsigned nku = 0;
604
-        int i;
605
-        for (i = 0; i < 8; i++)
606
-        {
607
-            if (ASN1_BIT_STRING_get_bit(ku, i))
608
-            {
609
-                nku |= 1 << (7 - i);
610
-            }
611
-        }
603
+        /* Extension required, value checked by TLS library */
604
+        return SUCCESS;
605
+    }
612 606
 
613
-        /*
614
-         * Fixup if no LSB bits
615
-         */
616
-        if ((nku & 0xff) == 0)
607
+    unsigned nku = 0;
608
+    for (size_t i = 0; i < 8; i++)
609
+    {
610
+        if (ASN1_BIT_STRING_get_bit(ku, i))
617 611
         {
618
-            nku >>= 8;
612
+            nku |= 1 << (7 - i);
619 613
         }
614
+    }
620 615
 
621
-        msg(D_HANDSHAKE, "Validating certificate key usage");
622
-        for (i = 0; fFound != SUCCESS && i < expected_len; i++)
623
-        {
624
-            if (expected_ku[i] != 0)
625
-            {
626
-                msg(D_HANDSHAKE, "++ Certificate has key usage  %04x, expects "
627
-                    "%04x", nku, expected_ku[i]);
616
+    /*
617
+     * Fixup if no LSB bits
618
+     */
619
+    if ((nku & 0xff) == 0)
620
+    {
621
+        nku >>= 8;
622
+    }
628 623
 
629
-                if (nku == expected_ku[i])
630
-                {
631
-                    fFound = SUCCESS;
632
-                }
633
-            }
624
+    msg(D_HANDSHAKE, "Validating certificate key usage");
625
+    result_t fFound = FAILURE;
626
+    for (size_t i = 0; fFound != SUCCESS && i < expected_len; i++)
627
+    {
628
+        if (expected_ku[i] != 0 && (nku & expected_ku[i]) == expected_ku[i])
629
+        {
630
+            fFound = SUCCESS;
634 631
         }
635 632
     }
636 633
 
637
-    if (ku != NULL)
634
+    if (fFound != SUCCESS)
638 635
     {
639
-        ASN1_BIT_STRING_free(ku);
636
+        msg(D_TLS_ERRORS,
637
+            "ERROR: Certificate has key usage %04x, expected one of:", nku);
638
+        for (size_t i = 0; i < expected_len && expected_ku[i]; i++)
639
+        {
640
+            msg(D_TLS_ERRORS, " * %04x", expected_ku[i]);
641
+        }
640 642
     }
641 643
 
644
+    ASN1_BIT_STRING_free(ku);
645
+
642 646
     return fFound;
643 647
 }
644 648