[Openvpn-devel,v2,for,2.4] Handle PSS padding in cryptoapicert

Message ID 1564346061-5683-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2,for,2.4] Handle PSS padding in cryptoapicert | expand

Commit Message

Selva Nair July 28, 2019, 10:34 a.m. UTC
From: Selva Nair <selva.nair@gmail.com>

For PSS padding, CNG requires the digest to be signed
and the digest algorithm in use, which are not accessible
via the rsa_sign and rsa_priv_enc callbacks of OpenSSL.
This patch uses the EVP_KEY interface to hook to
evp_pkey_sign callback if OpenSSL version is > 1.1.0.

Mapping of OpenSSL hash algorithm types to CNG is moved
to a function for code-reuse.

To test, both the server and client should be built with
OpenSSL 1.1.1 and use TLS version >= 1.2

Tested on Windows 7 client against a Linux server.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
v2: rebased to release/2.4 after siglen -> *siglen change

Essentially, commits 0cab3475a and ce1c1bee in master
combined and adapted for 2.4.
Required on Windows when built with OpenSSL 1.1.1 and
cryptoapicert is in use against a 1.1.1 peer.

 src/openvpn/cryptoapi.c | 377 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 329 insertions(+), 48 deletions(-)

Comments

Selva Nair Sept. 23, 2019, 3:10 a.m. UTC | #1
Hi,

On Sun, Jul 28, 2019 at 4:34 PM <selva.nair@gmail.com> wrote:
>
> From: Selva Nair <selva.nair@gmail.com>
>
> For PSS padding, CNG requires the digest to be signed
> and the digest algorithm in use, which are not accessible
> via the rsa_sign and rsa_priv_enc callbacks of OpenSSL.
> This patch uses the EVP_KEY interface to hook to
> evp_pkey_sign callback if OpenSSL version is > 1.1.0.
>
> Mapping of OpenSSL hash algorithm types to CNG is moved
> to a function for code-reuse.
>
> To test, both the server and client should be built with
> OpenSSL 1.1.1 and use TLS version >= 1.2
>
> Tested on Windows 7 client against a Linux server.
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> v2: rebased to release/2.4 after siglen -> *siglen change

As this is required for cryptoapicert +  OpenSSL 1.1.1,
nudging for a review and have it included in the next release.

We have already merged this into git master, but the patch here
is slightly different because of context differences and code
refactorings in 2.5.

Thanks,

Selva
Gert Doering Sept. 23, 2019, 9:12 a.m. UTC | #2
Acked-by: Gert Doering <gert@greenie.muc.de>

Sorry for slacking.  

I have stared at the patch a bit, compared to the master patch, and built 
with mingw & openssl 1.0.2n & openssl 1.1.0j on ubuntu 16 (which went fine),
didn't test 1.1.1 as my build system was a bit less than cooperative 
today :-/

This is not a "full review" and I haven't actually *tested* the resulting 
binary, but since the changes are close enough to the two joined commits 
in master and those have been reviewed and tested, "this should be good 
enough".

Your patch has been applied to the release/2.4 branch.

commit 1ed687e72cc1cc46ed1f5f8d9d825db6693e4b3e
Author: Selva Nair
Date:   Sun Jul 28 16:34:21 2019 -0400

     Handle PSS padding in cryptoapicert

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 35a9ebc..7f2c3c0 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -39,6 +39,7 @@ 
 #ifdef ENABLE_CRYPTOAPI
 
 #include <openssl/ssl.h>
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>
@@ -101,6 +102,12 @@  static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { 0, NULL }
 };
 
