Am 07.12.18 um 20:17 schrieb selva.n...@gmail.com: > From: Selva Nair <selva.n...@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. > > 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.
>
> #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.
> */
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.
>
> /* 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)
> @@ -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;
> + }
> + }
> + 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
> + {
> + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_INVALID_DIGEST_LENGTH);
> + return -1;
> + }
> +
> +
> + 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.
> + }
> +
> + 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.
> + /* 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
