Skip to content

Fix sha512 buffer copy #333

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

Closed
padelsbach wants to merge 1 commit into from
+561 −30
Closed

Fix sha512 buffer copy #333

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
155 changes: 130 additions & 25 deletions src/wh_client_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5162,7 +5162,8 @@ int wh_Client_Sha384Dma(whClientContext* ctx, wc_Sha384* sha, const uint8_t* in,

static int _xferSha512BlockAndUpdateDigest(whClientContext* ctx,
wc_Sha512* sha512,
uint32_t isLastBlock)
uint32_t isLastBlock,
int hashType)
{
uint16_t group = WH_MESSAGE_GROUP_CRYPTO;
uint16_t action = WH_MESSAGE_ACTION_NONE;
Expand Down Expand Up @@ -5199,10 +5200,13 @@ static int _xferSha512BlockAndUpdateDigest(whClientContext* ctx,

/* Send the hash state - this will be 0 on the first block on a properly
* initialized sha512 struct */
/* Source hashType from the dispatcher (info->hash.type) rather
* than sha512->hashType, since some wolfSSL ports do not
* populate the struct field. */
memcpy(req->resumeState.hash, sha512->digest, WC_SHA512_DIGEST_SIZE);
req->resumeState.hiLen = sha512->hiLen;
req->resumeState.loLen = sha512->loLen;
req->resumeState.hashType = sha512->hashType;
req->resumeState.hashType = hashType;
uint32_t req_len =
sizeof(whMessageCrypto_GenericRequestHeader) + sizeof(*req);

Expand Down Expand Up @@ -5232,7 +5236,7 @@ static int _xferSha512BlockAndUpdateDigest(whClientContext* ctx,
/* wolfCrypt allows positive error codes on success in some scenarios */
if (ret >= 0) {
WH_DEBUG_CLIENT_VERBOSE("Client SHA512 Res recv: ret=%d", ret);
WH_DEBUG_CLIENT_VERBOSE("hashType: %d\n", sha512->hashType);
WH_DEBUG_CLIENT_VERBOSE("hashType: %d\n", hashType);
/* Store the received intermediate hash in the sha512
* context and indicate the field is now valid and
* should be passed back and forth to the server */
Expand All @@ -5248,12 +5252,18 @@ static int _xferSha512BlockAndUpdateDigest(whClientContext* ctx,
return ret;
}

int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha512, const uint8_t* in,
uint32_t inLen, uint8_t* out)
/* Shared implementation for SHA-512 and its truncated variants
* (SHA-512/224, SHA-512/256). The hashType parameter selects the
* output digest size and the variant-appropriate re-init on
* finalize. Sourcing hashType from the caller (the dispatcher)
* rather than sha512->hashType avoids a dependency on the wolfSSL
* port populating that struct field. */
static int _doSha512(whClientContext* ctx, wc_Sha512* sha512,
const uint8_t* in, uint32_t inLen, uint8_t* out,
int hashType)
{
int ret = 0;
uint8_t* sha512BufferBytes = (uint8_t*)sha512->buffer;
int hashType = WC_HASH_TYPE_SHA512;

/* Caller invoked SHA Update:
* wc_CryptoCb_Sha512Hash(sha512, data, len, NULL) */
Expand All @@ -5268,15 +5278,16 @@ int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha512, const uint8_t* in,
sha512BufferBytes[sha512->buffLen++] = in[i++];
}
if (sha512->buffLen == WC_SHA512_BLOCK_SIZE) {
ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 0);
ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 0,
hashType);
sha512->buffLen = 0;
}
}

/* Process as many full blocks from the input data as we can */
while (ret == 0 && (inLen - i) >= WC_SHA512_BLOCK_SIZE) {
memcpy(sha512BufferBytes, in + i, WC_SHA512_BLOCK_SIZE);
ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 0);
ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 0, hashType);
i += WC_SHA512_BLOCK_SIZE;
}

