Skip to content

Commit

Permalink
fix AES-related code, in both crypto and TLS layers, for various unin…
Browse files Browse the repository at this point in the history
…itialized data and resource leak defects around wc_AesInit() and wc_AesFree():

* followup to wolfSSL#7009 "20231128-misc-fixes" and  wolfSSL#7011 "Add missing wc_AesInit calls."

* adds WC_DEBUG_CIPHER_LIFECYCLE, which embeds asserts in low-level AES implementations for proper usage of wc_AesInit() and wc_AesFree().

* fixes native CMAC, AES-EAX, and AES-XTS implementations to assure resource release.

* adds missing wc_AesXtsInit() API, and adds a new wc_AesXtsSetKey_NoInit().

* fixes misspellings in EVP that unconditionally gated out AES-OFB and AES-XTS.

* fixes misspellings in EVP that unconditionally gated out AES-CBC and AES-CFB code in wolfSSL_EVP_CIPHER_CTX_cleanup_cipher().

* openssl compat AES low level cipher API has no counterpart to wc_AesFree(), so these compat APIs will now be gated out in configurations where they would otherwise leak memory or file descriptors (WOLFSSL_AFALG, WOLFSSL_DEVCRYPTO, WOLF_CRYPTO_CB, etc.).  A new macro, WC_AESFREE_IS_MANDATORY, is defined in wolfcrypt/aes.h to streamline this dependency.

* fixes 40 missing EVP_CIPHER_CTX_cleanup()s and 11 wc_AesFree()s in src/ssl.c, src/ssl_crypto.c, tests/api.c, and wolfcrypt/test/test.c.
  • Loading branch information
douzzer committed Dec 5, 2023
1 parent 7753e3d commit 689a82a
Show file tree
Hide file tree
Showing 16 changed files with 898 additions and 218 deletions.
1 change: 1 addition & 0 deletions src/quic.c
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,7 @@ size_t wolfSSL_quic_get_aead_tag_len(const WOLFSSL_EVP_CIPHER* aead_cipher)
ret = 0;
}

