Skip to contentSkip to conversation

Remove dependency on mbed TLS PKCS#11 module

PATCH New Steffan Karger 3 participants 13 messages Sep 11, 2017 – Sep 26, 2018

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 Remove dependency on mbed TLS PKCS#11 module

Hi,

The following three patches remove our dependency on the mbed TLS PKCS#11 module. While doing so, this adds better error reporting and still manages to barely add any lines of code.

Not depending on the mbed TLS PKCS#11 module is good, because mbed TLS has the PKCS#11 module disabled by default. So this makes it easier to use e.g. a distro-provided mbed TLS library. Also, this makes it easier to move away from pkcs11-helper should we want to go that way.

-Steffan

author
#1 lore raw

PATCH 1/3 Do not load certificate from tls_ctx_use_external_private_key()

The cert and key loading logic surrounding management-external-key and management-external cert was somewhat intertwined. Untangle these to prepare for making the external key code more reusable.

The best part is that this even reduces the number of lines of code.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl.c         | 48 +++++++++++++++------------------
 src/openvpn/ssl_backend.h | 15 +++--------
 src/openvpn/ssl_mbedtls.c |  6 ++---
 src/openvpn/ssl_openssl.c | 68 +++++++++++++++++++++--------------------------
 4 files changed, 58 insertions(+), 79 deletions(-)
src/openvpn/ssl.c +22 -26
index cb94229..4582454 100644
@@ -649,41 +649,37 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     }
 #endif
 #ifdef MANAGMENT_EXTERNAL_KEY
-    else if ((options->management_flags & MF_EXTERNAL_KEY)
-             && (options->cert_file || options->management_flags & MF_EXTERNAL_CERT))
+    else if (options->management_flags & MF_EXTERNAL_CERT)
     {
-        if (options->cert_file)
-        {
-            tls_ctx_use_external_private_key(new_ctx, options->cert_file,
-                                             options->cert_file_inline);
-        }
-        else
-        {
-            char *external_certificate = management_query_cert(management,
-                                                               options->management_certificate);
-            tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
-                                             external_certificate);
-            free(external_certificate);
-        }
+        char *cert = management_query_cert(management,
+                                           options->management_certificate);
+        tls_ctx_load_cert_file(new_ctx, INLINE_FILE_TAG, cert);
+        free(cert);
     }
 #endif
-    else
+    else if (options->cert_file)
+    {
+        tls_ctx_load_cert_file(new_ctx, options->cert_file, options->cert_file_inline);
+    }
+
+    if (options->priv_key_file)
     {
-        /* Load Certificate */
-        if (options->cert_file)
+        if (0 != tls_ctx_load_priv_file(new_ctx, options->priv_key_file,
+                                        options->priv_key_file_inline))
         {
-            tls_ctx_load_cert_file(new_ctx, options->cert_file, options->cert_file_inline);
+            goto err;
         }
-
-        /* Load Private Key */
-        if (options->priv_key_file)
+    }
+#ifdef MANAGMENT_EXTERNAL_KEY
+    else if (options->management_flags & MF_EXTERNAL_KEY)
+    {
+        if (!tls_ctx_use_management_external_key(new_ctx))
         {
-            if (0 != tls_ctx_load_priv_file(new_ctx, options->priv_key_file, options->priv_key_file_inline))
-            {
-                goto err;
-            }
+            msg (M_WARN, "Cannot initialize mamagement-external-key");
+            goto err;
         }
     }
+#endif
 
     if (options->ca_file || options->ca_path)
     {
src/openvpn/ssl_backend.h +4 -11
index aba5a4d..6fa885d 100644
@@ -259,8 +259,7 @@ void tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const char *cert_file,
  *                              successful.
  */
 int tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
-                           const char *priv_key_file_inline
-                           );
+                           const char *priv_key_file_inline);
 
 #ifdef MANAGMENT_EXTERNAL_KEY
 

@@ -269,18 +268,12 @@ int tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
  * private key matching the given certificate.
  *
  * @param ctx                   TLS context to use
- * @param cert_file             The file name to load the certificate from, or
- *                              "[[INLINE]]" in the case of inline files.
- * @param cert_file_inline      A string containing the certificate
  *
- * @return                      1 if an error occurred, 0 if parsing was
- *                              successful.
+ * @return                      1 if an error occurred, 0 if successful.
  */
-int tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
-                                     const char *cert_file, const char *cert_file_inline);
-
-#endif
+int tls_ctx_use_management_external_key(struct tls_root_ctx *ctx);
 
