Skip to contentSkip to conversation

Move OpenSSL vs CNG signature digest type mapping to a function

PATCH In review [email protected]... 3 participants 14 messages Dec 7, 2018 – Jan 31, 2019

lore
Share & tools
lore

[email protected]...

author
#0 lore raw

PATCH 1/2 Move OpenSSL vs CNG signature digest type mapping to a function

From: Selva Nair <[email protected]...>

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 <[email protected]...>
---
 src/openvpn/cryptoapi.c | 120 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 38 deletions(-)
src/openvpn/cryptoapi.c +82 -38
index fa057cb..1ce97f5 100644
@@ -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;
 }
Signature · 1 line
2.1.4
author
#1 lore raw

PATCH 2/2 Handle PSS padding in cryptoapicert

From: Selva Nair <[email protected]...>

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.

To test this code path, 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 <[email protected]...>
---
 src/openvpn/cryptoapi.c | 257 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 244 insertions(+), 13 deletions(-)
src/openvpn/cryptoapi.c +244 -13
index 1ce97f5..653b76a 100644
@@ -40,6 +40,7 @@
 #ifdef ENABLE_CRYPTOAPI
 
 #include <openssl/ssl.h>
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>

@@ -105,6 +106,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
 /* index for storing external data in EC_KEY: < 0 means uninitialized */
 static int ec_data_idx = -1;
 
+/* 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;

@@ -318,28 +325,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.
+ * This is used only for RSA 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, DWORD 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);
-
-    /* The hash OID is already in 'from'.  So set the hash algorithm
-     * in the padding info struct to NULL.
-     */
-    BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
     DWORD status;
 
-    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
-                            to, tlen, &len, padding);
+    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);

@@ -365,16 +388,18 @@ 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 (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),
-                            cng_padding_type(padding));
+                            cng_padding_type(padding), 0);
     }
 
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would

@@ -444,6 +469,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
  * 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,

@@ -468,7 +494,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len,
     }
 
     *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa),
-                           cng_padding_type(padding));
+                           cng_padding_type(padding), 0);
 
     return (siglen == 0) ? 0 : 1;
 }

