Skip to content

force local wc_nnn hash struct variables to zeros #6068

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
gojimmypi wants to merge 4 commits into from
Closed

force local wc_nnn hash struct variables to zeros #6068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ccb5abb
force local wc_nnn hash struct variables to zeros
gojimmypi Feb 8, 2023
3ad7e29
revert hash.c changes
gojimmypi Feb 9, 2023
6a17a14
set hash sha and md5 struct to zero during init
gojimmypi Feb 9, 2023
3656703
remove redundant memset
gojimmypi Feb 9, 2023
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
11 changes: 6 additions & 5 deletions wolfcrypt/src/md5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,14 +314,18 @@ int wc_InitMd5_ex(wc_Md5* md5, void* heap, int devId)
{
int ret = 0;

if (md5 == NULL)
if (md5 == NULL) {
return BAD_FUNC_ARG;
}

XMEMSET(md5, 0, sizeof(wc_Md5));

md5->heap = heap;

ret = _InitMd5(md5);
if (ret != 0)
if (ret != 0) {
return ret;
}

#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_MD5)
ret = wolfAsync_DevCtxInit(&md5->asyncDev, WOLFSSL_ASYNC_MARKER_MD5,
Expand Down Expand Up @@ -495,9 +499,6 @@ int wc_Md5Final(wc_Md5* md5, byte* hash)

int wc_InitMd5(wc_Md5* md5)
{
if (md5 == NULL) {
return BAD_FUNC_ARG;
}
return wc_InitMd5_ex(md5, NULL, INVALID_DEVID);
}

Expand Down
15 changes: 10 additions & 5 deletions wolfcrypt/src/sha.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,8 +524,11 @@ int wc_InitSha_ex(wc_Sha* sha, void* heap, int devId)
{
int ret = 0;

if (sha == NULL)
if (sha == NULL) {
return BAD_FUNC_ARG;
}

XMEMSET(sha, 0, sizeof(wc_Sha));

sha->heap = heap;
#ifdef WOLF_CRYPTO_CB
Expand All @@ -540,8 +543,9 @@ int wc_InitSha_ex(wc_Sha* sha, void* heap, int devId)
sha->ctx.lockDepth = 0; /* keep track of how many times lock is called */
#endif
ret = InitSha(sha);
if (ret != 0)
if (ret != 0) {
return ret;
}

#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_SHA)
ret = wolfAsync_DevCtxInit(&sha->asyncDev, WOLFSSL_ASYNC_MARKER_SHA,
Expand Down Expand Up @@ -869,7 +873,7 @@ void wc_ShaFree(wc_Sha* sha)

#endif /* !defined(WOLFSSL_HAVE_PSA) || defined(WOLFSSL_PSA_NO_HASH) */
#endif /* !WOLFSSL_TI_HASH */
#endif /* HAVE_FIPS */
#endif /* !HAVE_FIPS ... */

#if !defined(WOLFSSL_TI_HASH) && !defined(WOLFSSL_IMXRT_DCP)

Expand Down Expand Up @@ -944,9 +948,10 @@ int wc_ShaCopy(wc_Sha* src, wc_Sha* dst)
#endif
return ret;
}
#endif /* defined(WOLFSSL_RENESAS_TSIP_CRYPT) ... */
#endif /* !WOLFSSL_TI_HASH && !WOLFSSL_IMXRT_DCP */
#endif /* !defined(WOLFSSL_HAVE_PSA) || defined(WOLFSSL_PSA_NO_HASH) */
#endif /* !defined(WOLFSSL_RENESAS_TSIP_CRYPT) ||
defined(NO_WOLFSSL_RENESAS_TSIP_CRYPT_HASH) */
#endif /* !defined(WOLFSSL_TI_HASH) && !defined(WOLFSSL_IMXRT_DCP) */

#ifdef WOLFSSL_HASH_FLAGS
int wc_ShaSetFlags(wc_Sha* sha, word32 flags)
Expand Down
8 changes: 6 additions & 2 deletions wolfcrypt/src/sha256.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1567,17 +1567,21 @@ static int InitSha256(wc_Sha256* sha256)
{
int ret = 0;

if (sha224 == NULL)
if (sha224 == NULL) {
return BAD_FUNC_ARG;
}

XMEMSET(sha224, 0, sizeof(wc_Sha224));
Copy link
Contributor

@SparkiDev SparkiDev Feb 9, 2023

Choose a reason for hiding this comment

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

Missed wc_InitSha256_ex()

Copy link
Contributor Author

@gojimmypi gojimmypi Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@SparkiDev thanks for your review! Would you recommend that I do a XMEMSET in each of these?

image

I'm not entirely convinced it is a good idea for software-only hashes. The only reason I found this and started to do the initializations was due to undesired hardware encryption state data being copied into a new sha ctx instance.

Please advise. Thank you.

  • Edit: Here's a code explorer view for conversation sake:

image

Copy link
Contributor

@SparkiDev SparkiDev Feb 12, 2023

Choose a reason for hiding this comment

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

If the PR is to memset the data structure when using hardware and they are the implementations for hardware, then yes.
If you can't find a common for each function, then it needs to be done in each.


sha224->heap = heap;
#ifdef WOLFSSL_SMALL_STACK_CACHE
sha224->W = NULL;
#endif

ret = InitSha224(sha224);
if (ret != 0)
if (ret != 0) {
return ret;
}

#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_SHA224)
ret = wolfAsync_DevCtxInit(&sha224->asyncDev,
Expand Down
8 changes: 6 additions & 2 deletions wolfcrypt/src/sha3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,13 +762,17 @@ static int wc_InitSha3(wc_Sha3* sha3, void* heap, int devId)
{
int ret = 0;

if (sha3 == NULL)
if (sha3 == NULL) {
return BAD_FUNC_ARG;
}

XMEMSET(sha3, 0, sizeof(wc_Sha3));

sha3->heap = heap;
ret = InitSha3(sha3);
if (ret != 0)
if (ret != 0) {
return ret;
}

#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_SHA3)
ret = wolfAsync_DevCtxInit(&sha3->asyncDev,
Expand Down
11 changes: 8 additions & 3 deletions wolfcrypt/src/sha512.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,10 +573,13 @@ static int InitSha512_256(wc_Sha512* sha512)
static int InitSha512_Family(wc_Sha512* sha512, void* heap, int devId,
int (*initfp)(wc_Sha512*))
{
int ret = 0;
int ret = 0;

if (sha512 == NULL)
if (sha512 == NULL) {
return BAD_FUNC_ARG;
}

XMEMSET(sha512, 0, sizeof(wc_Sha512));
Copy link
Contributor

@SparkiDev SparkiDev Feb 9, 2023

Choose a reason for hiding this comment

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

Missed wc_InitSha383_ex().


sha512->heap = heap;
#ifdef WOLFSSL_SMALL_STACK_CACHE
Expand All @@ -588,8 +591,9 @@ static int InitSha512_Family(wc_Sha512* sha512, void* heap, int devId,
#endif

ret = initfp(sha512);
if (ret != 0)
if (ret != 0) {
return ret;
}

#if defined(USE_INTEL_SPEEDUP) && \
(defined(HAVE_INTEL_AVX1) || defined(HAVE_INTEL_AVX2))
Expand Down Expand Up @@ -1620,6 +1624,7 @@ int wc_InitSha512_224(wc_Sha512* sha)
{
return wc_InitSha512_224_ex(sha, NULL, INVALID_DEVID);
}

int wc_Sha512_224Update(wc_Sha512* sha, const byte* data, word32 len)
{
return wc_Sha512Update(sha, data, len);
Expand Down