+#endif /* MANAGMENT_EXTERNAL_KEY */
 
 /**
  * Load certificate authority certificates from the given file or path.
src/openvpn/ssl_mbedtls.c +2 -4
index 861d936..6d023af 100644
@@ -572,15 +572,13 @@ external_key_len(void *vctx)
 }
 
 int
-tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
-                                 const char *cert_file, const char *cert_file_inline)
+tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
 {
     ASSERT(NULL != ctx);
 
-    tls_ctx_load_cert_file(ctx, cert_file, cert_file_inline);
-
     if (ctx->crt_chain == NULL)
     {
+        msg (M_WARN, "ERROR: external key requires a certificate.");
         return 0;
     }
src/openvpn/ssl_openssl.c +30 -38

145 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#2 lore raw

PATCH 2/3 mbedtls: make external signing code generic

In reply to Steffan Karger

This prepares for reusing this code from the mbedtls pkcs11 implementation. The change itself should not have any functional impact.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl_mbedtls.c | 115 ++++++++++++++++++++++++----------------------
 src/openvpn/ssl_mbedtls.h |  41 +++++++++++++++--
 2 files changed, 98 insertions(+), 58 deletions(-)
src/openvpn/ssl_mbedtls.c +60 -55
index 6d023af..0cf89a8 100644
@@ -146,12 +146,6 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->priv_key_pkcs11);
         }
 #endif
-#if defined(MANAGMENT_EXTERNAL_KEY)
-        if (ctx->external_key != NULL)
-        {
-            free(ctx->external_key);
-        }
-#endif
 
         if (ctx->allowed_ciphers)
         {

@@ -413,13 +407,6 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
     return 0;
 }
 
-#ifdef MANAGMENT_EXTERNAL_KEY
-
-
-struct external_context {
-    size_t signature_length;
-};
-
 /**
  * external_pkcs1_sign implements a mbed TLS rsa_sign_func callback, that uses
  * the management interface to request an RSA signature for the supplied hash.

@@ -446,11 +433,9 @@ external_pkcs1_sign( void *ctx_voidptr,
                      unsigned char *sig )
 {
     struct external_context *const ctx = ctx_voidptr;
-    char *in_b64 = NULL;
-    char *out_b64 = NULL;
     int rv;
-    unsigned char *p = sig;
-    size_t asn_len = 0, oid_size = 0, sig_len = 0;
+    uint8_t *to_sign = NULL;
+    size_t asn_len = 0, oid_size = 0;
     const char *oid = NULL;
 
     if (NULL == ctx)

@@ -486,12 +471,14 @@ external_pkcs1_sign( void *ctx_voidptr,
         asn_len = 10 + oid_size;
     }
 
-    sig_len = ctx->signature_length;
-    if ( (SIZE_MAX - hashlen) < asn_len || (hashlen + asn_len) > sig_len)
+    if ((SIZE_MAX - hashlen) < asn_len
+        || ctx->signature_length < (asn_len + hashlen))
     {
         return MBEDTLS_ERR_RSA_BAD_INPUT_DATA;
     }
 
+    ALLOC_ARRAY_CLEAR(to_sign, uint8_t, asn_len + hashlen);
+    uint8_t *p = to_sign;
     if (md_alg != MBEDTLS_MD_NONE)
     {
         /*

@@ -516,34 +503,16 @@ external_pkcs1_sign( void *ctx_voidptr,
         *p++ = MBEDTLS_ASN1_OCTET_STRING;
         *p++ = hashlen;
 
-        /* Determine added ASN length */
-        asn_len = p - sig;
+        /* Double-check ASN length */
+        ASSERT(asn_len == p - to_sign);
     }
 
     /* Copy the hash to be signed */
-    memcpy( p, hash, hashlen );
-
-    /* convert 'from' to base64 */
-    if (openvpn_base64_encode(sig, asn_len + hashlen, &in_b64) <= 0)
-    {
-        rv = MBEDTLS_ERR_RSA_BAD_INPUT_DATA;
-        goto done;
-    }
+    memcpy(p, hash, hashlen);
 
-    /* call MI for signature */
-    if (management)
-    {
-        out_b64 = management_query_rsa_sig(management, in_b64);
-    }
-    if (!out_b64)
-    {
-        rv = MBEDTLS_ERR_RSA_PRIVATE_FAILED;
-        goto done;
-    }
-
-    /* decode base64 signature to binary and verify length */
-    if (openvpn_base64_decode(out_b64, sig, ctx->signature_length) !=
-        ctx->signature_length)
+    /* Call external signature function */
+    if (!ctx->sign(ctx->sign_ctx, to_sign, asn_len + hashlen, sig,
+                   ctx->signature_length))
     {
         rv = MBEDTLS_ERR_RSA_PRIVATE_FAILED;
         goto done;

@@ -552,14 +521,7 @@ external_pkcs1_sign( void *ctx_voidptr,
     rv = 0;
 
 done:
-    if (in_b64)
-    {
-        free(in_b64);
-    }
-    if (out_b64)
-    {
-        free(out_b64);
-    }
+    free(to_sign);
     return rv;
 }
 

@@ -572,7 +534,8 @@ external_key_len(void *vctx)
 }
 
 int
-tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
+tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,
+                                  external_sign_func sign_func, void *sign_ctx)
 {
     ASSERT(NULL != ctx);
 

@@ -582,11 +545,12 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
         return 0;
     }
 
-    ALLOC_OBJ_CLEAR(ctx->external_key, struct external_context);
-    ctx->external_key->signature_length = mbedtls_pk_get_len(&ctx->crt_chain->pk);
+    ctx->external_key.signature_length = mbedtls_pk_get_len(&ctx->crt_chain->pk);
+    ctx->external_key.sign = sign_func;
+    ctx->external_key.sign_ctx = sign_ctx;
 
     ALLOC_OBJ_CLEAR(ctx->priv_key, mbedtls_pk_context);
-    if (!mbed_ok(mbedtls_pk_setup_rsa_alt(ctx->priv_key, ctx->external_key,
+    if (!mbed_ok(mbedtls_pk_setup_rsa_alt(ctx->priv_key, &ctx->external_key,
                                           NULL, external_pkcs1_sign, external_key_len)))
     {
         return 0;

@@ -594,6 +558,47 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
 
     return 1;
 }
+
+#ifdef MANAGMENT_EXTERNAL_KEY
+
+/** Query the management interface for a signature, see external_sign_func. */
+static bool
+management_sign_func(void *sign_ctx, const void *src, size_t src_len,
+                     void *dst, size_t dst_len)
+{
+    bool ret = false;
+    char *src_b64 = NULL;
+    char *dst_b64 = NULL;
+
+    if (!management || (openvpn_base64_encode(src, src_len, &src_b64) <= 0))
+    {
+        goto cleanup;
+    }
+
+    if (!(dst_b64 = management_query_rsa_sig(management, src_b64)))
+    {
+        goto cleanup;
+    }
+
+    if (openvpn_base64_decode(dst_b64, dst, dst_len) != dst_len)
+    {
+        goto cleanup;
+    }
+
+    ret = true;
+cleanup:
+    free (src_b64);
+    free (dst_b64);
+
+    return ret;
+}
+
+int
+tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
+{
+    return tls_ctx_use_external_signing_func(ctx, management_sign_func, NULL);
+}
+
 #endif /* ifdef MANAGMENT_EXTERNAL_KEY */
 
 void
src/openvpn/ssl_mbedtls.h +38 -3

58 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#3 lore raw

PATCH 3/3 mbedtls: remove dependency on mbedtls pkcs11 module

In reply to Steffan Karger

Instead of using mbedtls's pkcs11 module, reuse the code we already have for management-external-key to also do pkcs11 signatures. As far as mbed is concerned, we simply provide an external signature.

This has the following advantages:
* We no longer need mbed TLS to be compiled with the pkcs11 modules enabled (which is not enabled by default). This makes it easier to use a system/distribution-provided mbed shared library.
* We no longer have a dependency on pkcs11-helper through mbed TLS. So if we want to migrate to some other pkcs11 lib (see e.g. trac #491, #538 and #549 for reason why), this will be easier.

While touching this code, switch from M_FATAL to M_WARN and proper error handling. This improves the error reporting, and helps prevent potential future DoS attacks if someone starts using these functions on peer input.

Signed-off-by Steffan Karger <[email protected]...>
---
 configure.ac                 | 29 ---------------
 src/openvpn/pkcs11_mbedtls.c | 87 +++++++++++++++++++++++++++++---------------
 src/openvpn/ssl_mbedtls.c    |  7 +---
 src/openvpn/ssl_mbedtls.h    |  6 +--
 4 files changed, 62 insertions(+), 67 deletions(-)
configure.ac +0 -29
index 6f1044e..50d0352 100644
@@ -992,35 +992,6 @@ elif test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "mbedtls"; th
 		[AC_MSG_ERROR([mbed TLS 2.y.z required])]
 	)
 
-	mbedtls_with_pkcs11="no"
-	AC_COMPILE_IFELSE(
-		[AC_LANG_PROGRAM(
-			[[
-#include <mbedtls/config.h>
-			]],
-			[[
-#ifndef MBEDTLS_PKCS11_C
-#error pkcs11 wrapper missing
-#endif
-			]]
-		)],
-		mbedtls_with_pkcs11="yes")
-
-	AC_MSG_CHECKING([mbedtls pkcs11 support])
-	if test "${enable_pkcs11}" = "yes"; then
-		if test "${mbedtls_with_pkcs11}" = "yes"; then
-			AC_MSG_RESULT([ok])
-		else
-			AC_MSG_ERROR([mbedtls has no pkcs11 wrapper compiled in])
-		fi
-	else
-		if test "${mbedtls_with_pkcs11}" != "yes"; then
-			AC_MSG_RESULT([ok])
-		else
-			AC_MSG_ERROR([mbed TLS compiled with PKCS11, while OpenVPN is not])
-		fi
-	fi
-
 	have_crypto_aead_modes="yes"
 	AC_CHECK_FUNCS(
 		[ \
src/openvpn/pkcs11_mbedtls.c +57 -30

140 lines not on this page · raw message

src/openvpn/ssl_mbedtls.c +2 -5
index 0cf89a8..975d0b9 100644
@@ -43,6 +43,7 @@
 #include "buffer.h"
 #include "misc.h"
 #include "manage.h"
+#include "pkcs11_backend.h"
 #include "ssl_common.h"
 
 #include <mbedtls/havege.h>

@@ -140,11 +141,7 @@ tls_ctx_free(struct tls_root_ctx *ctx)
         }
 
 #if defined(ENABLE_PKCS11)
-        if (ctx->priv_key_pkcs11 != NULL)
-        {
-            mbedtls_pkcs11_priv_key_free(ctx->priv_key_pkcs11);
-            free(ctx->priv_key_pkcs11);
-        }
+        pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert);
 #endif
 
         if (ctx->allowed_ciphers)
src/openvpn/ssl_mbedtls.h +3 -3
index 2b76cc6..adb9541 100644
@@ -35,7 +35,7 @@
 #include <mbedtls/x509_crt.h>
 
 #if defined(ENABLE_PKCS11)
-#include <mbedtls/pkcs11.h>
+#include <pkcs11-helper-1.0/pkcs11h-certificate.h>
 #endif
 
 typedef struct _buffer_entry buffer_entry;

@@ -99,8 +99,8 @@ struct tls_root_ctx {
     mbedtls_x509_crl *crl;              /**< Certificate Revocation List */
     time_t crl_last_mtime;              /**< CRL last modification time */
     off_t crl_last_size;                /**< size of last loaded CRL */
-#if defined(ENABLE_PKCS11)
-    mbedtls_pkcs11_context *priv_key_pkcs11;    /**< PKCS11 private key */
+#ifdef ENABLE_PKCS11
+    pkcs11h_certificate_t pkcs11_cert;  /**< PKCS11 certificate */
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
Signature · 1 line
2.7.4
author
#4 lore raw

PATCH v2 1/3 Do not load certificate from tls_ctx_use_external_private_key()

In reply to Steffan Karger

The cert and key loading logic surrounding management-external-key and management-external cert was somewhat intertwined. Untangle these to prepare for making the external key code more reusable.

The best part is that this even reduces the number of lines of code.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase onto current master

 src/openvpn/ssl.c         | 48 ++++++++++++++++-------------------
 src/openvpn/ssl_backend.h | 15 +++--------
 src/openvpn/ssl_mbedtls.c |  6 ++---
 src/openvpn/ssl_openssl.c | 64 +++++++++++++++++++++++------------------------
 4 files changed, 59 insertions(+), 74 deletions(-)
src/openvpn/ssl.c +22 -26
index dcb5445..4257c33 100644
@@ -657,41 +657,37 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     }
 #endif
 #ifdef MANAGMENT_EXTERNAL_KEY
-    else if ((options->management_flags & MF_EXTERNAL_KEY)
-             && (options->cert_file || options->management_flags & MF_EXTERNAL_CERT))
+    else if (options->management_flags & MF_EXTERNAL_CERT)
     {
-        if (options->cert_file)
-        {
-            tls_ctx_use_external_private_key(new_ctx, options->cert_file,
-                                             options->cert_file_inline);
-        }
-        else
-        {
-            char *external_certificate = management_query_cert(management,
-                                                               options->management_certificate);
-            tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
-                                             external_certificate);
-            free(external_certificate);
-        }
+        char *cert = management_query_cert(management,
+                                           options->management_certificate);
+        tls_ctx_load_cert_file(new_ctx, INLINE_FILE_TAG, cert);
+        free(cert);
     }
 #endif
-    else
+    else if (options->cert_file)
+    {
+        tls_ctx_load_cert_file(new_ctx, options->cert_file, options->cert_file_inline);
+    }
+
+    if (options->priv_key_file)
     {
-        /* Load Certificate */
-        if (options->cert_file)
+        if (0 != tls_ctx_load_priv_file(new_ctx, options->priv_key_file,
+                                        options->priv_key_file_inline))
         {
-            tls_ctx_load_cert_file(new_ctx, options->cert_file, options->cert_file_inline);
+            goto err;
         }
-
-        /* Load Private Key */
-        if (options->priv_key_file)
+    }
+#ifdef MANAGMENT_EXTERNAL_KEY
+    else if (options->management_flags & MF_EXTERNAL_KEY)
+    {
+        if (tls_ctx_use_management_external_key(new_ctx))
         {
-            if (0 != tls_ctx_load_priv_file(new_ctx, options->priv_key_file, options->priv_key_file_inline))
-            {
-                goto err;
-            }
+            msg (M_WARN, "Cannot initialize mamagement-external-key");
+            goto err;
         }
     }
+#endif
 
     if (options->ca_file || options->ca_path)
     {
src/openvpn/ssl_backend.h +4 -11
index 202fe3f..5023c02 100644
@@ -270,8 +270,7 @@ void tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const char *cert_file,
  *                              successful.
  */
 int tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
-                           const char *priv_key_file_inline
-                           );
+                           const char *priv_key_file_inline);
 
 #ifdef MANAGMENT_EXTERNAL_KEY
 

@@ -280,18 +279,12 @@ int tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
  * private key matching the given certificate.
  *
  * @param ctx                   TLS context to use
- * @param cert_file             The file name to load the certificate from, or
- *                              "[[INLINE]]" in the case of inline files.
- * @param cert_file_inline      A string containing the certificate
  *
- * @return                      1 if an error occurred, 0 if parsing was
- *                              successful.
+ * @return                      1 if an error occurred, 0 if successful.
  */
-int tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
-                                     const char *cert_file, const char *cert_file_inline);
-
-#endif
+int tls_ctx_use_management_external_key(struct tls_root_ctx *ctx);
 
+#endif /* MANAGMENT_EXTERNAL_KEY */
 
 /**
  * Load certificate authority certificates from the given file or path.
src/openvpn/ssl_mbedtls.c +2 -4
index ef83e65..3c6d872 100644
@@ -621,15 +621,13 @@ external_key_len(void *vctx)
 }
 
 int
-tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
-                                 const char *cert_file, const char *cert_file_inline)
+tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
 {
     ASSERT(NULL != ctx);
 
-    tls_ctx_load_cert_file(ctx, cert_file, cert_file_inline);
-
     if (ctx->crt_chain == NULL)
     {
+        msg (M_WARN, "ERROR: external key requires a certificate.");
         return 1;
     }
src/openvpn/ssl_openssl.c +31 -33

129 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#5 lore raw

PATCH v2 2/3 mbedtls: make external signing code generic

This prepares for reusing this code from the mbedtls pkcs11 implementation. The change itself should not have any functional impact.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase onto current master

 src/openvpn/ssl_mbedtls.c | 115 ++++++++++++++++++++++++----------------------
 src/openvpn/ssl_mbedtls.h |  41 +++++++++++++++--
 2 files changed, 98 insertions(+), 58 deletions(-)
src/openvpn/ssl_mbedtls.c +60 -55
index 3c6d872..4c39cf3 100644
@@ -173,12 +173,6 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->priv_key_pkcs11);
         }
 #endif
-#if defined(MANAGMENT_EXTERNAL_KEY)
-        if (ctx->external_key != NULL)
-        {
-            free(ctx->external_key);
-        }
-#endif
 
         if (ctx->allowed_ciphers)
         {

@@ -462,13 +456,6 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
     return 0;
 }
 
-#ifdef MANAGMENT_EXTERNAL_KEY
-
-
-struct external_context {
-    size_t signature_length;
-};
-
 /**
  * external_pkcs1_sign implements a mbed TLS rsa_sign_func callback, that uses
  * the management interface to request an RSA signature for the supplied hash.

@@ -495,11 +482,9 @@ external_pkcs1_sign( void *ctx_voidptr,
                      unsigned char *sig )
 {
     struct external_context *const ctx = ctx_voidptr;
-    char *in_b64 = NULL;
-    char *out_b64 = NULL;
     int rv;
-    unsigned char *p = sig;
-    size_t asn_len = 0, oid_size = 0, sig_len = 0;
+    uint8_t *to_sign = NULL;
+    size_t asn_len = 0, oid_size = 0;
     const char *oid = NULL;
 
     if (NULL == ctx)

@@ -535,12 +520,14 @@ external_pkcs1_sign( void *ctx_voidptr,
         asn_len = 10 + oid_size;
     }
 
-    sig_len = ctx->signature_length;
-    if ( (SIZE_MAX - hashlen) < asn_len || (hashlen + asn_len) > sig_len)
+    if ((SIZE_MAX - hashlen) < asn_len
+        || ctx->signature_length < (asn_len + hashlen))
     {
         return MBEDTLS_ERR_RSA_BAD_INPUT_DATA;
     }
 
+    ALLOC_ARRAY_CLEAR(to_sign, uint8_t, asn_len + hashlen);
+    uint8_t *p = to_sign;
     if (md_alg != MBEDTLS_MD_NONE)
     {
         /*

@@ -565,34 +552,16 @@ external_pkcs1_sign( void *ctx_voidptr,
         *p++ = MBEDTLS_ASN1_OCTET_STRING;
         *p++ = hashlen;
 
-        /* Determine added ASN length */
-        asn_len = p - sig;
+        /* Double-check ASN length */
+        ASSERT(asn_len == p - to_sign);
     }
 
     /* Copy the hash to be signed */
-    memcpy( p, hash, hashlen );
-
-    /* convert 'from' to base64 */
-    if (openvpn_base64_encode(sig, asn_len + hashlen, &in_b64) <= 0)
-    {
-        rv = MBEDTLS_ERR_RSA_BAD_INPUT_DATA;
-        goto done;
-    }
+    memcpy(p, hash, hashlen);
 
-    /* call MI for signature */
-    if (management)
-    {
-        out_b64 = management_query_pk_sig(management, in_b64);
-    }
-    if (!out_b64)
-    {
-        rv = MBEDTLS_ERR_RSA_PRIVATE_FAILED;
-        goto done;
-    }
-
-    /* decode base64 signature to binary and verify length */
-    if (openvpn_base64_decode(out_b64, sig, ctx->signature_length) !=
-        ctx->signature_length)
+    /* Call external signature function */
+    if (!ctx->sign(ctx->sign_ctx, to_sign, asn_len + hashlen, sig,
+                   ctx->signature_length))
     {
         rv = MBEDTLS_ERR_RSA_PRIVATE_FAILED;
         goto done;

@@ -601,14 +570,7 @@ external_pkcs1_sign( void *ctx_voidptr,
     rv = 0;
 
 done:
-    if (in_b64)
-    {
-        free(in_b64);
-    }
-    if (out_b64)
-    {
-        free(out_b64);
-    }
+    free(to_sign);
     return rv;
 }
 

@@ -621,7 +583,8 @@ external_key_len(void *vctx)
 }
 
 int
-tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
+tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,
+                                  external_sign_func sign_func, void *sign_ctx)
 {
     ASSERT(NULL != ctx);
 

@@ -631,11 +594,12 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
         return 1;
     }
 
-    ALLOC_OBJ_CLEAR(ctx->external_key, struct external_context);
-    ctx->external_key->signature_length = mbedtls_pk_get_len(&ctx->crt_chain->pk);
+    ctx->external_key.signature_length = mbedtls_pk_get_len(&ctx->crt_chain->pk);
+    ctx->external_key.sign = sign_func;
+    ctx->external_key.sign_ctx = sign_ctx;
 
     ALLOC_OBJ_CLEAR(ctx->priv_key, mbedtls_pk_context);
-    if (!mbed_ok(mbedtls_pk_setup_rsa_alt(ctx->priv_key, ctx->external_key,
+    if (!mbed_ok(mbedtls_pk_setup_rsa_alt(ctx->priv_key, &ctx->external_key,
                                           NULL, external_pkcs1_sign, external_key_len)))
     {
         return 1;

@@ -643,6 +607,47 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
 
     return 0;
 }
+
+#ifdef MANAGMENT_EXTERNAL_KEY
+
+/** Query the management interface for a signature, see external_sign_func. */
+static bool
+management_sign_func(void *sign_ctx, const void *src, size_t src_len,
+                     void *dst, size_t dst_len)
+{
+    bool ret = false;
+    char *src_b64 = NULL;
+    char *dst_b64 = NULL;
+
+    if (!management || (openvpn_base64_encode(src, src_len, &src_b64) <= 0))
+    {
+        goto cleanup;
+    }
+
+    if (!(dst_b64 = management_query_pk_sig(management, src_b64)))
+    {
+        goto cleanup;
+    }
+
+    if (openvpn_base64_decode(dst_b64, dst, dst_len) != dst_len)
+    {
+        goto cleanup;
+    }
+
+    ret = true;
+cleanup:
+    free (src_b64);
+    free (dst_b64);
+
+    return ret;
+}
+
+int
+tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
+{
+    return tls_ctx_use_external_signing_func(ctx, management_sign_func, NULL);
+}
+
 #endif /* ifdef MANAGMENT_EXTERNAL_KEY */
 
 void
src/openvpn/ssl_mbedtls.h +38 -3

58 lines not on this page · raw message

Signature · 1 line
2.7.4
author
#8 lore raw

PATCH v2 3/3 mbedtls: remove dependency on mbedtls pkcs11 module

In reply to Steffan Karger

Instead of using mbedtls's pkcs11 module, reuse the code we already have for management-external-key to also do pkcs11 signatures. As far as mbed is concerned, we simply provide an external signature.

This has the following advantages:
* We no longer need mbed TLS to be compiled with the pkcs11 modules enabled (which is not enabled by default). This makes it easier to use a system/distribution-provided mbed shared library.
* We no longer have a dependency on pkcs11-helper through mbed TLS. So if we want to migrate to some other pkcs11 lib (see e.g. trac #491, #538 and #549 for reason why), this will be easier.

While touching this code, switch from M_FATAL to M_WARN and proper error handling. This improves the error reporting, and helps prevent potential future DoS attacks if someone starts using these functions on peer input.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase onto current master

 configure.ac                 | 29 ---------------
 src/openvpn/pkcs11_mbedtls.c | 87 +++++++++++++++++++++++++++++---------------
 src/openvpn/ssl_mbedtls.c    |  7 +---
 src/openvpn/ssl_mbedtls.h    |  6 +--
 4 files changed, 62 insertions(+), 67 deletions(-)
configure.ac +0 -29
index 9c31435..3d8e15b 100644
@@ -994,35 +994,6 @@ elif test "${with_crypto_library}" = "mbedtls"; then
 		[AC_MSG_ERROR([mbed TLS 2.y.z required])]
 	)
 
-	mbedtls_with_pkcs11="no"
-	AC_COMPILE_IFELSE(
-		[AC_LANG_PROGRAM(
-			[[
-#include <mbedtls/config.h>
-			]],
-			[[
-#ifndef MBEDTLS_PKCS11_C
-#error pkcs11 wrapper missing
-#endif
-			]]
-		)],
-		mbedtls_with_pkcs11="yes")
-
-	AC_MSG_CHECKING([mbedtls pkcs11 support])
-	if test "${enable_pkcs11}" = "yes"; then
-		if test "${mbedtls_with_pkcs11}" = "yes"; then
-			AC_MSG_RESULT([ok])
-		else
-			AC_MSG_ERROR([mbedtls has no pkcs11 wrapper compiled in])
-		fi
-	else
-		if test "${mbedtls_with_pkcs11}" != "yes"; then
-			AC_MSG_RESULT([ok])
-		else
-			AC_MSG_ERROR([mbed TLS compiled with PKCS11, while OpenVPN is not])
-		fi
-	fi
-
 	have_crypto_aead_modes="yes"
 	AC_CHECK_FUNCS(
 		[ \
src/openvpn/pkcs11_mbedtls.c +57 -30

140 lines not on this page · raw message

src/openvpn/ssl_mbedtls.c +2 -5
index 4c39cf3..e4850cb 100644
@@ -43,6 +43,7 @@
 #include "buffer.h"
 #include "misc.h"
 #include "manage.h"
+#include "pkcs11_backend.h"
 #include "ssl_common.h"
 
 #include <mbedtls/havege.h>

@@ -167,11 +168,7 @@ tls_ctx_free(struct tls_root_ctx *ctx)
         }
 
 #if defined(ENABLE_PKCS11)
-        if (ctx->priv_key_pkcs11 != NULL)
-        {
-            mbedtls_pkcs11_priv_key_free(ctx->priv_key_pkcs11);
-            free(ctx->priv_key_pkcs11);
-        }
+        pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert);
 #endif
 
         if (ctx->allowed_ciphers)
src/openvpn/ssl_mbedtls.h +3 -3
index 73b4c1a..998d6f2 100644
@@ -35,7 +35,7 @@
 #include <mbedtls/x509_crt.h>
 
 #if defined(ENABLE_PKCS11)
-#include <mbedtls/pkcs11.h>
+#include <pkcs11-helper-1.0/pkcs11h-certificate.h>
 #endif
 
 typedef struct _buffer_entry buffer_entry;

@@ -99,8 +99,8 @@ struct tls_root_ctx {
     mbedtls_x509_crl *crl;              /**< Certificate Revocation List */
     time_t crl_last_mtime;              /**< CRL last modification time */
     off_t crl_last_size;                /**< size of last loaded CRL */
-#if defined(ENABLE_PKCS11)
-    mbedtls_pkcs11_context *priv_key_pkcs11;    /**< PKCS11 private key */
+#ifdef ENABLE_PKCS11
+    pkcs11h_certificate_t pkcs11_cert;  /**< PKCS11 certificate */
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
Signature · 1 line
2.7.4
Acked-by on v2 2/3
#6 lore raw

Re: [Openvpn-devel] [PATCH v2 2/3] mbedtls: make external signing code generic

In reply to Steffan Karger

Am 14.09.18 um 11:14 schrieb Steffan Karger:

Quoted 4 lines

This prepares for reusing this code from the mbedtls pkcs11 implementation. The change itself should not have any functional impact.

Ack this only reorganises the code and adds a pointer to external key function.

Acked-by Arne Schwabe <[email protected]...>
Acked-by on v2 1/3
#11 lore raw

Re: [Openvpn-devel] [PATCH v2 1/3] Do not load certificate from tls_ctx_use_external_private_key()

In reply to Steffan Karger

Am 14.09.18 um 11:14 schrieb Steffan Karger:

Quoted 6 lines

The cert and key loading logic surrounding management-external-key and management-external cert was somewhat intertwined. Untangle these to prepare for making the external key code more reusable.

The best part is that this even reduces the number of lines of code.

ACK. The previous code is more an artefact of MF_EXTERNAL_KEY and when it was introduced that the simplicification wasn't made back then.

Acked-by Arne Schwabe <[email protected]...>
Acked-by on v2 3/3
#9 lore raw

Re: [Openvpn-devel] [PATCH v2 3/3] mbedtls: remove dependency on mbedtls pkcs11 module

In reply to Steffan Karger

Am 14.09.18 um 11:14 schrieb Steffan Karger:

Quoted 12 lines

Instead of using mbedtls's pkcs11 module, reuse the code we already have for management-external-key to also do pkcs11 signatures. As far as mbed is concerned, we simply provide an external signature.

This has the following advantages:
* We no longer need mbed TLS to be compiled with the pkcs11 modules enabled (which is not enabled by default). This makes it easier to use a system/distribution-provided mbed shared library.
* We no longer have a dependency on pkcs11-helper through mbed TLS. So if we want to migrate to some other pkcs11 lib (see e.g. trac #491, #538 and #549 for reason why), this will be easier.

Quoted 4 lines

While touching this code, switch from M_FATAL to M_WARN and proper error handling. This improves the error reporting, and helps prevent potential future DoS attacks if someone starts using these functions on peer input.

Ack. The code makes sense. I could not really test it since I don't have a PCKS#11 environement to test it but it looks good enough and I assume Steffan has already tested it.

Acked-by Arne Schwabe <[email protected]...>
#12 lore raw

[Openvpn-devel] [PATCH applied] Re: Do not load certificate from tls_ctx_use_external_private_key()

In reply to Steffan Karger

Your patch has been applied to the master branch.

I have only done a very cursory sanity check, plus test build (of course, mbedtls + openssl), and fixed one funky indentation artefact (8 spaces plus a tab in the very last change).

commit 73513aaa301e9e9413b6156ed263dd27f8fad7fd

Author: Steffan Karger
Date:   Fri Sep 14 11:14:17 2018 +0200
     Do not load certificate from tls_ctx_use_external_private_key()

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

--
kind regards,

Gert Doering

#7 lore raw

[Openvpn-devel] [PATCH applied] Re: mbedtls: make external signing code generic

In reply to Steffan Karger

Tested with a mbedTLS "t_client" run, but no "external key" tests here
- trusting Arne and Steffan on this. Cursory review.

Your patch has been applied to the master branch.

commit 03defa3b29eafc954304532d766aff11712ff9de

Author: Steffan Karger
Date:   Fri Sep 14 11:14:18 2018 +0200
     mbedtls: make external signing code generic

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

--
kind regards,

Gert Doering

#10 lore raw

[Openvpn-devel] [PATCH applied] Re: mbedtls: remove dependency on mbedtls pkcs11 module

In reply to Steffan Karger

Cursory review, way too much crypto / too little time for me to say "I understand the changes". But nothing that looks obviously erroneous.

Trusting Arne, Steffan and my t_client tests on this :-)

Your patch has been applied to the master branch.

commit 03c8bfc90fbc63007f62d3ed165942d149225551

Author: Steffan Karger
Date:   Fri Sep 14 11:14:19 2018 +0200
     mbedtls: remove dependency on mbedtls pkcs11 module

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

--
kind regards,

Gert Doering