@@ -775,6 +801,175 @@ 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)
+{
+    msg(D_LOW, "cryptoapicert: enter pkey_rsa_sign_init");
+
+    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 value is 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, "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);
+        EVP_MD_size(md);
+    }
+
+    msg(D_LOW, "cryptoapicert: calling priv_enc_CNG with alg = %ls", alg);
+    *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 */
+
 static int
 ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
 {

@@ -796,10 +991,46 @@ ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
     /* 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();
Signature · 1 line
2.1.4
#2 lore raw

Re: [Openvpn-devel] [PATCH 2/2] Handle PSS padding in cryptoapicert

Am 07.12.18 um 20:17 schrieb [email protected]...:

Quoted 8 lines

From: Selva Nair <[email protected]...>

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.

Quoted 4 lines

To test this code path, 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.

Tested on Windows 10 1809 against a Linux server.

Quoted 23 lines

#include <openssl/ssl.h>

src/openvpn/cryptoapi.c +7 -0
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>

@@ -105,6 +106,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
 /* index for storing external data in EC_KEY: < 0 means uninitialized */
 static int ec_data_idx = -1;
 
+/* 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;

@@ -318,28 +325,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.

src/openvpn/cryptoapi.c rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in · line 335

Quoted 10 lines
src/openvpn/cryptoapi.c +6 -2
+ * This is used only for RSA 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.
  */

If you modify the comments anyway, you might consider making them documentation of the functions (start with /**). This is true for all multiline comment and I don't comment further on it.

src/openvpn/cryptoapi.c rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i · line 405

Quoted 2 lines
src/openvpn/cryptoapi.c +0 -0
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would

Not 100% sure but I think for multiline comments the first line should be just /* (empty)

Quoted 21 lines
(quoted diff) +15 -0
@@ -444,6 +469,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
  * 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.
  */
 
+
+    /* 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;

Review rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i

Quoted 10 lines
(quoted diff) +8 -0
+        }
+    }
 
 
+        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)

no space after cast

Quoted 4 lines
src/openvpn/cryptoapi.c +4 -0
+    {
+        RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_INVALID_DIGEST_LENGTH);
+        return -1;
+    }

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 961

Quoted 10 lines
src/openvpn/cryptoapi.c +9 -0
+
 
+
+        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);
+        EVP_MD_size(md);

This call here should not have side effect (unless md is nullptr) and the result is not used.

src/openvpn/cryptoapi.c find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) · line 967

Quoted 6 lines
src/openvpn/cryptoapi.c +6 -0
+    }
+
+    msg(D_LOW, "cryptoapicert: calling priv_enc_CNG with alg = %ls", alg);
+    *siglen = priv_enc_CNG(cd, alg, tbs, (int) tbslen, sig, *siglen,
+                           cng_padding_type(padding), (DWORD) saltlen);
+

Inconsistent casting, we should always use (cast)variable without space I am told.

src/openvpn/cryptoapi.c ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey) · line 1025

Quoted 5 lines
src/openvpn/cryptoapi.c +5 -0
+            /* 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,

Style dictates a (EVP_PKEY_METHOD *)

Overall the code looks good. The overriding of the global RSA method is a bit of a hack but I also do not have any better solution for this. It might break using OpenSSL engines but that is a corner case that I would not worry about here.

Apart from the minor issues this gets an ACK from me.

Arne

author
#3 lore raw

Re: [Openvpn-devel] [PATCH 2/2] Handle PSS padding in cryptoapicert

Hi

Quoted 9 lines · Arne Schwabe

Overall the code looks good. The overriding of the global RSA method is a bit of a hack but I also do not have any better solution for this. It might break using OpenSSL engines but that is a corner case that I would not worry about here.

Apart from the minor issues this gets an ACK from me.

Thanks for taking time to review this. All good comments and v2 is in the next mail.

Selva

author
#4 lore raw

PATCH v2 2/2 Handle PSS padding in cryptoapicert

In reply to [email protected]...

From: Selva Nair <[email protected]...>

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.

To test this code path, 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 <[email protected]...>

---
v2: Changes:

- Meaningless call to EVP_MD_size() removed.
- /** for function doc strings
- Multiline comments: beginning /* and ending */ left blank
- White space in cast as (TYPE)foo and (TYPE *)bar
- Some extra white-space changes suggested by uncrustify

 src/openvpn/cryptoapi.c | 267 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 252 insertions(+), 15 deletions(-)
src/openvpn/cryptoapi.c +252 -15
index 139845b..0c11712 100644
@@ -40,6 +40,7 @@
 #ifdef ENABLE_CRYPTOAPI
 
 #include <openssl/ssl.h>
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>

@@ -105,6 +106,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
 /* index for storing external data in EC_KEY: < 0 means uninitialized */
 static int ec_data_idx = -1;
 
+/* 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;

@@ -177,6 +184,7 @@ cng_hash_algo(int md_type)
         case 0:
             alg = NULL;
             break;
+
         default:
             msg(M_WARN|M_INFO, "cryptoapicert: Unknown hash type NID=0x%x", md_type);
             break;

@@ -320,28 +328,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.
+ * This is used only for RSA 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, DWORD 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);
-
-    /* The hash OID is already in 'from'.  So set the hash algorithm
-     * in the padding info struct to NULL.
-     */
-    BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
     DWORD status;
 
-    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE *)from, flen,
-                            to, tlen, &len, padding);
+    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);

@@ -367,16 +391,18 @@ 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 (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),
-                            cng_padding_type(padding));
+                            cng_padding_type(padding), 0);
     }
 
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would

@@ -440,12 +466,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,

@@ -470,7 +497,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len,
     }
 
     *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa),
-                           cng_padding_type(padding));
+                           cng_padding_type(padding), 0);
 
     return (siglen == 0) ? 0 : 1;
 }