(void)wolfSSL_EVP_CIPHER_CTX_cleanup(ctx);
#ifdef WOLFSSL_SMALL_STACK
XFREE(ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
#endif
Expand Down
2 changes: 2 additions & 0 deletions src/ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -29665,6 +29665,8 @@ static int wolfSSL_TicketKeyCb(WOLFSSL* ssl,
end:

(void)wc_HmacFree(&hmacCtx.hmac);
(void)wolfSSL_EVP_CIPHER_CTX_cleanup(evpCtx);

#ifdef WOLFSSL_SMALL_STACK
XFREE(evpCtx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER);
#endif
Expand Down
38 changes: 27 additions & 11 deletions src/ssl_crypto.c
Original file line number Diff line number Diff line change
Expand Up @@ -2079,14 +2079,10 @@ WOLFSSL_CMAC_CTX* wolfSSL_CMAC_CTX_new(void)
ctx = (WOLFSSL_CMAC_CTX*)XMALLOC(sizeof(WOLFSSL_CMAC_CTX), NULL,
DYNAMIC_TYPE_OPENSSL);
if (ctx != NULL) {
/* Allocate memory for wolfSSL CMAC object. */
ctx->internal = (Cmac*)XMALLOC(sizeof(Cmac), NULL, DYNAMIC_TYPE_CMAC);
if (ctx->internal == NULL) {
XFREE(ctx, NULL, DYNAMIC_TYPE_OPENSSL);
ctx = NULL;
}
}
if (ctx != NULL) {
/* Memory for wolfSSL CMAC object is allocated in
* wolfSSL_CMAC_Init().
*/
ctx->internal = NULL;
/* Allocate memory for EVP cipher context object. */
ctx->cctx = wolfSSL_EVP_CIPHER_CTX_new();
if (ctx->cctx == NULL) {
Expand All @@ -2110,9 +2106,11 @@ void wolfSSL_CMAC_CTX_free(WOLFSSL_CMAC_CTX *ctx)
if (ctx != NULL) {
/* Deallocate dynamically allocated fields. */
if (ctx->internal != NULL) {
wc_CmacFinal((Cmac*)ctx->internal, NULL, NULL);
XFREE(ctx->internal, NULL, DYNAMIC_TYPE_CMAC);
}
if (ctx->cctx != NULL) {
wolfSSL_EVP_CIPHER_CTX_cleanup(ctx->cctx);
wolfSSL_EVP_CIPHER_CTX_free(ctx->cctx);
}
/* Deallocate CMAC context object. */
Expand Down Expand Up @@ -2167,22 +2165,37 @@ int wolfSSL_CMAC_Init(WOLFSSL_CMAC_CTX* ctx, const void *key, size_t keySz,
/* Only AES-CBC ciphers are supported. */
if ((ret == 1) && (cipher != EVP_AES_128_CBC) &&
(cipher != EVP_AES_192_CBC) && (cipher != EVP_AES_256_CBC)) {
WOLFSSL_MSG("wolfSSL_CMAC_Init: requested cipher is unsupported");
ret = 0;
}
/* Key length must match cipher. */
if ((ret == 1) && ((int)keySz != wolfSSL_EVP_Cipher_key_length(cipher))) {
WOLFSSL_MSG("wolfSSL_CMAC_Init: "
"supplied key size doesn't match requested cipher");
ret = 0;
}

if ((ret == 1) && (ctx->internal == NULL)) {
/* Allocate memory for wolfSSL CMAC object. */
ctx->internal = (Cmac*)XMALLOC(sizeof(Cmac), NULL, DYNAMIC_TYPE_CMAC);
if (ctx->internal == NULL)
ret = 0;
}

/* Initialize the wolfCrypt CMAC object. */
if ((ret == 1) && (wc_InitCmac((Cmac*)ctx->internal, (const byte*)key,
(word32)keySz, WC_CMAC_AES, NULL) != 0)) {
WOLFSSL_MSG("wolfSSL_CMAC_Init: wc_InitCmac() failed");
XFREE(ctx->internal, NULL, DYNAMIC_TYPE_CMAC);
ctx->internal = NULL;
ret = 0;
}
if (ret == 1) {
/* Initialize the EVP cipher context object for encryption. */
ret = wolfSSL_EVP_CipherInit(ctx->cctx, cipher, (const byte*)key, NULL,
1);
if (ret != WOLFSSL_SUCCESS)
WOLFSSL_MSG("wolfSSL_CMAC_Init: wolfSSL_EVP_CipherInit() failed");
}

WOLFSSL_LEAVE("wolfSSL_CMAC_Init", ret);
Expand Down Expand Up @@ -2237,7 +2250,7 @@ int wolfSSL_CMAC_Final(WOLFSSL_CMAC_CTX* ctx, unsigned char* out, size_t* len)

WOLFSSL_ENTER("wolfSSL_CMAC_Final");

/* Valiudate parameters. */
/* Validate parameters. */
if (ctx == NULL) {
ret = 0;
}
Expand Down Expand Up @@ -2268,6 +2281,9 @@ int wolfSSL_CMAC_Final(WOLFSSL_CMAC_CTX* ctx, unsigned char* out, size_t* len)
else if (len != NULL) {
*len = (size_t)len32;
}

XFREE(ctx->internal, NULL, DYNAMIC_TYPE_CMAC);
ctx->internal = NULL;
}

WOLFSSL_LEAVE("wolfSSL_CMAC_Final", ret);
Expand Down Expand Up @@ -2899,7 +2915,7 @@ void wolfSSL_DES_ecb_encrypt(WOLFSSL_DES_cblock* in, WOLFSSL_DES_cblock* out,

#ifdef OPENSSL_EXTRA

#ifndef NO_AES
#if !defined(NO_AES) && !defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)

/* Sets the key into the AES key object for encryption or decryption.
*
Expand Down Expand Up @@ -3408,7 +3424,7 @@ size_t wolfSSL_CRYPTO_cts128_decrypt(const unsigned char *in,
return len;
}
#endif /* HAVE_CTS */
#endif /* NO_AES */
#endif /* !NO_AES && !WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API */
#endif /* OPENSSL_EXTRA */

/*******************************************************************************
Expand Down
82 changes: 71 additions & 11 deletions tests/api.c
Original file line number Diff line number Diff line change
Expand Up @@ -5680,7 +5680,7 @@ static int test_wolfSSL_EVP_CIPHER_CTX(void)
return 0;
}

static WC_INLINE int myTicketEncCbOpenSSL(WOLFSSL* ssl,
static int myTicketEncCbOpenSSL(WOLFSSL* ssl,
byte name[WOLFSSL_TICKET_NAME_SZ],
byte iv[WOLFSSL_TICKET_IV_SZ],
WOLFSSL_EVP_CIPHER_CTX *ectx,
Expand Down Expand Up @@ -15994,6 +15994,10 @@ static int test_wc_AesGcmStream(void)
ExpectIntEQ(wc_AesGcmDecryptFinal(aesDec, tag, AES_BLOCK_SIZE), 0);

/* Set key and IV through streaming init API. */
wc_AesFree(aesEnc);
wc_AesFree(aesDec);
ExpectIntEQ(wc_AesInit(aesEnc, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesInit(aesDec, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesGcmInit(aesEnc, key, sizeof(key), iv, AES_IV_SIZE), 0);
ExpectIntEQ(wc_AesGcmInit(aesDec, key, sizeof(key), iv, AES_IV_SIZE), 0);
/* Encrypt/decrypt one block and AAD of one block. */
Expand All @@ -16007,6 +16011,10 @@ static int test_wc_AesGcmStream(void)
ExpectIntEQ(wc_AesGcmDecryptFinal(aesDec, tag, AES_BLOCK_SIZE), 0);

/* Set key and IV through streaming init API. */
wc_AesFree(aesEnc);
wc_AesFree(aesDec);
ExpectIntEQ(wc_AesInit(aesEnc, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesInit(aesDec, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesGcmInit(aesEnc, key, sizeof(key), iv, AES_IV_SIZE), 0);
ExpectIntEQ(wc_AesGcmInit(aesDec, key, sizeof(key), iv, AES_IV_SIZE), 0);
/* No data to encrypt/decrypt one byte of AAD. */
Expand All @@ -16018,6 +16026,10 @@ static int test_wc_AesGcmStream(void)
ExpectIntEQ(wc_AesGcmDecryptFinal(aesDec, tag, AES_BLOCK_SIZE), 0);

/* Set key and IV through streaming init API. */
wc_AesFree(aesEnc);
wc_AesFree(aesDec);
ExpectIntEQ(wc_AesInit(aesEnc, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesInit(aesDec, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesGcmInit(aesEnc, key, sizeof(key), iv, AES_IV_SIZE), 0);
ExpectIntEQ(wc_AesGcmInit(aesDec, key, sizeof(key), iv, AES_IV_SIZE), 0);
/* Encrypt/decrypt one byte and no AAD. */
Expand All @@ -16030,6 +16042,10 @@ static int test_wc_AesGcmStream(void)
ExpectIntEQ(wc_AesGcmDecryptFinal(aesDec, tag, AES_BLOCK_SIZE), 0);

/* Set key and IV through streaming init API. */
wc_AesFree(aesEnc);
wc_AesFree(aesDec);
ExpectIntEQ(wc_AesInit(aesEnc, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesInit(aesDec, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesGcmInit(aesEnc, key, sizeof(key), iv, AES_IV_SIZE), 0);
ExpectIntEQ(wc_AesGcmInit(aesDec, key, sizeof(key), iv, AES_IV_SIZE), 0);
/* Encryption AES is one byte at a time */
Expand Down Expand Up @@ -16057,6 +16073,9 @@ static int test_wc_AesGcmStream(void)
ExpectIntEQ(wc_AesGcmDecryptFinal(aesDec, tag, AES_BLOCK_SIZE), 0);

/* Check streaming encryption can be decrypted with one shot. */
wc_AesFree(aesDec);
ExpectIntEQ(wc_AesInit(aesDec, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_AesGcmInit(aesDec, key, sizeof(key), iv, AES_IV_SIZE), 0);
ExpectIntEQ(wc_AesGcmSetKey(aesDec, key, sizeof(key)), 0);
ExpectIntEQ(wc_AesGcmDecrypt(aesDec, plain, out, sizeof(in), iv,
AES_IV_SIZE, tag, AES_BLOCK_SIZE, aad, sizeof(aad)), 0);
Expand Down Expand Up @@ -17612,7 +17631,6 @@ static int test_wc_AesCbcEncryptDecrypt(void)
ExpectIntEQ(wc_AesSetKey(&aes, key32, AES_BLOCK_SIZE * 2, iv,
AES_ENCRYPTION), 0);
ExpectIntEQ(wc_AesCbcEncrypt(&aes, enc, vector, sizeof(vector)), 0);
wc_AesFree(&aes);

/* Re init for decrypt and set flag. */
ExpectIntEQ(wc_AesSetKey(&aes, key32, AES_BLOCK_SIZE * 2, iv,
Expand Down Expand Up @@ -18154,13 +18172,13 @@ static int test_wc_GmacUpdate(void)
XMEMSET(tagOut2, 0, sizeof(tagOut2));
XMEMSET(tagOut3, 0, sizeof(tagOut3));

ExpectIntEQ(wc_AesInit(&gmac.aes, NULL, INVALID_DEVID), 0);

#ifdef WOLFSSL_AES_128
ExpectIntEQ(wc_AesInit(&gmac.aes, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_GmacSetKey(&gmac, key16, sizeof(key16)), 0);
ExpectIntEQ(wc_GmacUpdate(&gmac, iv, sizeof(iv), authIn, sizeof(authIn),
tagOut, sizeof(tag1)), 0);
ExpectIntEQ(XMEMCMP(tag1, tagOut, sizeof(tag1)), 0);
wc_AesFree(&gmac.aes);
#endif

#ifdef WOLFSSL_AES_192
Expand All @@ -18170,6 +18188,7 @@ static int test_wc_GmacUpdate(void)
ExpectIntEQ(wc_GmacUpdate(&gmac, iv2, sizeof(iv2), authIn2, sizeof(authIn2),
tagOut2, sizeof(tag2)), 0);
ExpectIntEQ(XMEMCMP(tagOut2, tag2, sizeof(tag2)), 0);
wc_AesFree(&gmac.aes);
#endif

#ifdef WOLFSSL_AES_256
Expand All @@ -18179,17 +18198,19 @@ static int test_wc_GmacUpdate(void)
ExpectIntEQ(wc_GmacUpdate(&gmac, iv3, sizeof(iv3), authIn3, sizeof(authIn3),
tagOut3, sizeof(tag3)), 0);
ExpectIntEQ(XMEMCMP(tag3, tagOut3, sizeof(tag3)), 0);
wc_AesFree(&gmac.aes);
#endif

/* Pass bad args. */
ExpectIntEQ(wc_AesInit(&gmac.aes, NULL, INVALID_DEVID), 0);
ExpectIntEQ(wc_GmacUpdate(NULL, iv3, sizeof(iv3), authIn3, sizeof(authIn3),
tagOut3, sizeof(tag3)), BAD_FUNC_ARG);
ExpectIntEQ(wc_GmacUpdate(&gmac, iv3, sizeof(iv3), authIn3, sizeof(authIn3),
tagOut3, sizeof(tag3) - 5), BAD_FUNC_ARG);
ExpectIntEQ(wc_GmacUpdate(&gmac, iv3, sizeof(iv3), authIn3, sizeof(authIn3),
tagOut3, sizeof(tag3) + 1), BAD_FUNC_ARG);

wc_AesFree(&gmac.aes);

#endif
return EXPECT_RESULT();
} /* END test_wc_GmacUpdate */
Expand Down Expand Up @@ -42239,7 +42260,8 @@ static int test_wolfSSL_DES_ede3_cbc_encrypt(void)
static int test_wolfSSL_AES_encrypt(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(HAVE_AES_ECB)
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(HAVE_AES_ECB) \
&& !defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)
AES_KEY enc;
AES_KEY dec;
const byte msg[] = {
Expand Down Expand Up @@ -42289,7 +42311,8 @@ static int test_wolfSSL_AES_encrypt(void)
static int test_wolfSSL_AES_ecb_encrypt(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(HAVE_AES_ECB)
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(HAVE_AES_ECB) \
&& !defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)
AES_KEY aes;
const byte msg[] =
{
Expand Down Expand Up @@ -42337,7 +42360,8 @@ static int test_wolfSSL_AES_ecb_encrypt(void)
static int test_wolfSSL_AES_cbc_encrypt(void)
{
EXPECT_DECLS;
#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(OPENSSL_EXTRA)
#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(OPENSSL_EXTRA) && \
!defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)
AES_KEY aes;
AES_KEY* aesN = NULL;
size_t len = 0;
Expand Down Expand Up @@ -42592,7 +42616,8 @@ static int test_wolfSSL_AES_cbc_encrypt(void)
static int test_wolfSSL_AES_cfb128_encrypt(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(WOLFSSL_AES_CFB)
#if defined(OPENSSL_EXTRA) && !defined(NO_AES) && defined(WOLFSSL_AES_CFB) && \
!defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)
AES_KEY aesEnc;
AES_KEY aesDec;
const byte msg[] = {
Expand Down Expand Up @@ -42684,7 +42709,7 @@ static int test_wolfSSL_CRYPTO_cts128(void)
{
EXPECT_DECLS;
#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(OPENSSL_EXTRA) && \
defined(HAVE_CTS)
defined(HAVE_CTS) && !defined(WOLFSSL_NO_OPENSSL_AES_LOW_LEVEL_API)
byte tmp[64]; /* Largest vector size */
/* Test vectors taken form RFC3962 Appendix B */
const testVector vects[] = {
Expand Down Expand Up @@ -46276,7 +46301,8 @@ static int test_wolfSSL_EVP_Cipher_extra(void)

for (i = 0; test_drive[i]; i++) {

ExpectIntNE((ret = EVP_CipherInit(evp, NULL, key, iv, 1)), 0);
ExpectIntNE((ret = EVP_CipherInit(evp, NULL, key, iv, 1)), 0);

init_offset();
test_drive_len[i] = 0;

Expand Down Expand Up @@ -46319,13 +46345,15 @@ static int test_wolfSSL_EVP_Cipher_extra(void)
}

ret = EVP_CipherFinal(evp, outb, &outl);

binary_dump(outb, outl);

ret = (((test_drive_len[i] % 16) != 0) && (ret == 0)) ||
(((test_drive_len[i] % 16) == 0) && (ret == 1));
ExpectTrue(ret);
}

ExpectIntEQ(wolfSSL_EVP_CIPHER_CTX_cleanup(evp), WOLFSSL_SUCCESS);

EVP_CIPHER_CTX_free(evp);
evp = NULL;
Expand Down Expand Up @@ -47818,6 +47846,7 @@ static int test_wolfSSL_EVP_CIPHER_CTX_key_length(void)

ExpectIntEQ(EVP_CipherInit(ctx, init, key, iv, 1), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_EVP_CIPHER_CTX_key_length(ctx), key_lengths[i]);

ExpectIntEQ(wolfSSL_EVP_CIPHER_CTX_set_key_length(ctx, key_lengths[i]),
WOLFSSL_SUCCESS);

Expand Down Expand Up @@ -54689,6 +54718,36 @@ static int test_wolfssl_EVP_aes_gcm(void)
ExpectIntEQ(0, XMEMCMP(plaintxt, decryptedtxt, decryptedtxtSz));

/* modify tag*/
if (i == 0) {
/* Default uses 96-bits IV length */
#ifdef WOLFSSL_AES_128
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_128_gcm(), NULL,
key, iv));
#elif defined(WOLFSSL_AES_192)
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_192_gcm(), NULL,
key, iv));
#elif defined(WOLFSSL_AES_256)
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_256_gcm(), NULL,
key, iv));
#endif
}
else {
#ifdef WOLFSSL_AES_128
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_128_gcm(), NULL,
NULL, NULL));
#elif defined(WOLFSSL_AES_192)
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_192_gcm(), NULL,
NULL, NULL));
#elif defined(WOLFSSL_AES_256)
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], EVP_aes_256_gcm(), NULL,
NULL, NULL));
#endif
/* non-default must to set the IV length first */
ExpectIntEQ(1, EVP_CIPHER_CTX_ctrl(&de[i], EVP_CTRL_GCM_SET_IVLEN,
ivSz, NULL));
ExpectIntEQ(1, EVP_DecryptInit_ex(&de[i], NULL, NULL, key, iv));

}
tag[AES_BLOCK_SIZE-1]+=0xBB;
ExpectIntEQ(1, EVP_DecryptUpdate(&de[i], NULL, &len, aad, aadSz));
ExpectIntEQ(1, EVP_CIPHER_CTX_ctrl(&de[i], EVP_CTRL_GCM_SET_TAG,
Expand All @@ -54698,6 +54757,7 @@ static int test_wolfssl_EVP_aes_gcm(void)
ciphertxtSz));
ExpectIntEQ(0, EVP_DecryptFinal_ex(&de[i], decryptedtxt, &len));
ExpectIntEQ(0, len);

ExpectIntEQ(wolfSSL_EVP_CIPHER_CTX_cleanup(&de[i]), 1);
}
#endif /* OPENSSL_EXTRA && !NO_AES && HAVE_AESGCM */
Expand Down
Loading

0 comments on commit 689a82a

Please sign in to comment.