diff mbox series

[Openvpn-devel,1/2] Move OpenSSL vs CNG signature digest type mapping to a function

Message ID 1544210258-8754-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,1/2] Move OpenSSL vs CNG signature digest type mapping to a function | expand

Commit Message

Selva Nair Dec. 7, 2018, 8:17 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

Also add a function to map  OpenSSL padding identifier to
corresponding CNG constant.

This is to help add support for additional padding
types: only refactoring, no functional changes.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/cryptoapi.c | 120 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 38 deletions(-)

Comments

Arne Schwabe Jan. 22, 2019, 10:56 p.m. UTC | #1 
Am 07.12.18 um 20:17 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Also add a function to map  OpenSSL padding identifier to
> corresponding CNG constant.
> 
> This is to help add support for additional padding
> types: only refactoring, no functional changes.
> 

I have no compile and used tested it but the code looks good.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Arne
Gert Doering Jan. 23, 2019, 8:08 a.m. UTC | #2 
Your patch has been applied to the master branch.

It needed a bit of massaging due to the uncrustify patches to cryptoapi.c
that happened in between (f57431cdc88f2), but since the relevant conflicts
were all "simple whitespace changes", fairly easily solved.

From a quick clance on cryptoapi.c in release/2.4 it looks like this
might also be applicable - but "by default" I won't apply pure refactoring
changes.  So: if you need this for a change that falls into release/2.4
land ("long-term compatibility" or "bugfix"), please let me know.

Test built on ubuntu 16.04/mingw.

commit 0cab3475a83e9bad35b0eeb39b9ca886e6afaf1e
Author: Selva Nair
Date:   Fri Dec 7 14:17:37 2018 -0500

     Move OpenSSL vs CNG signature digest type mapping to a function

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


--
kind regards,

Gert Doering
Selva Nair Jan. 23, 2019, 8:53 a.m. UTC | #3 
Hi,

I noticed the uncrustify changes only this morning when rebasing the next
patch for a v2. Thanks for taking care of that.

This refactoring is used in the following patch 2/2 (PSS padding needed for
OpenSSL 1.1.1) which Arne is reviewing. If 2.5 is far away, we may want to
make 2.4.x work with openssl 1.1.1. That said, we could continue shipping
2.4.x for Windows built against OpenSSL 1.1.0, so I'm fine with no PSS (and
hence no OpenSSL 1.1.1) support in 2.4.

Selva

On Wed, Jan 23, 2019 at 2:08 PM Gert Doering <gert@greenie.muc.de> wrote:

> Your patch has been applied to the master branch.
>
> It needed a bit of massaging due to the uncrustify patches to cryptoapi.c
> that happened in between (f57431cdc88f2), but since the relevant conflicts
> were all "simple whitespace changes", fairly easily solved.
>
> From a quick clance on cryptoapi.c in release/2.4 it looks like this
> might also be applicable - but "by default" I won't apply pure refactoring
> changes.  So: if you need this for a change that falls into release/2.4
> land ("long-term compatibility" or "bugfix"), please let me know.
>
> Test built on ubuntu 16.04/mingw.
>
> commit 0cab3475a83e9bad35b0eeb39b9ca886e6afaf1e
> Author: Selva Nair
> Date:   Fri Dec 7 14:17:37 2018 -0500
>
>      Move OpenSSL vs CNG signature digest type mapping to a function
>
>      Signed-off-by: Selva Nair <selva.nair@gmail.com>
>      Acked-by: Arne Schwabe <arne@rfc2549.org>
>      Message-Id: <1544210258-8754-1-git-send-email-selva.nair@gmail.com>
>      URL:
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17999.html
>      Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
>
> --
> kind regards,
>
> Gert Doering
>
>
<div dir="ltr"><div>Hi,<br><br>I noticed the uncrustify changes only this morning when rebasing the next patch for a v2. Thanks for taking care of that.<br><br>This refactoring is used in the following patch 2/2 (PSS padding needed for OpenSSL 1.1.1) which Arne is reviewing. If 2.5 is far away, we may want to make 2.4.x work with openssl 1.1.1. That said, we could continue shipping 2.4.x for Windows built against OpenSSL 1.1.0, so I&#39;m fine with no PSS (and hence no OpenSSL 1.1.1) support in 2.4.<br><br></div>Selva<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail-m_-2900189953194776465gmail_attr">On Wed, Jan 23, 2019 at 2:08 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Your patch has been applied to the master branch.<br>
<br>
It needed a bit of massaging due to the uncrustify patches to cryptoapi.c<br>
that happened in between (f57431cdc88f2), but since the relevant conflicts<br>
were all &quot;simple whitespace changes&quot;, fairly easily solved.<br>
<br>
From a quick clance on cryptoapi.c in release/2.4 it looks like this<br>
might also be applicable - but &quot;by default&quot; I won&#39;t apply pure refactoring<br>
changes.  So: if you need this for a change that falls into release/2.4<br>
land (&quot;long-term compatibility&quot; or &quot;bugfix&quot;), please let me know.<br>
<br>
Test built on ubuntu 16.04/mingw.<br>
<br>
commit 0cab3475a83e9bad35b0eeb39b9ca886e6afaf1e<br>
Author: Selva Nair<br>
Date:   Fri Dec 7 14:17:37 2018 -0500<br>
<br>
     Move OpenSSL vs CNG signature digest type mapping to a function<br>