Expand All @@ -5291,17 +5302,33 @@ int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha512, const uint8_t* in,
/* Caller invoked SHA finalize:
* wc_CryptoCb_Sha512Hash(sha512, NULL, 0, * hash) */
if (ret == 0 && out != NULL) {
ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 1);
word32 digestSz;

ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 1, hashType);

/* Use the hashType from the dispatcher (info->hash.type) to
* select the correct output size. This is more reliable than
* sha512->hashType which depends on the wolfSSL port setting
* it during init. */
switch (hashType) {
case WC_HASH_TYPE_SHA512_224:
digestSz = WC_SHA512_224_DIGEST_SIZE;
break;
case WC_HASH_TYPE_SHA512_256:
digestSz = WC_SHA512_256_DIGEST_SIZE;
break;
default:
digestSz = WC_SHA512_DIGEST_SIZE;
break;
}

/* Copy out the final hash value */
if (ret == 0) {
memcpy(out, sha512->digest, WC_SHA512_DIGEST_SIZE);
memcpy(out, sha512->digest, digestSz);
}
/* keep hashtype before initialization */
hashType = sha512->hashType;
/* reset the state of the sha context (without blowing away devId and
* hashType)
*/

/* Reset the sha context for potential reuse, calling the
* variant-appropriate init to preserve devId and hashType */
switch (hashType) {
case WC_HASH_TYPE_SHA512_224:
(void)wc_InitSha512_224_ex(sha512, NULL, sha512->devId);
Expand All @@ -5318,9 +5345,32 @@ int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha512, const uint8_t* in,
return ret;
}

int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
uint32_t inLen, uint8_t* out)
{
return _doSha512(ctx, sha, in, inLen, out, WC_HASH_TYPE_SHA512);
}

#if !defined(WOLFSSL_NOSHA512_224)
int wh_Client_Sha512_224(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen, uint8_t* out)
{
return _doSha512(ctx, sha, in, inLen, out, WC_HASH_TYPE_SHA512_224);
}
#endif /* !WOLFSSL_NOSHA512_224 */

#if !defined(WOLFSSL_NOSHA512_256)
int wh_Client_Sha512_256(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen, uint8_t* out)
{
return _doSha512(ctx, sha, in, inLen, out, WC_HASH_TYPE_SHA512_256);
}
#endif /* !WOLFSSL_NOSHA512_256 */

#ifdef WOLFHSM_CFG_DMA
int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
uint32_t inLen, uint8_t* out)
static int _doSha512Dma(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen, uint8_t* out,
int hashType)
{
int ret = WH_ERROR_OK;
wc_Sha512* sha512 = sha;
Expand All @@ -5332,21 +5382,50 @@ int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
uintptr_t inAddr = 0;
uintptr_t outAddr = 0;
uintptr_t stateAddr = 0;
word32 digestSz;

/* Select digest size based on variant */
switch (hashType) {
case WC_HASH_TYPE_SHA512_224:
if (sha->hashType != WC_HASH_TYPE_SHA512_224) {
return WH_ERROR_BADARGS;
}
digestSz = WC_SHA512_224_DIGEST_SIZE;
break;
case WC_HASH_TYPE_SHA512_256:
if (sha->hashType != WC_HASH_TYPE_SHA512_256) {
return WH_ERROR_BADARGS;
}
digestSz = WC_SHA512_256_DIGEST_SIZE;
break;
default:
/* No sha->hashType match required here: many wolfcrypt
* ports do not populate the field, and this path uses
* the maximum digest size (WC_SHA512_DIGEST_SIZE), so
* the server cannot write past the mapped region no
* matter which Final variant it dispatches. */
digestSz = WC_SHA512_DIGEST_SIZE;
break;
}
Copy link
Copy Markdown
Contributor

@rizlik rizlik Apr 16, 2026

Choose a reason for hiding this comment

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

I think here we need to add a check that error out if hashType == WC_HASH_TYPE_SHA512 && sha512->hashType == hashType).
This protects us from wolfcrypt passing bad wc_sha512 objects and more importantly to avoid the server to write more bytes than the ones used in the wh_Client_DmaProcessClientAddress and related APIs

