Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

2023-03-14 Thread Selva Nair 
Hi,

On Tue, Mar 14, 2023 at 5:54 AM David Sommerseth 
wrote:

>
>
> Just got feedback from the reporter in the Fedora bugzilla; this patch
> works well on Fedora 38.
>
> I suggest adding this tag to the commit log.  Feel free to add the URL
> tag to the bugzilla ticket too.
>
> Tested-by: flor...@apolloner.eu


Thanks. Updated patch is on the list.

I "heard" whispers of "2.6.2 coming soon" in another thread. Would be great
if this fix can make it.

As this also touches cryptoapicert (via a refactor), an independent test on
Windows
would be useful.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

2023-03-14 Thread David Sommerseth 

On 14/03/2023 10:02, David Sommerseth wrote:

On 14/03/2023 09:45, David Sommerseth wrote:

On 11/03/2023 06:24, selva.n...@gmail.com wrote:

From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use 
pkcs11h_certificate_signAny_ex()
   which returns EC signature as raw r|s concatenated. But OpenSSL 
expects

   a DER encoded ASN.1 structure.

   Do this conversion as done in cryptoapi.c. For code re-use, 
ecdsa_bin2sig()

   is consolidated with sig to DER conversion as ecdsa_bin2der() and
   moved to xkey_helper.c

   In the past when we used OpenSSL hooks installed by pkcs11-helper,
   such a conversion was not required as it was internally handled by
   the library.

Reported by: Tom 

Signed-off-by: Selva Nair 
Just FYI, this report appeared in the bugzilla for the Fedora 
packaging.   This seems related to this patch.



I will try to prepare a Fedora build with this patch added, for 
further testing.


Fedora Koji builds of OpenVPN 2.6.1 with this patch included:

Fedora 38: 
  x86_64 packages: 



Fedora 37: 
  x86_64 packages: 



Just got feedback from the reporter in the Fedora bugzilla; this patch 
works well on Fedora 38.


I suggest adding this tag to the commit log.  Feel free to add the URL 
tag to the bugzilla ticket too.


Tested-by: flor...@apolloner.eu



--
kind regards,

David Sommerseth
OpenVPN Inc




___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

2023-03-14 Thread David Sommerseth 

On 14/03/2023 09:45, David Sommerseth wrote:

On 11/03/2023 06:24, selva.n...@gmail.com wrote:

From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use 
pkcs11h_certificate_signAny_ex()
   which returns EC signature as raw r|s concatenated. But OpenSSL 
expects

   a DER encoded ASN.1 structure.

   Do this conversion as done in cryptoapi.c. For code re-use, 
ecdsa_bin2sig()

   is consolidated with sig to DER conversion as ecdsa_bin2der() and
   moved to xkey_helper.c

   In the past when we used OpenSSL hooks installed by pkcs11-helper,
   such a conversion was not required as it was internally handled by
   the library.

Reported by: Tom 

Signed-off-by: Selva Nair 
Just FYI, this report appeared in the bugzilla for the Fedora packaging. 
  This seems related to this patch.



I will try to prepare a Fedora build with this patch added, for further 
testing.


Fedora Koji builds of OpenVPN 2.6.1 with this patch included:

Fedora 38: 
 x86_64 packages: 



Fedora 37: 
 x86_64 packages: 





--
kind regards,

David Sommerseth
OpenVPN Inc




___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

2023-03-14 Thread David Sommerseth 

On 11/03/2023 06:24, selva.n...@gmail.com wrote:

From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
   which returns EC signature as raw r|s concatenated. But OpenSSL expects
   a DER encoded ASN.1 structure.

   Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
   is consolidated with sig to DER conversion as ecdsa_bin2der() and
   moved to xkey_helper.c

   In the past when we used OpenSSL hooks installed by pkcs11-helper,
   such a conversion was not required as it was internally handled by
   the library.

Reported by: Tom 

Signed-off-by: Selva Nair 
Just FYI, this report appeared in the bugzilla for the Fedora packaging. 
 This seems related to this patch.



I will try to prepare a Fedora build with this patch added, for further 
testing.



--
kind regards,

David Sommerseth
OpenVPN Inc




___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

2023-03-10 Thread selva . nair 
From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
  which returns EC signature as raw r|s concatenated. But OpenSSL expects
  a DER encoded ASN.1 structure.

  Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
  is consolidated with sig to DER conversion as ecdsa_bin2der() and
  moved to xkey_helper.c

  In the past when we used OpenSSL hooks installed by pkcs11-helper,
  such a conversion was not required as it was internally handled by
  the library.