<br>
     Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
     Acked-by: Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt;<br>
     Message-Id: &lt;<a href="mailto:1544210258-8754-1-git-send-email-selva.nair@gmail.com" target="_blank">1544210258-8754-1-git-send-email-selva.nair@gmail.com</a>&gt;<br>
     URL: <a href="https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17999.html" rel="noreferrer" target="_blank">https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17999.html</a><br>
     Signed-off-by: Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;<br>
<br>
<br>
--<br>
kind regards,<br>
<br>
Gert Doering<br>
<br>
</blockquote></div>
diff mbox series

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index fa057cb..1ce97f5 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -113,6 +113,77 @@  typedef struct _CAPI_DATA {
     int ref_count;
 } CAPI_DATA;
 
+/* Translate OpenSSL padding type to CNG padding type
+ * Returns 0 for unknown/unsupported padding.
+ */
+static DWORD
+cng_padding_type(int padding)
+{
+    DWORD pad = 0;
+
+    switch (padding)
+    {
+        case RSA_NO_PADDING:
+            break;
+
+        case RSA_PKCS1_PADDING:
+            pad = BCRYPT_PAD_PKCS1;
+            break;
+
+        case RSA_PKCS1_PSS_PADDING:
+            pad = BCRYPT_PAD_PSS;
+            break;
+
+        default:
+            msg(M_WARN|M_INFO, "cryptoapicert: unknown OpenSSL padding type %d.",
+                padding);
+    }
+
+    return pad;
+}
+
+/*
+ * Translate OpenSSL hash OID to CNG algorithm name. Returns
+ * "UNKNOWN" for unsupported algorithms and NULL for MD5+SHA1
+ * mixed hash used in TLS 1.1 and earlier.
+ */
+static const wchar_t *
+cng_hash_algo(int md_type)
+{
+    const wchar_t *alg = L"UNKNOWN";
+    switch (md_type)
+    {
+        case NID_md5:
+            alg = BCRYPT_MD5_ALGORITHM;
+            break;
+
+        case NID_sha1:
+            alg = BCRYPT_SHA1_ALGORITHM;
+            break;
+
+        case NID_sha256:
+            alg = BCRYPT_SHA256_ALGORITHM;
+            break;
+
+        case NID_sha384:
+            alg = BCRYPT_SHA384_ALGORITHM;
+            break;
+
+        case NID_sha512:
+            alg = BCRYPT_SHA512_ALGORITHM;
+            break;
+
+        case NID_md5_sha1:
+        case 0:
+            alg = NULL;
+            break;
+        default:
+            msg(M_WARN|M_INFO, "cryptoapicert: Unknown hash type NID=0x%x", md_type);
+            break;
+    }
+    return alg;
+}
+
 static void
 CAPI_DATA_free(CAPI_DATA *cd)
 {
@@ -253,7 +324,7 @@  rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
  */
 static int
 priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned char *from,
-             int flen, unsigned char *to, int tlen, int padding)
+             int flen, unsigned char *to, int tlen, DWORD padding)
 {
     NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
     DWORD len = 0;
@@ -268,7 +339,7 @@  priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned char
     DWORD status;
 
     status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
-                            to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0);
+                            to, tlen, &len, padding);
     if (status != ERROR_SUCCESS)
     {
         SetLastError(status);
@@ -302,7 +373,8 @@  rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
     }
     if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
     {
-        return priv_enc_CNG(cd, NULL, from, flen, to, RSA_size(rsa), padding);
+        return priv_enc_CNG(cd, NULL, from, flen, to, RSA_size(rsa),
+                            cng_padding_type(padding));
     }
 
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
@@ -388,44 +460,16 @@  rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len,
         return 0;
     }
 
-    switch (type)
+    alg = cng_hash_algo(type);
+    if (alg && wcscmp(alg, L"UNKNOWN") == 0)
     {
-        case NID_md5:
-            alg = BCRYPT_MD5_ALGORITHM;
-            break;
-
-        case NID_sha1:
-            alg = BCRYPT_SHA1_ALGORITHM;
-            break;
-
-        case NID_sha256:
-            alg = BCRYPT_SHA256_ALGORITHM;
-            break;
-
-        case NID_sha384:
-            alg = BCRYPT_SHA384_ALGORITHM;
-            break;
-
-        case NID_sha512:
-            alg = BCRYPT_SHA512_ALGORITHM;
-            break;
-
-        case NID_md5_sha1:
-            if (m_len != SSL_SIG_LENGTH)
-            {
-                RSAerr(RSA_F_RSA_SIGN, RSA_R_INVALID_MESSAGE_LENGTH);
-                return 0;
-            }
-            /* No DigestInfo header is required -- set alg-name to NULL */
-            alg = NULL;
-            break;
-        default:
-            msg(M_WARN, "cryptoapicert: Unknown hash type NID=0x%x", type);
-            RSAerr(RSA_F_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE);
-            return 0;
+        RSAerr(RSA_F_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE);
+        return 0;
     }
 
-    *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), padding);
+    *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa),
+                           cng_padding_type(padding));
+
     return (siglen == 0) ? 0 : 1;
 }