Browse code

OpenSSL: don't use direct access to the internal of X509

OpenSSL 1.1 does not allow us to directly access the internal of
any data type, including X509. We have to use the defined
functions to do so.

In x509_verify_ns_cert_type() in particular, this means that we
cannot directly check for the extended flags to find whether the
certificate should be used as a client or as a server certificate.
We need to leverage the X509_check_purpose() API yet this API is
far stricter than the currently implemented check. So far, I have
not been able to find a situation where this stricter test fails
(although I must admit that I haven't tested that very well).

We double-check the certificate purpose using "direct access" to the
internal of the certificate object (of course, this is not a real
direct access, but we still fetch ASN1 strings within the X509 object
and we check the internal value of these strings). This allow us to
warn the user if there is a discrepancy between the X509_check_purpose()
return value and our internal, less strict check.

We use these changes to make peer_cert a non-const parameter to
x509_verify_ns_cert_type(). The underlying library waits for a
non-const pointer, and forcing it to be a const pointer does not make
much sense (please note that this has an effect on the mbedtls part
too).

Compatibility with OpenSSL 1.0 is kept by defining the corresponding
functions when they are not found in the library.

Signed-off-by: Emmanuel Deloget <logout@free.fr>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <20170612134330.20971-2-logout@free.fr>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14792.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Emmanuel Deloget authored on 2017/06/12 22:43:23
Showing 6 changed files
... ...
@@ -921,6 +921,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
921 921
 		[ \
922 922
 			SSL_CTX_get_default_passwd_cb \
923 923
 			SSL_CTX_get_default_passwd_cb_userdata \
924
+			X509_get0_pubkey \
924 925
 			X509_STORE_get0_objects \
925 926
 			X509_OBJECT_free \
926 927
 			X509_OBJECT_get_type \
... ...
@@ -73,6 +73,21 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
73 73
 }
74 74
 #endif
75 75
 
76
+#if !defined(HAVE_X509_GET0_PUBKEY)
77
+/**
78
+ * Get the public key from a X509 certificate
79
+ *
80
+ * @param x                  X509 certificate
81
+ * @return                   The certificate public key
82
+ */
83
+static inline EVP_PKEY *
84
+X509_get0_pubkey(const X509 *x)
85
+{
86
+    return (x && x->cert_info && x->cert_info->key) ?
87
+           x->cert_info->key->pkey : NULL;
88
+}
89
+#endif
90
+
76 91
 #if !defined(HAVE_X509_STORE_GET0_OBJECTS)
77 92
 /**
78 93
  * Fetch the X509 object stack from the X509 store
... ...
@@ -1070,7 +1070,8 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
1070 1070
     }
1071 1071
 
1072 1072
     /* get the public key */
1073
-    ASSERT(cert->cert_info->key->pkey); /* NULL before SSL_CTX_use_certificate() is called */
1073
+    EVP_PKEY *pkey = X509_get0_pubkey(cert);
1074
+    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
1074 1075
     pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
1075 1076
 
1076 1077
     /* initialize RSA object */
... ...
@@ -210,7 +210,7 @@ void x509_setenv_track(const struct x509_track *xt, struct env_set *es,
210 210
  *                      the expected bit set. \c FAILURE if the certificate does
211 211
  *                      not have NS cert type verification or the wrong bit set.
212 212
  */
213
-result_t x509_verify_ns_cert_type(const openvpn_x509_cert_t *cert, const int usage);
213
+result_t x509_verify_ns_cert_type(openvpn_x509_cert_t *cert, const int usage);
214 214
 
215 215
 /*
216 216
  * Verify X.509 key usage extension field.
... ...
@@ -410,7 +410,7 @@ x509_setenv(struct env_set *es, int cert_depth, mbedtls_x509_crt *cert)
410 410
 }
411 411
 
412 412
 result_t
413
-x509_verify_ns_cert_type(const mbedtls_x509_crt *cert, const int usage)
413
+x509_verify_ns_cert_type(mbedtls_x509_crt *cert, const int usage)
414 414
 {
415 415
     if (usage == NS_CERT_CHECK_NONE)
416 416
     {
... ...
@@ -293,18 +293,20 @@ backend_x509_get_serial_hex(openvpn_x509_cert_t *cert, struct gc_arena *gc)
293 293
 struct buffer
294 294
 x509_get_sha1_fingerprint(X509 *cert, struct gc_arena *gc)
295 295
 {
296
-    struct buffer hash = alloc_buf_gc(sizeof(cert->sha1_hash), gc);
297
-    memcpy(BPTR(&hash), cert->sha1_hash, sizeof(cert->sha1_hash));
298
-    ASSERT(buf_inc_len(&hash, sizeof(cert->sha1_hash)));
296
+    const EVP_MD *sha1 = EVP_sha1();
297
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha1), gc);
298
+    X509_digest(cert, EVP_sha1(), BPTR(&hash), NULL);
299
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha1)));
299 300
     return hash;
300 301
 }
301 302
 
302 303
 struct buffer
303 304
 x509_get_sha256_fingerprint(X509 *cert, struct gc_arena *gc)
304 305
 {
305
-    struct buffer hash = alloc_buf_gc((EVP_sha256())->md_size, gc);
306
+    const EVP_MD *sha256 = EVP_sha256();
307
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha256), gc);
306 308
     X509_digest(cert, EVP_sha256(), BPTR(&hash), NULL);
307
-    ASSERT(buf_inc_len(&hash, (EVP_sha256())->md_size));
309
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha256)));
308 310
     return hash;
309 311
 }
310 312
 
... ...
@@ -569,7 +571,7 @@ x509_setenv(struct env_set *es, int cert_depth, openvpn_x509_cert_t *peer_cert)
569 569
 }
570 570
 
571 571
 result_t
572
-x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
572
+x509_verify_ns_cert_type(openvpn_x509_cert_t *peer_cert, const int usage)
573 573
 {
574 574
     if (usage == NS_CERT_CHECK_NONE)
575 575
     {
... ...
@@ -577,13 +579,59 @@ x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
577 577
     }
578 578
     if (usage == NS_CERT_CHECK_CLIENT)
579 579
     {
580
-        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
581
-                && (peer_cert->ex_nscert & NS_SSL_CLIENT)) ? SUCCESS : FAILURE;
580
+        /*
581
+         * Unfortunately, X509_check_purpose() does some weird thing that
582
+         * prevent it to take a const argument
583
+         */
584
+        result_t result = X509_check_purpose(peer_cert, X509_PURPOSE_SSL_CLIENT, 0) ?
585
+	       SUCCESS : FAILURE;
586
+
587
+        /*
588
+         * old versions of OpenSSL allow us to make the less strict check we used to
589
+         * do. If this less strict check pass, warn user that this might not be the
590
+         * case when its distribution will update to OpenSSL 1.1
591
+         */
592
+        if (result == FAILURE)
593
+        {
594
+            ASN1_BIT_STRING *ns;
595
+            ns = X509_get_ext_d2i(peer_cert, NID_netscape_cert_type, NULL, NULL);
596
+            result = (ns && ns->length > 0 && (ns->data[0] & NS_SSL_CLIENT)) ? SUCCESS : FAILURE;
597
+            if (result == SUCCESS)
598
+            {
599
+                msg(M_WARN, "X509: Certificate is a client certificate yet it's purpose "
600
+                    "cannot be verified (check may fail in the future)");
601
+            }
602
+            ASN1_BIT_STRING_free(ns);
603
+        }
604
+        return result;
582 605
     }
583 606
     if (usage == NS_CERT_CHECK_SERVER)
584 607
     {
585
-        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
586
-                && (peer_cert->ex_nscert & NS_SSL_SERVER))  ? SUCCESS : FAILURE;
608
+        /*
609
+         * Unfortunately, X509_check_purpose() does some weird thing that
610
+         * prevent it to take a const argument
611
+         */
612
+        result_t result = X509_check_purpose(peer_cert, X509_PURPOSE_SSL_SERVER, 0) ?
613
+	       SUCCESS : FAILURE;
614
+
615
+        /*
616
+         * old versions of OpenSSL allow us to make the less strict check we used to
617
+         * do. If this less strict check pass, warn user that this might not be the
618
+         * case when its distribution will update to OpenSSL 1.1
619
+         */
620
+        if (result == FAILURE)
621
+        {
622
+            ASN1_BIT_STRING *ns;
623
+            ns = X509_get_ext_d2i(peer_cert, NID_netscape_cert_type, NULL, NULL);
624
+            result = (ns && ns->length > 0 && (ns->data[0] & NS_SSL_SERVER)) ? SUCCESS : FAILURE;
625
+            if (result == SUCCESS)
626
+            {
627
+                msg(M_WARN, "X509: Certificate is a server certificate yet it's purpose "
628
+                    "cannot be verified (check may fail in the future)");
629
+            }
630
+            ASN1_BIT_STRING_free(ns);
631
+        }
632
+        return result;
587 633
     }
588 634
 
589 635
     return FAILURE;