diff mbox series

[Openvpn-devel] Bugfix: dangling pointer passed to pkcs11-helper

Message ID 20230509170517.2637245-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Bugfix: dangling pointer passed to pkcs11-helper |

Commit Message

Selva Nair May 9, 2023, 5:05 p.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Github: Fixes OpenVPN/openvpn#323

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
This will fix #323 is my best guess, untested as yet..
This is a bug that needs fixing, regardless.
 
 src/openvpn/pkcs11_openssl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gert Doering May 10, 2023, 8:13 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

The patch itself looks trivial ("just move the structure to the outer
scope").

The interesting bit is "why" :-) - "set_pss_params()" is harmless (it
just fills the data in the structure), but then mech.pParameter is set
to &pss_params, and *this* is passed to pkcs11h_certificate_signAny_ex()
a few lines down, outside the original scope.

So - the patch makes sense, the bug is obvious in hindsight, and we
also have a confirmation in #323 that the patch fixes a real problem
"depending on compiler and OS", nasty.

For testing, I have only used the GH Action builds - there's two
instances that build with --enable-pkcs11 and run the tests (and I
have no suitable setup locally).

[==========] Running 3 test(s).
Slot 0 has a free/uninitialized token.
The token has been initialized and is reassigned to slot 379532672
[ RUN      ] test_pkcs11_ids
[       OK ] test_pkcs11_ids
[ RUN      ] test_tls_ctx_use_pkcs11
[       OK ] test_tls_ctx_use_pkcs11
[ RUN      ] test_tls_ctx_use_pkcs11__management
[       OK ] test_tls_ctx_use_pkcs11__management
Found token (541bef49-4423-01c1-e7c6-600c169f3580) with matching token label.
The token (softhsm2_tokens_Fi02IS/541bef49-4423-01c1-e7c6-600c169f3580) has been deleted.
[==========] 3 test(s) run.
[  PASSED  ] 3 test(s).
PASS: pkcs11_testdriver


Your patch has been applied to the master branch.

commit f4850745709c5b80ab7d09c03a86c5ceea6d10a2 (master)
commit 7e4becb4cd8be7f0d5ff80cf80877ea152f99830 (release/2.6)
Author: Selva Nair
Date:   Tue May 9 13:05:17 2023 -0400

     Bugfix: dangling pointer passed to pkcs11-helper

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index eee86e17..9b0ab39f 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -165,6 +165,7 @@  xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 {
     pkcs11h_certificate_t cert = handle;
     CK_MECHANISM mech = {CKM_RSA_PKCS, NULL, 0}; /* default value */
+    CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
 
     unsigned char buf[EVP_MAX_MD_SIZE];
     size_t buflen;
@@ -203,7 +204,6 @@  xkey_pkcs11h_sign(void *handle, unsigned char *sig,
         }
         else if (!strcmp(sigalg.padmode, "pss"))
         {
-            CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
             mech.mechanism = CKM_RSA_PKCS_PSS;
 
             if (!set_pss_params(&pss_params, sigalg, cert))