+/* Global EVP_PKEY_METHOD used to override the sign operation */
+static EVP_PKEY_METHOD *pmethod;
+static int (*default_pkey_sign_init) (EVP_PKEY_CTX *ctx);
+static int (*default_pkey_sign) (EVP_PKEY_CTX *ctx, unsigned char *sig,
+                                 size_t *siglen, const unsigned char *tbs, size_t tbslen);
+
 typedef struct _CAPI_DATA {
     const CERT_CONTEXT *cert_context;
     HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov;
@@ -108,6 +115,80 @@  typedef struct _CAPI_DATA {
     BOOL free_crypt_prov;
 } 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:
+            pad = BCRYPT_PAD_NONE;
+            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 char *
 ms_error_text(DWORD ms_err)
 {
@@ -217,25 +298,44 @@  rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
  * Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
  * key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
  * the length of the signature or 0 on error.
+ * Only RSA is supported and padding should be BCRYPT_PAD_PKCS1 or
+ * BCRYPT_PAD_PSS.
  * If the hash_algo is not NULL, PKCS #1 DigestInfo header gets added
- * to 'from', else it is signed as is.
- * For now we support only RSA and the padding is assumed to be PKCS1 v1.5
+ * to |from|, else it is signed as is. Use NULL for MD5 + SHA1 hash used
+ * in TLS 1.1 and earlier.
+ * In case of PSS padding, |saltlen| should specify the size of salt to use.
+ * If |to| is NULL returns the required buffer size.
  */
 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, DWORD saltlen)
 {
     NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
     DWORD len = 0;
     ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
 
-    msg(D_LOW, "Signing hash using CNG: data size = %d", flen);
-
-    BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
     DWORD status;
 
-    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
-                            to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0);
+    msg(D_LOW, "Signing hash using CNG: data size = %d padding = %lu", flen, padding);
+
+    if (padding == BCRYPT_PAD_PKCS1)
+    {
+        BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
+        status = NCryptSignHash(hkey, &padinfo, (BYTE *)from, flen,
+                                to, tlen, &len, padding);
+    }
+    else if (padding == BCRYPT_PAD_PSS)
+    {
+        BCRYPT_PSS_PADDING_INFO padinfo = {hash_algo, saltlen};
+        status = NCryptSignHash(hkey, &padinfo, (BYTE *)from, flen,
+                                to, tlen, &len, padding);
+    }
+    else
+    {
+        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
+        return 0;
+    }
+
     if (status != ERROR_SUCCESS)
     {
         SetLastError(status);
@@ -261,16 +361,19 @@  rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
         RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
     }
+
+    if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+    {
+        return priv_enc_CNG(cd, NULL, from, flen, to, RSA_size(rsa),
+                            cng_padding_type(padding), 0);
+    }
+
     if (padding != RSA_PKCS1_PADDING)
     {
         /* AFAICS, CryptSignHash() *always* uses PKCS1 padding. */
         RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
         return 0;
     }
-    if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
-    {
-        return priv_enc_CNG(cd, NULL, from, flen, to, RSA_size(rsa), padding);
-    }
 
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
      * be way to straightforward for M$, I guess... So we have to do it this
@@ -333,12 +436,13 @@  rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
     return len;
 }
 
-/*
+/**
  * Sign the hash in |m| and return the signature in |sig|.
  * Returns 1 on success, 0 on error.
  * NCryptSignHash() is used to sign and it is instructed to add the
  * the PKCS #1 DigestInfo header to |m| unless the hash algorithm is
  * the MD5/SHA1 combination used in TLS 1.1 and earlier versions.
+ * OpenSSL exercises this callback only when padding is PKCS1 v1.5.
  */
 static int
 rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len,
@@ -355,44 +459,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), 0);
+
     return (*siglen == 0) ? 0 : 1;
 }
 
@@ -518,6 +594,176 @@  find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
     return rv;
 }
 
