Skip to content

Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gasbytes wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/wolfprovider/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,9 @@ int wp_decode_should_skip(int castType, const unsigned char* der, word32 len,
const int* allowedNids, size_t nAllowed);
#endif /* HAVE_FIPS */

int wp_hmac_tls_dummy_blocks(enum wc_HashType hashType, size_t macSize,
size_t tlsDataSize, size_t dataLen);

byte wp_ct_byte_mask_eq(byte a, byte b);
byte wp_ct_byte_mask_ne(byte a, byte b);
byte wp_ct_int_mask_gte(int a, int b);
Expand Down
40 changes: 38 additions & 2 deletions src/wp_aes_stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ typedef struct wp_AesStreamCtx {

/** Operation being performed is encryption. */
unsigned int enc:1;
/** An IV has been set. */
unsigned int ivSet:1;

/** Current IV. */
unsigned char iv[AES_BLOCK_SIZE];
Expand Down Expand Up @@ -268,6 +270,7 @@ static int wp_aes_init_iv(wp_AesStreamCtx *ctx, const unsigned char *iv,
if (ok) {
XMEMCPY(ctx->iv, iv, ivLen);
XMEMCPY(ctx->oiv, iv, ivLen);
ctx->ivSet = 1;
}

WOLFPROV_LEAVE(WP_LOG_COMP_AES, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
Expand Down Expand Up @@ -308,6 +311,13 @@ static int wp_aes_stream_init(wp_AesStreamCtx *ctx, const unsigned char *key,
if (ok && (iv != NULL) && (!wp_aes_init_iv(ctx, iv, ivLen))) {
ok = 0;
}
if (ok && (iv == NULL) && ctx->ivSet &&
Comment thread
aidangarske marked this conversation as resolved.
((ctx->mode == EVP_CIPH_CBC_MODE) ||
(ctx->mode == EVP_CIPH_CFB_MODE))) {
if (!wp_aes_init_iv(ctx, ctx->oiv, ctx->ivLen)) {
ok = 0;
}
}

if (ok && (key != NULL)) {
if (keyLen != ctx->keyLen) {
Expand All @@ -321,7 +331,7 @@ static int wp_aes_stream_init(wp_AesStreamCtx *ctx, const unsigned char *key,
}
#endif
WP_CHECK_FIPS_ALGO(WP_CAST_ALGO_AES);
rc = wc_AesSetKey(&ctx->aes, key, (word32)ctx->keyLen, iv,
rc = wc_AesSetKey(&ctx->aes, key, (word32)ctx->keyLen, ctx->iv,
dir);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_AesSetKey", rc);
Expand Down Expand Up @@ -404,11 +414,24 @@ static int wp_aes_cts_encrypt(wp_AesStreamCtx *ctx, unsigned char *out,
* the existing wolfSSL AES_CTS APIs with FIPS, so the implementation is
* effectively copied here from wolfSSL internals. */

if (inLen == AES_BLOCK_SIZE) {
XMEMCPY(&ctx->aes.reg, ctx->iv, ctx->ivLen);
rc = wc_AesCbcEncrypt(&ctx->aes, out, in, AES_BLOCK_SIZE);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_AesCbcEncrypt", rc);
ok = 0;
}
if (ok) {
XMEMCPY(ctx->iv, ctx->aes.reg, ctx->ivLen);
}
return ok;
}
Comment thread
gasbytes marked this conversation as resolved.

blocks = (int)((inLen + (AES_BLOCK_SIZE - 1)) / AES_BLOCK_SIZE);
blocks -= 2;
XMEMSET(ctsBlock, 0, AES_BLOCK_SIZE * 2);
XMEMCPY(&ctx->aes.reg, ctx->iv, ctx->ivLen);
if (ok && blocks > 0) {
XMEMCPY(&ctx->aes.reg, ctx->iv, ctx->ivLen);
rc = wc_AesCbcEncrypt(&ctx->aes, out, in, blocks * AES_BLOCK_SIZE);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_AesCbcEncrypt", rc);
Expand Down Expand Up @@ -458,6 +481,19 @@ static int wp_aes_cts_decrypt(wp_AesStreamCtx *ctx, unsigned char *out,
* the existing wolfSSL AES_CTS APIs with FIPS, so the implementation is
* effectively copied here from wolfSSL internals. */

if (inLen == AES_BLOCK_SIZE) {
XMEMCPY(&ctx->aes.reg, ctx->iv, ctx->ivLen);
rc = wc_AesCbcDecrypt(&ctx->aes, out, in, AES_BLOCK_SIZE);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_AesCbcDecrypt", rc);
ok = 0;
}
if (ok) {
XMEMCPY(ctx->iv, ctx->aes.reg, ctx->ivLen);
}
return ok;
}

partialSz = inLen % AES_BLOCK_SIZE;
if (partialSz == 0) {
partialSz = AES_BLOCK_SIZE;
Expand Down
98 changes: 98 additions & 0 deletions src/wp_hmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ typedef struct wp_HmacCtx {
unsigned char* key;
/** Length of private key in bytes. */
size_t keyLen;

/** Length of the padded TLS record including MAC and padding. */
Comment thread
aidangarske marked this conversation as resolved.
size_t tlsDataSize;
/** Number of dummy blocks to hash in final to equalize hashing time. */
int tlsDummyBlocks;
} wp_HmacCtx;


Expand Down Expand Up @@ -205,6 +210,8 @@ static wp_HmacCtx* wp_hmac_dup(wp_HmacCtx* src)
dst->type = src->type;
dst->size = src->size;
dst->provCtx = src->provCtx;
dst->tlsDataSize = src->tlsDataSize;
dst->tlsDummyBlocks = src->tlsDummyBlocks;

/* Copy the Hmac struct directly to preserve in-progress state.
* wc_HmacCopy is not available in all wolfSSL versions. */
Expand Down Expand Up @@ -258,6 +265,64 @@ static int wp_hmac_init(wp_HmacCtx* macCtx, const unsigned char* key,
return ok;
}

/** Length of the TLS record header hashed before the record data. */
#define WP_TLS_HMAC_HEADER_SZ 13

/**
* Count the blocks a hash processes for a message of the given length.
*
* @param [in] len Length of message in bytes.
* @param [in] blockBits Log base 2 of the hash block size.
* @param [in] blockMask Hash block size minus one.
* @param [in] padSz Bytes of padding the hash appends to the message.
* @return Number of blocks processed.
*/
static int wp_hmac_blocks(word32 len, int blockBits, word32 blockMask,
word32 padSz)
{
return (int)(len >> blockBits) +

@padelsbach padelsbach Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the shift arithmetic work for HMAC-SHA3?

(wp_ct_int_mask_lt((int)((len + padSz) & blockMask), (int)padSz) & 1);
}

/**
* Calculate the number of dummy blocks to hash when finalizing a TLS record.
*
* @param [in] hashType wolfSSL digest type.
* @param [in] macSize Output size of the digest in bytes.
* @param [in] tlsDataSize Length of the padded record including MAC and
* padding.
* @param [in] dataLen Length of data passed to update in bytes.
* @return Number of dummy blocks to hash. At least one.
*/
int wp_hmac_tls_dummy_blocks(enum wc_HashType hashType, size_t macSize,
size_t tlsDataSize, size_t dataLen)
{
int blockSizeRet = wc_HashGetBlockSize(hashType);
word32 blockSz;
word32 blockMask;
word32 padSz;
word32 realSz;
word32 maxSz;
int blockBits = 0;

if ((blockSizeRet <= 0) || (tlsDataSize <= macSize)) {
return 1;
}

blockSz = (word32)blockSizeRet;
blockMask = blockSz - 1;
while (((word32)1 << blockBits) < blockSz) {
blockBits++;
}
padSz = blockSz >> 3;

realSz = WP_TLS_HMAC_HEADER_SZ + (word32)dataLen;
maxSz = WP_TLS_HMAC_HEADER_SZ + (word32)(tlsDataSize - 1 - macSize);

return 1 + wp_hmac_blocks(maxSz, blockBits, blockMask, padSz) -
wp_hmac_blocks(realSz, blockBits, blockMask, padSz);
}

/**
* Update the MAC state with data.
*
Expand All @@ -274,6 +339,11 @@ static int wp_hmac_update(wp_HmacCtx* macCtx, const unsigned char* data,

WOLFPROV_ENTER(WP_LOG_COMP_MAC, "wp_hmac_update");

if (macCtx->tlsDataSize > 0) {

@padelsbach padelsbach Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work on chunk'ed data, or combined header+data? Seems like the block count is reestablished on each call

macCtx->tlsDummyBlocks = wp_hmac_tls_dummy_blocks(macCtx->type,
macCtx->size, macCtx->tlsDataSize, dataLen);
}

while (ok && (dataLen > 0)) {
word32 chunk = (!WP_FITS_WORD32(dataLen)) ?
0xFFFFFFFFU : (word32)dataLen;
Expand Down Expand Up @@ -323,6 +393,24 @@ static int wp_hmac_final(wp_HmacCtx* macCtx, unsigned char* out, size_t* outl,
ok = 0;
}
}
if (ok && (macCtx->tlsDataSize > 0)) {
unsigned char dummy[WC_MAX_BLOCK_SIZE];
int blockSz = wc_HashGetBlockSize(macCtx->type);
int i;

if (blockSz <= 0) {
ok = 0;
}
XMEMSET(dummy, 0, sizeof(dummy));
for (i = 0; ok && (i < macCtx->tlsDummyBlocks); i++) {
rc = wc_HmacUpdate(&macCtx->hmac, dummy, (word32)blockSz);
if (rc != 0) {

@padelsbach padelsbach Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does a hmac operation after wc_HmacFinal which resets the object. We might get bogus or non-deterministic results. Can you check?

WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_HmacUpdate",
rc);
ok = 0;
}
}
}
if (ok) {
*outl = macCtx->size;
}
Expand Down Expand Up @@ -402,6 +490,7 @@ static const OSSL_PARAM* wp_hmac_settable_ctx_params(wp_HmacCtx* macCtx,
static const OSSL_PARAM wp_hmac_supported_settable_ctx_params[] = {
OSSL_PARAM_utf8_string(OSSL_MAC_PARAM_DIGEST, NULL, 0),
OSSL_PARAM_octet_string(OSSL_MAC_PARAM_KEY, NULL, 0),
OSSL_PARAM_size_t(OSSL_MAC_PARAM_TLS_DATA_SIZE, NULL),
OSSL_PARAM_END
};
(void)macCtx;
Expand Down Expand Up @@ -443,6 +532,15 @@ static int wp_hmac_set_ctx_params(wp_HmacCtx* macCtx, const OSSL_PARAM params[])
ok = 0;
}
}

if (ok) {
const OSSL_PARAM* p = OSSL_PARAM_locate_const(params,
OSSL_MAC_PARAM_TLS_DATA_SIZE);
if ((p != NULL) && (!OSSL_PARAM_get_size_t(p,
&macCtx->tlsDataSize))) {
ok = 0;
}
}
}

WOLFPROV_LEAVE(WP_LOG_COMP_MAC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
Expand Down
Loading
Loading