[PATCH 2/6] ncrypt: Make use of bcrypt for low level cryptography.
Santino Mazza
mazzasantino1206 at gmail.com
Thu Mar 3 10:54:45 CST 2022
On 2/3/22 06:27, Hans Leidekker wrote:
> On Tue, 2022-03-01 at 19:34 -0300, Santino Mazza wrote:
> Signed-off-by: Santino Mazza <mazzasantino1206 at gmail.com>
> ---
> dlls/ncrypt/Makefile.in | 1 +
> dlls/ncrypt/main.c | 101 +++++++++++++---------------------
> dlls/ncrypt/ncrypt_internal.h | 30 ++--------
> dlls/ncrypt/tests/ncrypt.c | 2 +
> 4 files changed, 45 insertions(+), 89 deletions(-)
>
> diff --git a/dlls/ncrypt/Makefile.in b/dlls/ncrypt/Makefile.in
> index ad3ed409961..58ec6b3456b 100644
> --- a/dlls/ncrypt/Makefile.in
> +++ b/dlls/ncrypt/Makefile.in
> @@ -1,5 +1,6 @@
> IMPORTLIB = ncrypt
> MODULE = ncrypt.dll
> +IMPORTS = bcrypt
>
>
> EXTRADLLFLAGS = -Wb,--prefer-native
>
>
> diff --git a/dlls/ncrypt/main.c b/dlls/ncrypt/main.c
> index 68d9d884618..2804708f10f 100644
> --- a/dlls/ncrypt/main.c
> +++ b/dlls/ncrypt/main.c
> @@ -88,20 +88,14 @@ SECURITY_STATUS WINAPI NCryptFreeBuffer(PVOID buf)
>
>
> static SECURITY_STATUS free_key_object(struct key *key)
> {
> - switch (key->alg)
> - {
> - case RSA:
> - {
> - free(key->rsa.modulus);
> - free(key->rsa.public_exp);
> - free(key->rsa.prime1);
> - free(key->rsa.prime2);
> - break;
> - }
> - default:
> - WARN("invalid key %p\n", key);
> - return NTE_INVALID_HANDLE;
> - }
> + NTSTATUS bcrypt_ret;
> +
> + bcrypt_ret = BCryptDestroyKey(key->bcrypt_key);
> + if(bcrypt_ret != ERROR_SUCCESS) return NTE_INVALID_HANDLE;
> + bcrypt_ret = BCryptCloseAlgorithmProvider(key->alg_prov, 0);
> + if(bcrypt_ret != ERROR_SUCCESS) return NTE_INVALID_HANDLE;
> +
>
> There's no need to check for failure here. If BCryptDestroyKey() would fail you
> would leak the algorithm handle.
>
> return ERROR_SUCCESS;
> }
>
>
> @@ -246,6 +240,7 @@ SECURITY_STATUS WINAPI NCryptImportKey(NCRYPT_PROV_HANDLE provider, NCRYPT_KEY_H
> BYTE *data, DWORD datasize, DWORD flags)
> {
> BCRYPT_KEY_BLOB *header = (BCRYPT_KEY_BLOB *)data;
> + struct object *key_object;
>
>
> TRACE("(%#Ix, %#Ix, %s, %p, %p, %p, %lu, %#lx): stub\n", provider, decrypt_key, wine_dbgstr_w(type),
> params, handle, data, datasize, flags);
> @@ -270,70 +265,48 @@ SECURITY_STATUS WINAPI NCryptImportKey(NCRYPT_PROV_HANDLE provider, NCRYPT_KEY_H
> return NTE_BAD_FLAGS;
> }
>
>
> - switch (header->Magic)
> + if(!(key_object = allocate_object(KEY)))
> + {
> + ERR("Error allocating memory\n");
> + return NTE_NO_MEMORY;
> + }
> +
> + key_object->key.storage_prov = provider;
> + switch(header->Magic)
> {
> + case BCRYPT_RSAFULLPRIVATE_MAGIC:
> + case BCRYPT_RSAPRIVATE_MAGIC:
> case BCRYPT_RSAPUBLIC_MAGIC:
> {
> - DWORD expected_size;
> - struct object *object;
> - struct key *key;
> - BYTE *public_exp, *modulus;
> - BCRYPT_RSAKEY_BLOB *rsaheader = (BCRYPT_RSAKEY_BLOB *)data;
> -
> - if (datasize < sizeof(*rsaheader))
> + NTSTATUS ret;
> + BCRYPT_RSAKEY_BLOB *rsablob = (BCRYPT_RSAKEY_BLOB *)data;
> + ret = BCryptOpenAlgorithmProvider(&key_object->key.alg_prov, BCRYPT_RSA_ALGORITHM, NULL, 0);
> + if(ret != ERROR_SUCCESS)
> {
> - ERR("Invalid buffer size.\n");
> - return NTE_BAD_DATA;
> + ERR("Error opening algorithm provider\n");
> + return NTE_INTERNAL_ERROR;
>
> Should we return the error from BCryptOpenAlgorithmProvider() here?
>
> }
> -
> - expected_size = sizeof(*rsaheader) + rsaheader->cbPublicExp + rsaheader->cbModulus;
> - if (datasize != expected_size)
> + ret = BCryptImportKeyPair(key_object->key.alg_prov, NULL, type, &key_object->key.bcrypt_key, data, datasize, 0);
> + if(ret != ERROR_SUCCESS)
> {
> - ERR("Invalid buffer size.\n");
> - return NTE_BAD_DATA;
> + ERR("Error importing keypair with bcrypt %#lx\n", ret);
> + return NTE_INTERNAL_ERROR;
>
> And return the error from BCryptImportKeyPair() here?
>
> }
>
>
> - if (!(object = allocate_object(KEY)))
> - {
> - ERR("Error allocating memory.\n");
> - return NTE_NO_MEMORY;
> - }
> -
> - key = &object->key;
> - key->alg = RSA;
> - key->rsa.bit_length = rsaheader->BitLength;
> - key->rsa.public_exp_size = rsaheader->cbPublicExp;
> - key->rsa.modulus_size = rsaheader->cbModulus;
> - if (!(key->rsa.public_exp = malloc(rsaheader->cbPublicExp)))
> - {
> - ERR("Error allocating memory.\n");
> - free(object);
> - return NTE_NO_MEMORY;
> - }
> - if (!(key->rsa.modulus = malloc(rsaheader->cbModulus)))
> - {
> - ERR("Error allocating memory.\n");
> - free(key->rsa.public_exp);
> - free(object);
> - return NTE_NO_MEMORY;
> - }
> -
> - public_exp = &data[sizeof(*rsaheader)]; /* The public exp is after the header. */
> - modulus = &public_exp[rsaheader->cbPublicExp]; /* The modulus is after the public exponent. */
> - memcpy(key->rsa.public_exp, public_exp, rsaheader->cbPublicExp);
> - memcpy(key->rsa.modulus, modulus, rsaheader->cbModulus);
> -
> - set_object_property(object, NCRYPT_ALGORITHM_GROUP_PROPERTY, (BYTE *)L"RSA", sizeof(L"RSA"));
> - set_object_property(object, NCRYPT_LENGTH_PROPERTY, (BYTE *)&key->rsa.bit_length, sizeof(key->rsa.bit_length));
> - set_object_property(object, NCRYPT_PROVIDER_HANDLE_PROPERTY, (BYTE *)&provider, sizeof(provider));
> - *handle = (NCRYPT_KEY_HANDLE)object;
> + set_object_property(key_object, NCRYPT_PROVIDER_HANDLE_PROPERTY, (BYTE *)&provider, sizeof(NCRYPT_PROV_HANDLE));
> + set_object_property(key_object, NCRYPT_ALGORITHM_GROUP_PROPERTY, (BYTE *)BCRYPT_RSA_ALGORITHM, sizeof(BCRYPT_RSA_ALGORITHM));
> + set_object_property(key_object, NCRYPT_LENGTH_PROPERTY, (BYTE *)&rsablob->BitLength, sizeof(rsablob->BitLength));
> break;
> }
> default:
> - FIXME("Unhandled key magic %#lx.\n", header->Magic);
> + {
> + FIXME("Unhandled key magic %#lx\n", header->Magic);
> return NTE_INVALID_PARAMETER;
> + break;
> + }
> }
>
>
> + *handle = (NCRYPT_KEY_HANDLE)key_object;
> return ERROR_SUCCESS;
> }
>
>
> diff --git a/dlls/ncrypt/ncrypt_internal.h b/dlls/ncrypt/ncrypt_internal.h
> index 3966dd73ed6..2d916d4fbd8 100644
> --- a/dlls/ncrypt/ncrypt_internal.h
> +++ b/dlls/ncrypt/ncrypt_internal.h
> @@ -16,34 +16,14 @@
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> */
>
>
> -enum key_algorithm
> -{
> - DH,
> - DSA,
> - ECC,
> - RSA,
> -};
> -
> -struct rsa_key
> -{
> - DWORD bit_length;
> - DWORD public_exp_size;
> - BYTE *public_exp;
> - DWORD modulus_size;
> - BYTE *modulus;
> - DWORD prime1_size;
> - BYTE *prime1;
> - DWORD prime2_size;
> - BYTE *prime2;
> -};
> +#include <ncrypt.h>
> +#include <bcrypt.h>
>
>
> struct key
> {
> - enum key_algorithm alg;
> - union
> - {
> - struct rsa_key rsa;
> - };
> + NCRYPT_PROV_HANDLE storage_prov;
> + BCRYPT_ALG_HANDLE alg_prov;
> + BCRYPT_KEY_HANDLE bcrypt_key;
> };
>
>
> struct storage_provider
> diff --git a/dlls/ncrypt/tests/ncrypt.c b/dlls/ncrypt/tests/ncrypt.c
> index d1bcbbcab2a..8080e465527 100644
> --- a/dlls/ncrypt/tests/ncrypt.c
> +++ b/dlls/ncrypt/tests/ncrypt.c
> @@ -123,6 +123,7 @@ static void test_key_import_rsa(void)
> ok(key, "got null handle\n");
> NCryptFreeObject(key);
>
>
> + todo_wine{
> ret = NCryptImportKey(prov, 0, BCRYPT_PUBLIC_KEY_BLOB, NULL, &key, rsa_key_blob_with_invalid_publicexp_size,
> sizeof(rsa_key_blob_with_invalid_publicexp_size), 0);
> ok(ret == NTE_BAD_DATA, "got %#lx\n", ret);
> @@ -132,6 +133,7 @@ static void test_key_import_rsa(void)
>
>
> ret = NCryptImportKey(prov, 0, BCRYPT_PUBLIC_KEY_BLOB, NULL, &key, rsa_key_blob, sizeof(rsa_key_blob) + 1, 0);
> ok(ret == NTE_BAD_DATA, "got %#lx\n", ret);
> + }
>
>
> NCryptFreeObject(prov);
> }
>
> You should restructure your patches so that you don't have to reintroduce todo_wine statements.
>
>
Hello Hans, thanks for your review. About returning the errors of bcrypt
from a ncrypt function (Like the calls to BCryptOpenAlgorithmProvider()
from NCryptImportKey), I try to prevent doing that because both
functions return two different types of errors, ncrypt returns
SECURITY_STATUS and bcrypt returns NTSTATUS.
More information about the wine-devel
mailing list