@@ -778,6 +805,179 @@ 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)
+{
+    msg(D_LOW, "cryptoapicert: enter pkey_rsa_sign_init");
+
+    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 value is 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, "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);
+    }
+
+    msg(D_LOW, "cryptoapicert: calling priv_enc_CNG with alg = %ls", alg);
+    *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 */
+
 static int
 ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
 {

@@ -796,13 +996,50 @@ ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
     RSA_meth_set_finish(my_rsa_method, finish);
     RSA_meth_set0_app_data(my_rsa_method, cd);
 
-    /* For CNG, set the RSA_sign method which gets priority over priv_enc().
+    /*
+     * 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();
Signature · 1 line
2.1.4
#5 lore raw

Re: [Openvpn-devel] [PATCH 2/2 v2] Handle PSS padding in cryptoapicert

Am 23.01.19 um 18:48 schrieb [email protected]...:

Quoted 8 lines

From: Selva Nair <[email protected]...>

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.

Quoted 5 lines

To test this code path, 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.

Normally, I not always expect patches to always cleanly apply but in this case I cannot get git to apply this thing.

git am <~/dl/Openvpn-devel-2-2-v2-Handle-PSS-padding-in-cryptoapicert.patch
Applying: Handle PSS padding in cryptoapicert
error: patch failed: src/openvpn/cryptoapi.c:320
error: src/openvpn/cryptoapi.c: patch does not apply
Patch failed at 0001 Handle PSS padding in cryptoapicert
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

git am --3way
Applying: Handle PSS padding in cryptoapicert
error: sha1 information is lacking or useless (src/openvpn/cryptoapi.c).
error: could not build fake ancestor
Patch failed at 0001 Handle PSS padding in cryptoapicert
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

Since Gert/David will also face the same problems later it might be easier to resend this patch in a form that applies to master without manually resolving conflicts.

Arne

author
#6 lore raw

Re: [Openvpn-devel] [PATCH 2/2 v2] Handle PSS padding in cryptoapicert

Quoted 40 lines · Arne Schwabe

Am 23.01.19 um 18:48 schrieb [email protected]...:

Quoted 8 lines

From: Selva Nair <[email protected]...>

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.

Quoted 5 lines

To test this code path, 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.

Normally, I not always expect patches to always cleanly apply but in this case I cannot get git to apply this thing.

git am <~/dl/Openvpn-devel-2-2-v2-Handle-PSS-padding-in-cryptoapicert.patch
Applying: Handle PSS padding in cryptoapicert
error: patch failed: src/openvpn/cryptoapi.c:320
error: src/openvpn/cryptoapi.c: patch does not apply
Patch failed at 0001 Handle PSS padding in cryptoapicert
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

git am --3way
Applying: Handle PSS padding in cryptoapicert
error: sha1 information is lacking or useless (src/openvpn/cryptoapi.c).
error: could not build fake ancestor
Patch failed at 0001 Handle PSS padding in cryptoapicert
hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

Quoted 5 lines

Since Gert/David will also face the same problems later it might be easier to resend this patch in a form that applies to master without manually resolving conflicts.

It seems I had second guessed uncrustify in a context --- a space before "?" added by uncrustify and a space in "(BYTE *) from" which is a sin in our books, but uncrustify doesn't care.
I would have though "git am --ignore-whitespace" would have worked around that, but it doesn't when the mismatch is in the context?

What happens twice happens thrice -- v3 is in the mail..

Selva

author
#7 lore raw

PATCH v3 2/2 Handle PSS padding in cryptoapicert

In reply to [email protected]...

From: Selva Nair <[email protected]...>

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.

To test this code path, 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 <[email protected]...>

---
v2: Changes:

- Meaningless call to EVP_MD_size() removed.
- /** for function doc strings
- Multiline comments: beginning /* and ending */ left blank
- White space in cast as (TYPE)foo and (TYPE *)bar
- Some extra white-space changes suggested by uncrustify

v3 changes
- rebase to current master

 src/openvpn/cryptoapi.c | 267 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 252 insertions(+), 15 deletions(-)
src/openvpn/cryptoapi.c +252 -15
index 55902ae..0c11712 100644
@@ -40,6 +40,7 @@
 #ifdef ENABLE_CRYPTOAPI
 
 #include <openssl/ssl.h>
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>

@@ -105,6 +106,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
 /* index for storing external data in EC_KEY: < 0 means uninitialized */
 static int ec_data_idx = -1;
 
+/* 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;

@@ -177,6 +184,7 @@ cng_hash_algo(int md_type)
         case 0:
             alg = NULL;
             break;
+
         default:
             msg(M_WARN|M_INFO, "cryptoapicert: Unknown hash type NID=0x%x", md_type);
             break;

@@ -320,28 +328,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.
+ * This is used only for RSA 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, DWORD 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);
-
-    /* The hash OID is already in 'from'.  So set the hash algorithm
-     * in the padding info struct to NULL.
-     */
-    BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
     DWORD status;
 
-    status = NCryptSignHash(hkey, padding ? &padinfo : NULL, (BYTE *) from, flen,
-                            to, tlen, &len, padding);
+    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);

