-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix: WolfCrypt Fenrir - 12 fixes #10786
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
d6dc919
5ada297
5fc8031
2541f5e
1d990db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,20 +135,14 @@ int wc_Sha256Update(wc_Sha256* sha, const byte* in, word32 sz) | |
| #ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP | ||
| /* keep full message to hash at end instead of incremental updates */ | ||
| if (sha->len < sha->used + sz) { | ||
| if (sha->msg == NULL) { | ||
| sha->msg = (byte*)XMALLOC(sha->used + sz, sha->heap, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
| } else { | ||
| byte* pt = (byte*)XREALLOC(sha->msg, sha->used + sz, sha->heap, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (pt == NULL) { | ||
| return MEMORY_E; | ||
| } | ||
| sha->msg = pt; | ||
| } | ||
| if (sha->msg == NULL) { | ||
| byte* pt = (byte*)XREALLOC(sha->msg, sha->used + sz, sha->heap, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (pt == NULL) { | ||
| return MEMORY_E; | ||
| } | ||
|
|
||
| sha->msg = pt; | ||
|
|
||
| sha->len = sha->used + sz; | ||
| } | ||
| XMEMCPY(sha->msg + sha->used, in, sz); | ||
|
|
@@ -173,14 +167,16 @@ int wc_Sha256Final(wc_Sha256* sha, byte* hash) | |
| #ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP | ||
| /* keep full message to hash at end instead of incremental updates */ | ||
| if ((ret = HashUpdate(sha, CRYPTO_SHA2_256, sha->msg, sha->used)) < 0) { | ||
| wc_Sha256Free(sha); | ||
| return ret; | ||
| } | ||
| XFREE(sha->msg, sha->heap, DYNAMIC_TYPE_TMP_BUFFER); | ||
| sha->msg = NULL; | ||
| #endif | ||
| ret = GetDigest(sha, CRYPTO_SHA2_256, hash); | ||
| if (ret != 0) { | ||
| return ret; | ||
| wc_Sha256Free(sha); | ||
| return ret; | ||
| } | ||
|
|
||
| wc_Sha256Free(sha); | ||
|
|
@@ -198,9 +194,11 @@ int wc_Sha256GetHash(wc_Sha256* sha, byte* hash) | |
| { | ||
| int ret; | ||
| wc_Sha256 cpy; | ||
| wc_Sha256Copy(sha, &cpy); | ||
| XMEMSET(&cpy, 0, sizeof(cpy)); /* ZII */ | ||
| ret = wc_Sha256Copy(sha, &cpy); | ||
|
|
||
| if ((ret = HashUpdate(&cpy, CRYPTO_SHA2_256, cpy.msg, cpy.used)) == 0) { | ||
| if (ret == 0 && | ||
| (ret = HashUpdate(&cpy, CRYPTO_SHA2_256, cpy.msg, cpy.used)) == 0) { | ||
| /* help static analysis tools out */ | ||
| XMEMSET(hash, 0, WC_SHA256_DIGEST_SIZE); | ||
| ret = GetDigest(&cpy, CRYPTO_SHA2_256, hash); | ||
|
|
@@ -219,22 +217,39 @@ int wc_Sha256GetHash(wc_Sha256* sha, byte* hash) | |
|
|
||
| int wc_Sha256Copy(wc_Sha256* src, wc_Sha256* dst) | ||
| { | ||
| int ret = 0; | ||
|
|
||
| if (src == NULL || dst == NULL) { | ||
| return BAD_FUNC_ARG; | ||
| } | ||
|
|
||
| wc_InitSha256_ex(dst, src->heap, 0); | ||
| #ifdef WOLFSSL_DEVCRYPTO_HASH_KEEP | ||
| wc_Sha256Free(dst); | ||
| if ((ret = wc_InitSha256_ex(dst, src->heap, 0)) != 0) { | ||
| /* make sure that any attempts to free dst | ||
| * dont accidentally close an unopened fd */ | ||
| dst->ctx.inited = 0; | ||
| dst->ctx.cfd = -1; | ||
| return ret; | ||
| } | ||
| dst->len = src->len; | ||
| dst->used = src->used; | ||
| dst->msg = (byte*)XMALLOC(src->len, dst->heap, DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (dst->msg == NULL) { | ||
| wc_Sha256Free(dst); | ||
| return MEMORY_E; | ||
| } | ||
| XMEMCPY(dst->msg, src->msg, src->len); | ||
| #endif | ||
|
|
||
| return 0; | ||
| return ret; | ||
| #else | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [Medium] wc_Sha256Copy now returns NOT_COMPILED_IN without WOLFSSL_DEVCRYPTO_HASH_KEEP · api When Fix: Confirm this is intended and note in the devcrypto documentation/config that WOLFSSL_DEVCRYPTO_HASH_KEEP is required whenever hash-copy (e.g. TLS) is needed; consider a build-time guard/warning to catch the misconfiguration. |
||
| (void)src; | ||
| (void)dst; | ||
| (void)ret; | ||
|
|
||
| WOLFSSL_MSG("Compile with WOLFSSL_DEVCRYPTO_HASH_KEEP for this feature"); | ||
| return NOT_COMPILED_IN; | ||
| #endif | ||
| } | ||
|
|
||
| #endif /* !NO_SHA256 */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 [Medium] devcrypto wc_Sha256Copy frees the destination before initializing it, reading an uninitialized dst (arbitrary fd close… · Security
BEFORE, devcrypto wc_Sha256Copy called
wc_InitSha256_ex(dst, ...)FIRST, which doesXMEMSET(sha, 0, sizeof(wc_Sha256))and is therefore safe on an uninitialized/garbage dst. AFTER, the function callswc_Sha256Free(dst)as its very first action (to fix a leak of a pre-existing dst session/msg). For WOLFSSL_DEVCRYPTO_HASH,wc_Sha256Freeexecuteswc_DevCryptoFree(&dst->ctx)-> if the garbagedst->ctx.initedbit reads as 1 it runsioctl(dst->ctx.cfd, ...)andclose(dst->ctx.cfd)on a garbage descriptor (closing an unrelated live fd/socket in the process), and for WOLFSSL_DEVCRYPTO_HASH_KEEP it runsForceZero(dst->msg, dst->len)thenXFREE(dst->msg)on a garbage pointer/length (arbitrary write + invalid free / heap corruption). This changes the long-standing wc_XxxCopy contract ("the copy methods overwrite the entire dest low level hash struct"), which the generic sha256.c backend still honors viaXMEMCPY(dst, src, sizeof)without touching dst first. wc_Sha256Copy is a public WOLFSSL_API. The one internal caller that could hit this (wc_Sha256GetHash) was patched in the same diff withXMEMSET(&cpy, 0, sizeof(cpy)), and the in-tree SHA-256 call sites zero dst first, so in-tree exploitability is low; but any external consumer that relied on the previous "copy overwrites everything" behavior and passed an uninitialized dst now triggers undefined behavior.Fix: Zero/initialize dst before freeing it, or free the prior contents conditionally. Simplest:
XMEMSET(dst, 0, sizeof(*dst));(or restore the original order: init dst first) and only free a caller-supplied dst when the API contract explicitly guarantees dst is already initialized. Alternatively, document and enforce that dst must be a zeroed/initialized object and update the sha256.h contract for all backends.