Copy link
Copy Markdown
Contributor Author

@padelsbach padelsbach Apr 16, 2026

Choose a reason for hiding this comment

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

Many existing wolfcrypt ports do not set hashType so this check would go against the intent of this PR.

This wolfcrypt PR addresses that, but this wolfHSM change should be independent, imho. wolfSSL/wolfssl#10193

Copy link
Copy Markdown
Contributor Author

@padelsbach padelsbach Apr 16, 2026

Choose a reason for hiding this comment

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

Added a defensive check for the truncated 256 and 224 paths to protect against the overflow. Existing callers should fall into the default case -- likely an incorrect hash, but at least no overflow

Copy link
Copy Markdown
Contributor

@rizlik rizlik Apr 16, 2026

Choose a reason for hiding this comment

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

if we really want to allow port that do not set hashType we can do sha512->hashType = hashType but I probably leaner to be stricter than forgiving.
If you think that "fixing" sha512->hashType for the ports is better, go ahead.
I don't think this will generate a bad hash btw, the init is not done here and update operation is the same, final is just a truncation. Am I missing something?

Copy link
Copy Markdown
Contributor Author

@padelsbach padelsbach Apr 16, 2026

Choose a reason for hiding this comment

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

Ah, you're right, hash should be fine.


/* Get data pointer from the context to use as request/response storage */
dataPtr = (uint8_t*)wh_CommClient_GetDataPtr(ctx->comm);
if (dataPtr == NULL) {
return WH_ERROR_BADARGS;
}

/* Setup generic header and get pointer to request data */
/* Setup generic header and get pointer to request data. The
* algorithm-category is always WC_HASH_TYPE_SHA512 at this layer;
* the server's top-level dispatcher keys on this to find the
* SHA512 handler. The specific variant (512/224/256) is handled
* internally via sha512->hashType on the server side. */
req = (whMessageCrypto_Sha2DmaRequest*)_createCryptoRequest(
dataPtr, WC_HASH_TYPE_SHA512, ctx->cryptoAffinity);

if (in != NULL || out != NULL) {
req->state.sz = sizeof(*sha512);
req->input.sz = inLen;
req->output.sz = WC_SHA512_DIGEST_SIZE; /* not needed, but YOLO */
req->output.sz = digestSz;

/* Perform address translations */
ret = wh_Client_DmaProcessClientAddress(
Expand Down Expand Up @@ -5398,8 +5477,8 @@ int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
* rc */
ret = _getCryptoResponse(dataPtr, WC_HASH_TYPE_SHA512,
(uint8_t**)&resp);
/* Nothing to do on success, as server will have updated the context
* in client memory */
/* Nothing to do on success, as server will have updated the
* context in client memory */
}
}

Expand Down Expand Up @@ -5429,8 +5508,8 @@ int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
* rc */
ret = _getCryptoResponse(dataPtr, WC_HASH_TYPE_SHA512,
(uint8_t**)&resp);
/* Nothing to do on success, as server will have updated the output
* hash in client memory */
/* Nothing to do on success, as server will have updated the
* output hash in client memory */
}
}

Expand All @@ -5442,12 +5521,38 @@ int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in,
ctx, (uintptr_t)in, (void**)&inAddr, inLen,
WH_DMA_OPER_CLIENT_READ_POST, (whDmaFlags){0});
(void)wh_Client_DmaProcessClientAddress(
ctx, (uintptr_t)out, (void**)&outAddr, WC_SHA512_DIGEST_SIZE,
ctx, (uintptr_t)out, (void**)&outAddr, digestSz,
WH_DMA_OPER_CLIENT_WRITE_POST, (whDmaFlags){0});
}
return ret;
}
#endif /* WOLFHSM_CFG_DMA */