+#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
+
+static const CAPI_DATA *
+retrieve_capi_data(EVP_PKEY *pkey)
+{
+    const CAPI_DATA *cd = NULL;
+
+    if (pkey && EVP_PKEY_id(pkey) == EVP_PKEY_RSA)
+    {
+        RSA *rsa = EVP_PKEY_get0_RSA(pkey);
+        if (rsa)
+        {
+            cd = (CAPI_DATA *)RSA_meth_get0_app_data(RSA_get_method(rsa));
+        }
+    }
+    return cd;
+}
+
+static int
+pkey_rsa_sign_init(EVP_PKEY_CTX *ctx)
+{
+    EVP_PKEY *pkey = EVP_PKEY_CTX_get0_pkey(ctx);
+
+    if (pkey && retrieve_capi_data(pkey))
+    {
+        return 1; /* Return success */
+    }
+    else if (default_pkey_sign_init)  /* Not our key. Call the default method */
+    {
+        return default_pkey_sign_init(ctx);
+    }
+    return 1;
+}
+
+/**
+ * Implementation of EVP_PKEY_sign() using CNG: sign the digest in |tbs|
+ * and save the the signature in |sig| and its size in |*siglen|.
+ * If |sig| is NULL the required buffer size is returned in |*siglen|.
+ * Returns 1 on success, 0 or a negative integer on error.
+ */
+static int
+pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, size_t *siglen,
+              const unsigned char *tbs, size_t tbslen)
+{
+    EVP_PKEY *pkey = NULL;
+    const CAPI_DATA *cd = NULL;
+    EVP_MD *md = NULL;
+    const wchar_t *alg = NULL;
+
+    int padding;
+    int hashlen;
+    int saltlen;
+
+    pkey = EVP_PKEY_CTX_get0_pkey(ctx);
+    if (pkey)
+    {
+        cd = retrieve_capi_data(pkey);
+    }
+
+    /*
+     * We intercept all sign requests, not just the one's for our key.
+     * Check the key and call the saved OpenSSL method for unknown keys.
+     */
+    if (!pkey || !cd)
+    {
+        if (default_pkey_sign)
+        {
+            return default_pkey_sign(ctx, sig, siglen, tbs, tbslen);
+        }
+        else  /* This should not happen */
+        {
+            msg(M_FATAL, "cryptopaicert: Unknown key and no default sign operation to fallback on");
+            return -1;
+        }
+    }
+
+    if (!EVP_PKEY_CTX_get_rsa_padding(ctx, &padding))
+    {
+        padding = RSA_PKCS1_PADDING; /* Default padding for RSA */
+    }
+
+    if (EVP_PKEY_CTX_get_signature_md(ctx, &md))
+    {
+        hashlen = EVP_MD_size(md);
+        alg = cng_hash_algo(EVP_MD_type(md));
+
+        /*
+         * alg == NULL indicates legacy MD5+SHA1 hash, else alg should be a valid
+         * digest algorithm.
+         */
+        if (alg && wcscmp(alg, L"UNKNOWN") == 0)
+        {
+            RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE);
+            return -1;
+        }
+    }
+    else
+    {
+        msg(M_NONFATAL, "cryptoapicert: could not determine the signature digest algorithm");
+        RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE);
+        return -1;
+    }
+
+    if (tbslen != (size_t)hashlen)
+    {
+        RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_INVALID_DIGEST_LENGTH);
+        return -1;
+    }
+
+    /* If padding is PSS, determine parameters to pass to CNG */
+    if (padding == RSA_PKCS1_PSS_PADDING)
+    {
+        /*
+         * Ensure the digest type for signature and mask generation match.
+         * In CNG there is no option to specify separate hash functions for
+         * the two, but OpenSSL supports it. However, I have not seen the
+         * two being different in practice. Also the recommended practice is
+         * to use the same for both (rfc 8017 sec 8.1).
+         */
+        EVP_MD *mgf1md;
+        if (!EVP_PKEY_CTX_get_rsa_mgf1_md(ctx, &mgf1md)
+            || EVP_MD_type(mgf1md) != EVP_MD_type(md))
+        {
+            msg(M_NONFATAL, "cryptoapicert: Unknown MGF1 digest type or does"
+                " not match the signature digest type.");
+            RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNSUPPORTED_MASK_PARAMETER);
+        }
+
+        if (!EVP_PKEY_CTX_get_rsa_pss_saltlen(ctx, &saltlen))
+        {
+            msg(M_WARN|M_INFO, "cryptoapicert: unable to get the salt length from context."
+                " Using the default value.");
+            saltlen = -1;
+        }
+
+        /*
+         * In OpenSSL saltlen = -1 indicates to use the size of the digest as
+         * size of the salt. A value of -2 or -3 indicates maximum salt length
+         * that will fit. See RSA_padding_add_PKCS1_PSS_mgf1() of OpenSSL.
+         */
+        if (saltlen == -1)
+        {
+            saltlen = hashlen;
+        }
+        else if (saltlen < 0)
+        {
+            const RSA *rsa = EVP_PKEY_get0_RSA(pkey);
+            saltlen = RSA_size(rsa) - hashlen - 2; /* max salt length for RSASSA-PSS */
+            if (RSA_bits(rsa) &0x7) /* number of bits in the key not a multiple of 8 */
+            {
+                saltlen--;
+            }
+        }
+
+        if (saltlen < 0)
+        {
+            RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE);
+            return -1;
+        }
+        msg(D_LOW, "cryptoapicert: PSS padding using saltlen = %d", saltlen);
+    }
+
+    *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen,
+                           cng_padding_type(padding), (DWORD)saltlen);
+
+    return (*siglen == 0) ? 0 : 1;
+}
+
+#endif /* OPENSSL_VERSION >= 1.1.0 */
+
 int
 SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
 {
@@ -620,10 +866,45 @@  SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     /* For CNG, set the RSA_sign method which gets priority over priv_enc().
      * This method is called with the raw hash without the digestinfo
      * header and works better when using NCryptSignHash() with some tokens.
+     * However, if PSS padding is in use, openssl does not call this
+     * function but adds the padding and then calls rsa_priv_enc()
+     * with padding set to NONE which is not supported by CNG.
+     * So, when posisble (OpenSSL 1.1.0 and up), we hook on to the sign
+     * operation in EVP_PKEY_METHOD struct.
      */
     if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
     {
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L)
         RSA_meth_set_sign(my_rsa_method, rsa_sign_CNG);
+#else
+        /* pmethod is global -- initialize only if NULL */
+        if (!pmethod)
+        {
+            pmethod = EVP_PKEY_meth_new(EVP_PKEY_RSA, 0);
+            if (!pmethod)
+            {
+                SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_FILE, ERR_R_MALLOC_FAILURE);
+                goto err;
+            }
+            const EVP_PKEY_METHOD *default_pmethod = EVP_PKEY_meth_find(EVP_PKEY_RSA);
+            EVP_PKEY_meth_copy(pmethod, default_pmethod);
+
+            /* We want to override only sign_init() and sign() */
+            EVP_PKEY_meth_set_sign(pmethod, pkey_rsa_sign_init, pkey_rsa_sign);
+            EVP_PKEY_meth_add0(pmethod);
+
+            /* Keep a copy of the default sign and sign_init methods */
+
+#if (OPENSSL_VERSION_NUMBER < 0x1010009fL)   /* < version 1.1.0i */
+            /* The function signature is not const-correct in these versions */
+            EVP_PKEY_meth_get_sign((EVP_PKEY_METHOD *)default_pmethod, &default_pkey_sign_init,
+                                   &default_pkey_sign);
+#else
+            EVP_PKEY_meth_get_sign(default_pmethod, &default_pkey_sign_init,
+                                   &default_pkey_sign);
+#endif
+        }
+#endif /* (OPENSSL_VERSION_NUMBER < 0x10100000L) */
     }
 
     rsa = RSA_new();