From 1eac8fa15eb5fa68fc3a1c09a05d640ea1f84b2f Mon Sep 17 00:00:00 2001 From: akallabeth Date: Mon, 28 Oct 2024 11:47:58 +0100 Subject: [PATCH] [winpr,crypto] deprecate winpr_Cipher_New Deprecate function in favor of new winpr_CipherNewEx with explict key and IV length parameters, adjust test cases to test both. --- winpr/include/winpr/custom-crypto.h | 29 +++- winpr/libwinpr/crypto/cipher.c | 162 ++++++++++++------ winpr/libwinpr/crypto/crypto.c | 10 +- winpr/libwinpr/crypto/test/TestCryptoCipher.c | 20 ++- 4 files changed, 153 insertions(+), 68 deletions(-) diff --git a/winpr/include/winpr/custom-crypto.h b/winpr/include/winpr/custom-crypto.h index cc1c37332..e1d779ff7 100644 --- a/winpr/include/winpr/custom-crypto.h +++ b/winpr/include/winpr/custom-crypto.h @@ -265,9 +265,32 @@ extern "C" WINPR_API void winpr_Cipher_Free(WINPR_CIPHER_CTX* ctx); WINPR_ATTR_MALLOC(winpr_Cipher_Free, 1) - WINPR_API WINPR_CIPHER_CTX* winpr_Cipher_New(WINPR_CIPHER_TYPE cipher, - WINPR_CRYPTO_OPERATION op, const void* key, - const void* iv); + WINPR_API WINPR_DEPRECATED_VAR("[since 3.10.0] use winpr_Cipher_NewEx", + WINPR_CIPHER_CTX* winpr_Cipher_New(WINPR_CIPHER_TYPE cipher, + WINPR_CRYPTO_OPERATION op, + const void* key, + const void* iv)); + + /** @brief Create a new \b WINPR_CIPHER_CTX + * + * creates a new stream cipher. Only the ciphers supported by your SSL library are available, + * fallback to WITH_INTERNAL_RC4 is not possible. + * + * @param cipher The cipher to create the context for + * @param op Operation \b WINPR_ENCRYPT or \b WINPR_DECRYPT + * @param key A pointer to the key material (size must match expectations for the cipher used) + * @param keylen The length in bytes of key material + * @param iv A pointer to the IV material (size must match expectations for the cipher used) + * @param ivlen The length in bytes of the IV + * + * @return A newly allocated context or \b NULL + * + * @since version 3.10.0 + */ + WINPR_ATTR_MALLOC(winpr_Cipher_Free, 1) + WINPR_API WINPR_CIPHER_CTX* winpr_Cipher_NewEx(WINPR_CIPHER_TYPE cipher, + WINPR_CRYPTO_OPERATION op, const void* key, + size_t keylen, const void* iv, size_t ivlen); WINPR_API BOOL winpr_Cipher_SetPadding(WINPR_CIPHER_CTX* ctx, BOOL enabled); WINPR_API BOOL winpr_Cipher_Update(WINPR_CIPHER_CTX* ctx, const void* input, size_t ilen, void* output, size_t* olen); diff --git a/winpr/libwinpr/crypto/cipher.c b/winpr/libwinpr/crypto/cipher.c index ee3df9c30..64f63f638 100644 --- a/winpr/libwinpr/crypto/cipher.c +++ b/winpr/libwinpr/crypto/cipher.c @@ -47,6 +47,19 @@ #endif #endif +struct winpr_cipher_ctx_private_st +{ + WINPR_CIPHER_TYPE cipher; + WINPR_CRYPTO_OPERATION op; + +#ifdef WITH_OPENSSL + EVP_CIPHER_CTX* ectx; +#endif +#ifdef WITH_MBEDTLS + mbedtls_cipher_context_t* mctx; +#endif +}; + /** * RC4 */ @@ -562,69 +575,92 @@ mbedtls_cipher_type_t winpr_mbedtls_get_cipher_type(int cipher) WINPR_CIPHER_CTX* winpr_Cipher_New(WINPR_CIPHER_TYPE cipher, WINPR_CRYPTO_OPERATION op, const void* key, const void* iv) { - WINPR_CIPHER_CTX* ctx = NULL; -#if defined(WITH_OPENSSL) - int operation = 0; - const EVP_CIPHER* evp = NULL; - EVP_CIPHER_CTX* octx = NULL; + return winpr_Cipher_NewEx(cipher, op, key, 0, iv, 0); +} - if (!(evp = winpr_openssl_get_evp_cipher(cipher))) - return NULL; - - if (!(octx = EVP_CIPHER_CTX_new())) - return NULL; - - operation = (op == WINPR_ENCRYPT) ? 1 : 0; - - if (EVP_CipherInit_ex(octx, evp, NULL, key, iv, operation) != 1) +WINPR_API WINPR_CIPHER_CTX* winpr_Cipher_NewEx(WINPR_CIPHER_TYPE cipher, WINPR_CRYPTO_OPERATION op, + const void* key, size_t keylen, const void* iv, + size_t ivlen) +{ + if (cipher == WINPR_CIPHER_ARC4_128) { - EVP_CIPHER_CTX_free(octx); + WLog_ERR(TAG, + "WINPR_CIPHER_ARC4_128 (RC4) cipher not supported, use winpr_RC4_new instead"); return NULL; } - EVP_CIPHER_CTX_set_padding(octx, 0); - ctx = (WINPR_CIPHER_CTX*)octx; + WINPR_CIPHER_CTX* ctx = calloc(1, sizeof(WINPR_CIPHER_CTX)); + if (!ctx) + return NULL; + + ctx->cipher = cipher; + ctx->op = op; + +#if defined(WITH_OPENSSL) + const EVP_CIPHER* evp = winpr_openssl_get_evp_cipher(cipher); + if (!evp) + goto fail; + + ctx->ectx = EVP_CIPHER_CTX_new(); + if (!ctx->ectx) + goto fail; + + if (keylen != 0) + { + WINPR_ASSERT(keylen <= INT32_MAX); + const int len = EVP_CIPHER_CTX_key_length(ctx->ectx); + if ((len > 0) && (len != keylen)) + { + if (EVP_CIPHER_CTX_set_key_length(ctx->ectx, (int)keylen) != 1) + goto fail; + } + } + + if (ivlen != 0) + { + WINPR_ASSERT(ivlen <= INT32_MAX); + const int len = EVP_CIPHER_CTX_iv_length(ctx->ectx); + if ((len > 0) && (ivlen != len)) + goto fail; + } + const int operation = (op == WINPR_ENCRYPT) ? 1 : 0; + + if (EVP_CipherInit_ex(ctx->ectx, evp, NULL, key, iv, operation) != 1) + goto fail; + + EVP_CIPHER_CTX_set_padding(ctx->ectx, 0); + #elif defined(WITH_MBEDTLS) - int key_bitlen; - mbedtls_operation_t operation; - mbedtls_cipher_context_t* mctx; mbedtls_cipher_type_t cipher_type = winpr_mbedtls_get_cipher_type(cipher); const mbedtls_cipher_info_t* cipher_info = mbedtls_cipher_info_from_type(cipher_type); if (!cipher_info) - return NULL; + goto fail; - if (!(mctx = (mbedtls_cipher_context_t*)calloc(1, sizeof(mbedtls_cipher_context_t)))) - return NULL; + ctx->mctx = calloc(1, sizeof(mbedtls_cipher_context_t)); + if (!ctx->mctx) + goto fail; - operation = (op == WINPR_ENCRYPT) ? MBEDTLS_ENCRYPT : MBEDTLS_DECRYPT; - mbedtls_cipher_init(mctx); + const mbedtls_operation_t operation = (op == WINPR_ENCRYPT) ? MBEDTLS_ENCRYPT : MBEDTLS_DECRYPT; + mbedtls_cipher_init(ctx->mctx); - if (mbedtls_cipher_setup(mctx, cipher_info) != 0) - { - free(mctx); - return NULL; - } + if (mbedtls_cipher_setup(ctx->mctx, cipher_info) != 0) + goto fail; - key_bitlen = mbedtls_cipher_get_key_bitlen(mctx); + const int key_bitlen = mbedtls_cipher_get_key_bitlen(ctx->mctx); - if (mbedtls_cipher_setkey(mctx, key, key_bitlen, operation) != 0) - { - mbedtls_cipher_free(mctx); - free(mctx); - return NULL; - } + if (mbedtls_cipher_setkey(ctx->mctx, key, key_bitlen, operation) != 0) + goto fail; - if (mbedtls_cipher_set_padding_mode(mctx, MBEDTLS_PADDING_NONE) != 0) - { - mbedtls_cipher_free(mctx); - free(mctx); - return NULL; - } + if (mbedtls_cipher_set_padding_mode(ctx->mctx, MBEDTLS_PADDING_NONE) != 0) + goto fail; - ctx = (WINPR_CIPHER_CTX*)mctx; #endif return ctx; + +fail: + winpr_Cipher_Free(ctx); + return NULL; } BOOL winpr_Cipher_SetPadding(WINPR_CIPHER_CTX* ctx, BOOL enabled) @@ -632,7 +668,9 @@ BOOL winpr_Cipher_SetPadding(WINPR_CIPHER_CTX* ctx, BOOL enabled) WINPR_ASSERT(ctx); #if defined(WITH_OPENSSL) - EVP_CIPHER_CTX_set_padding((EVP_CIPHER_CTX*)ctx, enabled); + if (!ctx->ectx) + return FALSE; + EVP_CIPHER_CTX_set_padding(ctx->ectx, enabled); #elif defined(WITH_MBEDTLS) mbedtls_cipher_padding_t option = enabled ? MBEDTLS_PADDING_PKCS7 : MBEDTLS_PADDING_NONE; if (mbedtls_cipher_set_padding_mode((mbedtls_cipher_context_t*)ctx, option) != 0) @@ -646,6 +684,9 @@ BOOL winpr_Cipher_SetPadding(WINPR_CIPHER_CTX* ctx, BOOL enabled) BOOL winpr_Cipher_Update(WINPR_CIPHER_CTX* ctx, const void* input, size_t ilen, void* output, size_t* olen) { + WINPR_ASSERT(ctx); + WINPR_ASSERT(olen); + #if defined(WITH_OPENSSL) int outl = (int)*olen; @@ -655,16 +696,16 @@ BOOL winpr_Cipher_Update(WINPR_CIPHER_CTX* ctx, const void* input, size_t ilen, return FALSE; } - WINPR_ASSERT(ctx); - if (EVP_CipherUpdate((EVP_CIPHER_CTX*)ctx, output, &outl, input, (int)ilen) == 1) + WINPR_ASSERT(ctx->ectx); + if (EVP_CipherUpdate(ctx->ectx, output, &outl, input, (int)ilen) == 1) { *olen = (size_t)outl; return TRUE; } #elif defined(WITH_MBEDTLS) - - if (mbedtls_cipher_update((mbedtls_cipher_context_t*)ctx, input, ilen, output, olen) == 0) + WINPR_ASSERT(ctx->mctx); + if (mbedtls_cipher_update(ctx->mctx, input, ilen, output, olen) == 0) return TRUE; #endif @@ -675,10 +716,13 @@ BOOL winpr_Cipher_Update(WINPR_CIPHER_CTX* ctx, const void* input, size_t ilen, BOOL winpr_Cipher_Final(WINPR_CIPHER_CTX* ctx, void* output, size_t* olen) { + WINPR_ASSERT(ctx); + #if defined(WITH_OPENSSL) int outl = (int)*olen; - if (EVP_CipherFinal_ex((EVP_CIPHER_CTX*)ctx, output, &outl) == 1) + WINPR_ASSERT(ctx->ectx); + if (EVP_CipherFinal_ex(ctx->ectx, output, &outl) == 1) { *olen = (size_t)outl; return TRUE; @@ -686,10 +730,12 @@ BOOL winpr_Cipher_Final(WINPR_CIPHER_CTX* ctx, void* output, size_t* olen) #elif defined(WITH_MBEDTLS) - if (mbedtls_cipher_finish((mbedtls_cipher_context_t*)ctx, output, olen) == 0) + WINPR_ASSERT(ctx->mctx); + if (mbedtls_cipher_finish(ctx->mctx, output, olen) == 0) return TRUE; #endif + return FALSE; } @@ -699,11 +745,17 @@ void winpr_Cipher_Free(WINPR_CIPHER_CTX* ctx) return; #if defined(WITH_OPENSSL) - EVP_CIPHER_CTX_free((EVP_CIPHER_CTX*)ctx); + if (ctx->ectx) + EVP_CIPHER_CTX_free(ctx->ectx); #elif defined(WITH_MBEDTLS) - mbedtls_cipher_free((mbedtls_cipher_context_t*)ctx); - free(ctx); + if (ctx->mctx) + { + mbedtls_cipher_free(ctx->mctx); + free(ctx->mctx); + } #endif + + free(ctx); } /** @@ -789,7 +841,7 @@ int winpr_Cipher_BytesToKey(int cipher, WINPR_MD_TYPE md, const void* salt, cons goto err; } - i = 0; + unsigned int i = 0; if (nkey) { diff --git a/winpr/libwinpr/crypto/crypto.c b/winpr/libwinpr/crypto/crypto.c index 26d371fa1..9a0cc8773 100644 --- a/winpr/libwinpr/crypto/crypto.c +++ b/winpr/libwinpr/crypto/crypto.c @@ -186,8 +186,9 @@ BOOL CryptProtectMemory(LPVOID pData, DWORD cbData, DWORD dwFlags) if (!pCipherText) goto out; - if ((enc = winpr_Cipher_New(WINPR_CIPHER_AES_256_CBC, WINPR_ENCRYPT, pMemBlock->key, - pMemBlock->iv)) == NULL) + if ((enc = winpr_Cipher_NewEx(WINPR_CIPHER_AES_256_CBC, WINPR_ENCRYPT, pMemBlock->key, + sizeof(pMemBlock->key), pMemBlock->iv, sizeof(pMemBlock->iv))) == + NULL) goto out; if (!winpr_Cipher_Update(enc, pMemBlock->pData, pMemBlock->cbData, pCipherText, &cbOut)) goto out; @@ -234,8 +235,9 @@ BOOL CryptUnprotectMemory(LPVOID pData, DWORD cbData, DWORD dwFlags) if (!pPlainText) goto out; - if ((dec = winpr_Cipher_New(WINPR_CIPHER_AES_256_CBC, WINPR_DECRYPT, pMemBlock->key, - pMemBlock->iv)) == NULL) + if ((dec = winpr_Cipher_NewEx(WINPR_CIPHER_AES_256_CBC, WINPR_DECRYPT, pMemBlock->key, + sizeof(pMemBlock->key), pMemBlock->iv, sizeof(pMemBlock->iv))) == + NULL) goto out; if (!winpr_Cipher_Update(dec, pMemBlock->pData, pMemBlock->cbData, pPlainText, &cbOut)) goto out; diff --git a/winpr/libwinpr/crypto/test/TestCryptoCipher.c b/winpr/libwinpr/crypto/test/TestCryptoCipher.c index da58eec6e..afa661ddc 100644 --- a/winpr/libwinpr/crypto/test/TestCryptoCipher.c +++ b/winpr/libwinpr/crypto/test/TestCryptoCipher.c @@ -4,12 +4,11 @@ #include #include -static BOOL test_crypto_cipher_aes_128_cbc(void) +static BOOL test_crypto_cipher_aes_128_cbc(BOOL ex) { - WINPR_CIPHER_CTX* ctx = NULL; BOOL result = FALSE; - BYTE key[] = "0123456789abcdeF"; - BYTE iv[] = "1234567887654321"; + BYTE key[16] = "0123456789abcdeF"; + BYTE iv[16] = "1234567887654321"; BYTE ibuf[1024] = { 0 }; BYTE obuf[1024] = { 0 }; size_t ilen = 0; @@ -20,7 +19,13 @@ static BOOL test_crypto_cipher_aes_128_cbc(void) /* encrypt */ - if (!(ctx = winpr_Cipher_New(WINPR_CIPHER_AES_128_CBC, WINPR_ENCRYPT, key, iv))) + WINPR_CIPHER_CTX* ctx = NULL; + if (ex) + ctx = winpr_Cipher_NewEx(WINPR_CIPHER_AES_128_CBC, WINPR_ENCRYPT, key, sizeof(key), iv, + sizeof(iv)); + else + ctx = winpr_Cipher_New(WINPR_CIPHER_AES_128_CBC, WINPR_ENCRYPT, key, iv); + if (!ctx) { (void)fprintf(stderr, "%s: winpr_Cipher_New (encrypt) failed\n", __func__); return FALSE; @@ -219,7 +224,10 @@ int TestCryptoCipher(int argc, char* argv[]) winpr_InitializeSSL(WINPR_SSL_INIT_DEFAULT); - if (!test_crypto_cipher_aes_128_cbc()) + if (!test_crypto_cipher_aes_128_cbc(TRUE)) + return -1; + + if (!test_crypto_cipher_aes_128_cbc(FALSE)) return -1; if (!test_crypto_cipher_rc4())