int wh_Client_Sha512Dma(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen, uint8_t* out)
{
return _doSha512Dma(ctx, sha, in, inLen, out, WC_HASH_TYPE_SHA512);
}

#if !defined(WOLFSSL_NOSHA512_224)
int wh_Client_Sha512_224Dma(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen,
uint8_t* out)
{
return _doSha512Dma(ctx, sha, in, inLen, out,
WC_HASH_TYPE_SHA512_224);
}
#endif /* !WOLFSSL_NOSHA512_224 */

#if !defined(WOLFSSL_NOSHA512_256)
int wh_Client_Sha512_256Dma(whClientContext* ctx, wc_Sha512* sha,
const uint8_t* in, uint32_t inLen,
uint8_t* out)
{
return _doSha512Dma(ctx, sha, in, inLen, out,
WC_HASH_TYPE_SHA512_256);
}
#endif /* !WOLFSSL_NOSHA512_256 */
#endif /* WOLFHSM_CFG_DMA */
#endif /* WOLFSSL_SHA512 */

#ifdef HAVE_DILITHIUM
Expand Down
42 changes: 41 additions & 1 deletion src/wh_client_cryptocb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,26 @@ int wh_Client_CryptoCb(int devId, wc_CryptoInfo* info, void* inCtx)

ret = wh_Client_Sha512(ctx, sha, in, inLen, out);
} break;
#if !defined(WOLFSSL_NOSHA512_224)
case WC_HASH_TYPE_SHA512_224: {
wc_Sha512* sha = info->hash.sha512;
const uint8_t* in = info->hash.in;
uint32_t inLen = info->hash.inSz;
uint8_t* out = info->hash.digest;

ret = wh_Client_Sha512_224(ctx, sha, in, inLen, out);
} break;
#endif /* !WOLFSSL_NOSHA512_224 */
#if !defined(WOLFSSL_NOSHA512_256)
case WC_HASH_TYPE_SHA512_256: {
wc_Sha512* sha = info->hash.sha512;
const uint8_t* in = info->hash.in;
uint32_t inLen = info->hash.inSz;
uint8_t* out = info->hash.digest;

ret = wh_Client_Sha512_256(ctx, sha, in, inLen, out);
} break;
#endif /* !WOLFSSL_NOSHA512_256 */
#endif /* WOLFSSL_SHA512 && WOLFSSL_SHA512_HASHTYPE */
default:
ret = CRYPTOCB_UNAVAILABLE;
Expand Down Expand Up @@ -847,7 +867,27 @@ int wh_Client_CryptoCbDma(int devId, wc_CryptoInfo* info, void* inCtx)

ret = wh_Client_Sha512Dma(ctx, sha, in, inLen, out);
} break;
#endif /* WOLFSSL_SHA512 && defined(WOLFSSL_SHA512_HASHTYPE) */
#if !defined(WOLFSSL_NOSHA512_224)
case WC_HASH_TYPE_SHA512_224: {
wc_Sha512* sha = info->hash.sha512;
const uint8_t* in = info->hash.in;
uint32_t inLen = info->hash.inSz;
uint8_t* out = info->hash.digest;

ret = wh_Client_Sha512_224Dma(ctx, sha, in, inLen, out);
} break;
#endif /* !WOLFSSL_NOSHA512_224 */
#if !defined(WOLFSSL_NOSHA512_256)
case WC_HASH_TYPE_SHA512_256: {
wc_Sha512* sha = info->hash.sha512;
const uint8_t* in = info->hash.in;
uint32_t inLen = info->hash.inSz;
uint8_t* out = info->hash.digest;

ret = wh_Client_Sha512_256Dma(ctx, sha, in, inLen, out);
} break;
#endif /* !WOLFSSL_NOSHA512_256 */
#endif /* WOLFSSL_SHA512 && WOLFSSL_SHA512_HASHTYPE */
default:
ret = CRYPTOCB_UNAVAILABLE;
break;
Expand Down
Loading
Loading