Reported by: Tom 

Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c  | 61 +---
 src/openvpn/pkcs11_openssl.c | 23 --
 src/openvpn/xkey_common.h| 15 +
 src/openvpn/xkey_helper.c| 45 ++
 4 files changed, 88 insertions(+), 56 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 136c6ffc..022f53d4 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -146,40 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
 free(cd);
 }
 
-/**
- * Helper to convert ECDSA signature returned by NCryptSignHash
- * to an ECDSA_SIG structure.
- * On entry 'buf[]' of length len contains r and s concatenated.
- * Returns a newly allocated ECDSA_SIG or NULL (on error).
- */
-static ECDSA_SIG *
-ecdsa_bin2sig(unsigned char *buf, int len)
-{
-ECDSA_SIG *ecsig = NULL;
-DWORD rlen = len/2;
-BIGNUM *r = BN_bin2bn(buf, rlen, NULL);
-BIGNUM *s = BN_bin2bn(buf+rlen, rlen, NULL);
-if (!r || !s)
-{
-goto err;
-}
-ecsig = ECDSA_SIG_new(); /* in openssl 1.1 this does not allocate r, s */
-if (!ecsig)
-{
-goto err;
-}
-if (!ECDSA_SIG_set0(ecsig, r, s)) /* ecsig takes ownership of r and s */
-{
-ECDSA_SIG_free(ecsig);
-goto err;
-}
-return ecsig;
-err:
-BN_free(r); /* it is ok to free NULL BN */
-BN_free(s);
-return NULL;
-}
-
 /**
  * Parse a hex string with optional embedded spaces into
  * a byte array.
@@ -287,12 +253,11 @@ static int
 xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, size_t *siglen, const 
unsigned char *tbs,
  size_t tbslen)
 {
-BYTE buf[1024]; /* large enough for EC keys upto 1024 bits */
-DWORD len = _countof(buf);
+DWORD len = *siglen;
 
 msg(D_LOW, "Signing using NCryptSignHash with EC key");
 
-DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
buf, len, , 0);
+DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
sig, len, , 0);
 
 if (status != ERROR_SUCCESS)
 {
@@ -301,26 +266,14 @@ xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, 
size_t *siglen, const unsign
 return 0;
 }
 
-/* NCryptSignHash returns r|s -- convert to OpenSSL's ECDSA_SIG */
-ECDSA_SIG *ecsig = ecdsa_bin2sig(buf, len);
-if (!ecsig)
+/* NCryptSignHash returns r|s -- convert to DER encoded buffer expected by 
OpenSSL */
+int derlen = ecdsa_bin2der(sig, (int) len, *siglen);
+if (derlen <= 0)
 {
-msg(M_NONFATAL, "Error in cryptopicert: Failed to convert ECDSA 
signature");
 return 0;
 }
-
-/* convert internal signature structure 's' to DER encoded byte array in 
sig */
-if (i2d_ECDSA_SIG(ecsig, NULL) > EVP_PKEY_size(cd->pubkey))
-{
-ECDSA_SIG_free(ecsig);
-msg(M_NONFATAL, "Error in cryptoapicert: DER encoded ECDSA signature 
is too long");
-return 0;
-}
-
-*siglen = i2d_ECDSA_SIG(ecsig, );
-ECDSA_SIG_free(ecsig);
-
-return (*siglen > 0);
+*siglen = derlen;
+return 1;
 }
 
 /** Sign hash in tbs using RSA key in cd and NCryptSignHash */
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 5c1de44e..eee86e17 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -168,6 +168,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 
 unsigned char buf[EVP_MAX_MD_SIZE];
 size_t buflen;
+size_t siglen_max = *siglen;
 
 unsigned char enc[EVP_MAX_MD_SIZE + 32]; /* 32 bytes enough for DigestInfo 
header */
 size_t enc_len = sizeof(enc);
@@ -234,8 +235,26 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 ASSERT(0);  /* coding error -- we couldnt have created any such key */
 }
 
-return CKR_OK == pkcs11h_certificate_signAny_ex(cert, ,
-tbs, tbslen, sig, siglen);
+if (CKR_OK != pkcs11h_certificate_signAny_ex(cert, ,
+ tbs, tbslen, sig, siglen))
+{
+return 0;
+}
+if (strcmp(sigalg.keytype, "EC"))
+{
+return 1;
+}
+
+/* For EC keys, pkcs11 returns signature as r|s: convert to der encoded */
+int derlen = ecdsa_bin2der(sig, (int) *siglen, siglen_max);
+
+if (derlen <= 0)
+{
+return 0;
+}
+*siglen = derlen;
+
+return 1;
 }
 
 /* wrapper