@@ -367,16 +391,18 @@ 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 (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),
-                            cng_padding_type(padding));
+                            cng_padding_type(padding), 0);
     }
 
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would

@@ -440,12 +466,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,

@@ -470,7 +497,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len,
     }
 
     *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa),
-                           cng_padding_type(padding));
+                           cng_padding_type(padding), 0);
 
     return (siglen == 0) ? 0 : 1;
 }

@@ -778,6 +805,179 @@ 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)
+{
+    msg(D_LOW, "cryptoapicert: enter pkey_rsa_sign_init");
+
+    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 value is 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, "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);
+    }
+
+    msg(D_LOW, "cryptoapicert: calling priv_enc_CNG with alg = %ls", alg);
+    *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 */
+
 static int
 ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
 {

@@ -796,13 +996,50 @@ ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey)
     RSA_meth_set_finish(my_rsa_method, finish);
     RSA_meth_set0_app_data(my_rsa_method, cd);
 
-    /* For CNG, set the RSA_sign method which gets priority over priv_enc().
+    /*
+     * 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();
Signature · 1 line
2.1.4
Acked-by on v3 2/2
#8 lore raw

Re: [Openvpn-devel] [PATCH 2/2 v3] Handle PSS padding in cryptoapicert

Am 30.01.19 um 16:53 schrieb [email protected]...:

Quoted 8 lines

From: Selva Nair <[email protected]...>

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.

Quoted 6 lines

To test this code path, 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.

ACK. I verified that the changes are almost exclusively white spaces changes and apart from the MD_size change and therefore did not retest it.

So my ACK still stands:

Acked-by Arne Schwabe <[email protected]...>
#9 lore raw

[Openvpn-devel] [PATCH applied] Re: Handle PSS padding in cryptoapicert

In reply to [email protected]...

Your patch has been applied to the master branch.

(Test built on ubuntu 16.04 / mingw, not really tested as such)

commit ce1c1beef1eb9ea776e00861117f72c4a1a6f1f8

Author: Selva Nair
Date:   Wed Jan 30 10:53:20 2019 -0500
     Handle PSS padding in cryptoapicert

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg18188.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

author
#10 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Handle PSS padding in cryptoapicert

Thanks.

So now the question -- do we want to support Windows builds with OpenSSL 1.1.1 in 2.4?

Selva

Quoted 24 lines · Gert Doering

Your patch has been applied to the master branch.

(Test built on ubuntu 16.04 / mingw, not really tested as such)

commit ce1c1beef1eb9ea776e00861117f72c4a1a6f1f8

Author: Selva Nair
Date:   Wed Jan 30 10:53:20 2019 -0500
     Handle PSS padding in cryptoapicert

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL:
https://www.mail-archive.com/[email protected]/msg18188.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#12 lore raw
In reply to [email protected]...

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 <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17999.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

author
#13 lore raw

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

Quoted 33 lines · Gert Doering

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 <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL:
https://www.mail-archive.com/[email protected]/msg17999.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering