diff mbox series

[Openvpn-devel] Log OpenSSL errors on failure to set certificate

Message ID 20231001174920.54154-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Log OpenSSL errors on failure to set certificate |

Commit Message

Selva Nair Oct. 1, 2023, 5:49 p.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Currently we log a bogus error message saying private key password verification
failed when SSL_CTX_use_cert_and_key() fails in pkcs11_openssl.c. Instead print
OpenSSL error queue and exit promptly.

Also log OpenSSL errors when SSL_CTX_use_certiifcate() fails in cryptoapi.c
and elsewhere. Such logging could be useful especially when the ceritficate is
rejected by OpenSSL due to stricter security restrictions in recent versions
of the library.

Change-Id: Ic7ec25ac0503a91d5869b8da966d0065f264af22
Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/cryptoapi.c                   |  2 ++
 src/openvpn/pkcs11_openssl.c              |  6 ++++--
 src/openvpn/ssl_openssl.c                 |  2 ++
 tests/unit_tests/openvpn/test_cryptoapi.c | 11 +++++++++++
 tests/unit_tests/openvpn/test_pkcs11.c    | 11 +++++++++++
 5 files changed, 30 insertions(+), 2 deletions(-)

Comments

Arne Schwabe Oct. 2, 2023, 8:05 a.m. UTC | #1 
Am 01.10.23 um 19:49 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Currently we log a bogus error message saying private key password verification
> failed when SSL_CTX_use_cert_and_key() fails in pkcs11_openssl.c. Instead print
> OpenSSL error queue and exit promptly.
> 
> Also log OpenSSL errors when SSL_CTX_use_certiifcate() fails in cryptoapi.c
> and elsewhere. Such logging could be useful especially when the ceritficate is
> rejected by OpenSSL due to stricter security restrictions in recent versions
> of the library.

Yeah, looks good.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Oct. 2, 2023, 8:29 a.m. UTC | #2 
Good error messages are always an improvement, and not going on after
a "cannot use this certificate" error is propably also a good idea
(so, change M_WARN to M_FATAL makes sense).

For the "test on many different OpenSSL versions and OSes" I have
subjected this to the GH builds and my local MinGW test, and all
succeeded.

Backporting to 2.6 was not fully automatic, had to manually skip
the patch to test_pkcs11.c (which does not exist in 2.6).

Your patch has been applied to the master and release/2.6 branch.

commit 2671dcb69837ae58b3303f11c1b6ba4cee8eea00 (master)
commit ebfa5f3811e92863a3bbcc53b7a3f1b29dff1bc1 (release/2.6)
Author: Selva Nair
Date:   Sun Oct 1 13:49:20 2023 -0400

     Log OpenSSL errors on failure to set certificate

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20231001174920.54154-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27122.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 3b92e481..f7e5b674 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -51,6 +51,7 @@ 
 #include "openssl_compat.h"
 #include "win32.h"
 #include "xkey_common.h"
+#include "crypto_openssl.h"
 
 #ifndef HAVE_XKEY_PROVIDER
 
@@ -505,6 +506,7 @@  SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     if (SSL_CTX_use_certificate(ssl_ctx, cert)
         && SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
     {
+        crypto_print_openssl_errors(M_WARN);
         ret = 1;
     }
 
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 40080efa..aa0819f9 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -302,7 +302,8 @@  xkey_load_from_pkcs11h(pkcs11h_certificate_t certificate,
 
     if (!SSL_CTX_use_cert_and_key(ctx->ctx, x509, pkey, NULL, 0))
     {
-        msg(M_WARN, "PKCS#11: Failed to set cert and private key for OpenSSL");
+        crypto_print_openssl_errors(M_WARN);
+        msg(M_FATAL, "PKCS#11: Failed to set cert and private key for OpenSSL");
         goto cleanup;
     }
     ret = 1;
@@ -369,7 +370,8 @@  pkcs11_init_tls_session(pkcs11h_certificate_t certificate,
 
     if (!SSL_CTX_use_certificate(ssl_ctx->ctx, x509))
     {
-        msg(M_WARN, "PKCS#11: Cannot set certificate for openssl");
+        crypto_print_openssl_errors(M_WARN);
+        msg(M_FATAL, "PKCS#11: Cannot set certificate for openssl");
         goto cleanup;
     }
     ret = 0;
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 0b310de3..b5cc9a7f 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -857,6 +857,7 @@  tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
     /* Load Certificate */
     if (!SSL_CTX_use_certificate(ctx->ctx, cert))
     {
+        crypto_print_openssl_errors(M_WARN);
         crypto_msg(M_FATAL, "Cannot use certificate");
     }
 
@@ -1007,6 +1008,7 @@  tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const char *cert_file,
 end:
     if (!ret)
     {
+        crypto_print_openssl_errors(M_WARN);
         if (cert_file_inline)
         {
             crypto_msg(M_FATAL, "Cannot load inline certificate file");
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c b/tests/unit_tests/openvpn/test_cryptoapi.c
index 008f41c0..d90bfc35 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -58,6 +58,17 @@  management_query_pk_sig(struct management *man, const char *b64_data,
     return NULL;
 }
 
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+    unsigned long e;
+    while ((e = ERR_get_error()))
+    {
+        msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+    }
+}
+
 /* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
 OSSL_LIB_CTX *tls_libctx;
 
diff --git a/tests/unit_tests/openvpn/test_pkcs11.c b/tests/unit_tests/openvpn/test_pkcs11.c
index 235cc43f..b6c130ec 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -44,6 +44,17 @@ 
 
 struct management *management; /* global */
 
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+    unsigned long e;
+    while ((e = ERR_get_error()))
+    {
+        msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+    }
+}
+
 /* stubs for some unused functions instead of pulling in too many dependencies */
 int
 parse_line(const char *line, char **p, const int n, const char *file,