Re: [PATCH v2 3/6] ncrypt: Make use of bcrypt for low level cryptography.

[Hans Leidekker]

On Sun, 2022-03-06 at 12:22 -0300, Santino Mazza wrote:
> Signed-off-by: Santino Mazza
>
> dlls/ncrypt/Makefile.in       |  1 +
>  dlls/ncrypt/main.c            | 98 +++++++++++++----------------------
>  dlls/ncrypt/ncrypt_internal.h | 29 ++---------
>  3 files changed, 40 insertions(+), 88 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

Please use spaces here.
> diff --git a/dlls/ncrypt/main.c b/dlls/ncrypt/main.c
> index 68d9d884618..e82c76103de 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;

I'd say the bcrypt_ prefix is redundant here. It's pretty clear from the
function names that these are bcrypt return values.
> @@ -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;
> -
>    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");
> -
>        ERR("Error importing keypair with bcrypt %#lx\n", ret);
>
> return NTE_BAD_DATA;
>          }

This should be a WARN.
> -
>    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));

This change is not needed.
> -
>    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;
> - }
>
> }

This break will never be reached. 'object' is leaked on error.
> diff --git a/dlls/ncrypt/ncrypt_internal.h b/dlls/ncrypt/ncrypt_internal.h
> index 3966dd73ed6..de5cde5fa8b 100644
> --- a/dlls/ncrypt/ncrypt_internal.h
> +++ b/dlls/ncrypt/ncrypt_internal.h
> @@ -16,34 +16,13 @@
> - 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>

The ncrypt.h include is not needed.