Conversation
.github/workflows/wolfCrypt-Wconversion.yml: Add -Wcast-qual to all scenarios.
wolfssl/wolfcrypt/signature.h, wolfcrypt/src/signature.c, doc/dox_comments/header_files/signature.h:
Remove incorrect const qualifier on the key argument in
* wc_SignatureVerifyHash()
* wc_SignatureVerify()
* wc_SignatureGenerateHash()
* wc_SignatureGenerateHash_ex()
* wc_SignatureGenerate()
* wc_SignatureGenerate_ex()
This fixes UB code patterns throughout signature.c. key is inherently
accessed readwrite by the underlying low level crypto. Fortunately, wolfCrypt
has no APIs/methods to allow actual const MPI key objects, therefore these
seeming breaking API changes can't actually break any users.
globally:
* Add const qualifiers to all struct pointer members that are assigned values
computed from const pointers.
* Add const qualifiers to intermediate casts for accessors and read-only
dereference constructs, as needed for -Wcast-qual hygiene, e.g. for a macro
GET_U16(a), use (*(const word16*)(a)) rather than (*(word16*)(a)).
* Add const qualifiers to internal declarations, and remove illegal casts, as
needed for -Wcast-qual hygiene.
* Add missing const qualifiers to all casts for argument, operand, and
assignment type agreement, as needed for -Wcast-qual hygiene, e.g.
"*data = (const byte*)dataASN->data.ref.data" rather than
"*data = (byte*)dataASN->data.ref.data".
wolfssl/wolfcrypt/asn.h, wolfssl/wolfcrypt/asn_public.h, wolfcrypt/src/asn.c, wolfcrypt/src/asn_orig.c:
* Add additional lifecycle management for object members that are only sometimes locally allocated:
DNS_entry.nameStored
DNS_entry.ipStringStored
DNS_entry.ridStringStored
wolfssl/wolfcrypt/types.h: add WC_BARRIER() macro -- a portable construct that
prevents compiler optimizers from reordering operations across the barrier.
wolfssl/wolfcrypt/blake2-impl.h, wolfcrypt/src/blake2s.c, wolfcrypt/src/blake2b.c:
* In blake2b_init(), blake2b_init_key(), blake2s_init(), and
blake2s_init_key(), refactor blake2b_param initialization using WC_BARRIER()
(fixes volatile abuse that triggered -Wcast-qual).
* Remove the residual and unused WOLFSSL_BLAKE2[BS]_INIT_EACH_FIELD code.
wolfcrypt/src/ecc.c and wolfssl/wolfcrypt/ecc.h:
Remove incorrect const qualifier on curve arg to wc_ecc_free_curve() (internal function).
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10120
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-portability, wolfcrypt-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| @@ -3272,8 +3273,9 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| dnsEntry->len = strLen; | |||
| XMEMCPY(dnsEntry->name, &input[idx], (size_t)strLen); | |||
| dnsEntry->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)dnsEntry->name, &input[idx], | |||
| (size_t)strLen); | |||
| ((char *)(wc_ptr_t)dnsEntry->name)[strLen] = '\0'; | |||
|
|
|||
| AddAltName(cert, dnsEntry); | |||
|
|
|||
| @@ -3316,8 +3318,9 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| dirEntry->len = strLen; | |||
| XMEMCPY(dirEntry->name, &input[idx], (size_t)strLen); | |||
| dirEntry->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)dirEntry->name, &input[idx], | |||
| (size_t)strLen); | |||
| ((char *)(wc_ptr_t)dirEntry->name)[strLen] = '\0'; | |||
| dirEntry->next = cert->altDirNames; | |||
| cert->altDirNames = dirEntry; | |||
|
|
|||
| @@ -3353,8 +3356,9 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| emailEntry->len = strLen; | |||
| XMEMCPY(emailEntry->name, &input[idx], (size_t)strLen); | |||
| emailEntry->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)emailEntry->name, &input[idx], | |||
| (size_t)strLen); | |||
| ((char *)(wc_ptr_t)emailEntry->name)[strLen] = '\0'; | |||
|
|
|||
| emailEntry->next = cert->altEmailNames; | |||
| cert->altEmailNames = emailEntry; | |||
| @@ -3436,8 +3440,9 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| uriEntry->len = strLen; | |||
| XMEMCPY(uriEntry->name, &input[idx], (size_t)strLen); | |||
| uriEntry->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)uriEntry->name, &input[idx], | |||
| (size_t)strLen); | |||
| ((char *)(wc_ptr_t)uriEntry->name)[strLen] = '\0'; | |||
|
|
|||
| AddAltName(cert, uriEntry); | |||
|
|
|||
| @@ -3479,12 +3484,13 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| ipAddr->len = strLen; | |||
| XMEMCPY(ipAddr->name, &input[idx], strLen); | |||
| ipAddr->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)ipAddr->name, &input[idx], strLen); | |||
| ((char *)(wc_ptr_t)ipAddr->name)[strLen] = '\0'; | |||
|
|
|||
| if (GenerateDNSEntryIPString(ipAddr, cert->heap) != 0) { | |||
| WOLFSSL_MSG("\tOut of Memory for IP string"); | |||
| XFREE(ipAddr->name, cert->heap, DYNAMIC_TYPE_ALTNAME); | |||
| XFREE((void *)(wc_ptr_t)ipAddr->name, cert->heap, | |||
| DYNAMIC_TYPE_ALTNAME); | |||
| XFREE(ipAddr, cert->heap, DYNAMIC_TYPE_ALTNAME); | |||
| return MEMORY_E; | |||
| } | |||
| @@ -3529,12 +3535,13 @@ static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert) | |||
| return MEMORY_E; | |||
| } | |||
| rid->len = strLen; | |||
| XMEMCPY(rid->name, &input[idx], strLen); | |||
| rid->name[strLen] = '\0'; | |||
| XMEMCPY((void *)(wc_ptr_t)rid->name, &input[idx], strLen); | |||
| ((char *)(wc_ptr_t)rid->name)[strLen] = '\0'; | |||
|
|
|||
| if (GenerateDNSEntryRIDString(rid, cert->heap) != 0) { | |||
| WOLFSSL_MSG("\tOut of Memory for registered Id string"); | |||
| XFREE(rid->name, cert->heap, DYNAMIC_TYPE_ALTNAME); | |||
| XFREE((void *)(wc_ptr_t)rid->name, cert->heap, | |||
| DYNAMIC_TYPE_ALTNAME); | |||
| XFREE(rid, cert->heap, DYNAMIC_TYPE_ALTNAME); | |||
| return MEMORY_E; | |||
| } | |||
There was a problem hiding this comment.
🔶 [Medium] Memory leak: asn_orig.c allocations missing nameStored/ipStringStored/ridStringStored flags
Category: Resource leaks
The PR changes FreeAltNames() in asn.c to conditionally free altNames->name only when altNames->nameStored is set, and similarly for ipString/ipStringStored and ridString/ridStringStored. However, the asn_orig.c functions DecodeAltNames() and DecodeConstructedOtherName() allocate memory for dnsEntry->name (via XMALLOC), ipAddr->name, and rid->name but never set the corresponding nameStored, ipStringStored, or ridStringStored flags to 1. If FreeAltNames() from asn.c is used to free entries created by asn_orig.c code paths, the allocated name, ipString, and ridString buffers will never be freed, causing memory leaks. The asn_orig.c diff shows casts were added to XMEMCPY/XFREE calls for -Wcast-qual hygiene, but the nameStored = 1 assignments that are present in the asn.c version of SetDNSEntry() were not added to the asn_orig.c allocations. For comparison, SetDNSEntry() in asn.c (line 13880) correctly sets dnsEntry->nameStored = 1 after allocation, and GenerateDNSEntryIPString() sets entry->ipStringStored = 1 (line 13698), but no equivalent assignments appear in asn_orig.c.
/* In asn_orig.c DecodeAltNames - allocates name but no nameStored flag: */
ipAddr->len = strLen;
XMEMCPY((void *)(wc_ptr_t)ipAddr->name, &input[idx], strLen);
((char *)(wc_ptr_t)ipAddr->name)[strLen] = '\0';
/* In asn.c FreeAltNames - now conditionally frees: */
if (altNames->nameStored) {
XFREE((void *)(wc_ptr_t)altNames->name, heap, DYNAMIC_TYPE_ALTNAME);
altNames->nameStored = 0;
}Recommendation: Add nameStored = 1 after each successful XMALLOC of dnsEntry->name (and similar entries) in asn_orig.c DecodeAltNames() and DecodeConstructedOtherName(). Similarly add ipStringStored = 1 after GenerateDNSEntryIPString() succeeds and ridStringStored = 1 after GenerateDNSEntryRIDString() succeeds within asn_orig.c.
wolfcrypt/src/tfm.c and wolfssl/wolfcrypt/tfm.h: fix for -Wdiscarded-qualifiers in ecc_check_order_minus_1(). wolfssl/wolfcrypt/types.h: in WC_BARRIER(), use XFENCE() too, for best possible barrier. fixes an ARM32 -Ofast -Wmaybe-uninitialized in blake2s_init_key(). wolfcrypt/src/asn_orig.c: set Stored flag after each allocation of a member that needs it. wolfcrypt/src/signature.c: in wc_SignatureGetSize(), provide for legacy FIPS non-const-arg wc_ecc_sig_size() and wc_RsaEncryptSize().
…l/wolfcrypt/wc_port.h, and strengthen it.
…s to mp_cmp() and mp_cmp_mag().
…-based workaround for ARM32 gcc optimizer bug.
Fixes for
-Wcast-qualhygiene in wolfCrypt..github/workflows/wolfCrypt-Wconversion.yml: Add-Wcast-qualto all scenarios.wolfssl/wolfcrypt/signature.h,wolfcrypt/src/signature.c,doc/dox_comments/header_files/signature.h:remove incorrect const qualifier on the key argument in
wc_SignatureVerifyHash()wc_SignatureVerify()wc_SignatureGenerateHash()wc_SignatureGenerateHash_ex()wc_SignatureGenerate()wc_SignatureGenerate_ex()This fixes UB code patterns throughout
signature.c.keyis inherentlyaccessed readwrite by the underlying low level crypto. Fortunately, wolfCrypt
has no APIs/methods to allow actual
constMPI key objects, therefore theseseeming breaking API changes can't actually break any users.
globally:
Add
constqualifiers to allstructpointer members that are assigned valuescomputed from
constpointers.Add
constqualifiers to intermediate casts for accessors and read-onlydereference constructs, as needed for
-Wcast-qualhygiene, e.g. for a macroGET_U16(a), use(*(const word16*)(a))rather than(*(word16*)(a)).Add
constqualifiers to internal declarations, and remove illegal casts, asneeded for
-Wcast-qualhygiene.Add missing
constqualifiers to all casts for argument, operand, andassignment type agreement, as needed for
-Wcast-qualhygiene, e.g.*data = (const byte*)dataASN->data.ref.datarather than*data = (byte*)dataASN->data.ref.data.wolfssl/wolfcrypt/asn.h,wolfssl/wolfcrypt/asn_public.h,wolfcrypt/src/asn.c,wolfcrypt/src/asn_orig.c:Add additional lifecycle management for object members that are only sometimes locally allocated:
DNS_entry.nameStoredDNS_entry.ipStringStoredDNS_entry.ridStringStoredwolfssl/wolfcrypt/types.h: addWC_BARRIER()macro -- a portable construct thatprevents compiler optimizers from reordering operations across the barrier.
wolfssl/wolfcrypt/blake2-impl.h,wolfcrypt/src/blake2s.c,wolfcrypt/src/blake2b.c:In
blake2b_init(),blake2b_init_key(),blake2s_init(), andblake2s_init_key(), refactorblake2b_paraminitialization usingWC_BARRIER()(fixes
volatileabuse that triggered-Wcast-qual).Remove the residual and unused
WOLFSSL_BLAKE2[BS]_INIT_EACH_FIELDcode.wolfcrypt/src/ecc.candwolfssl/wolfcrypt/ecc.h:Remove incorrect
constqualifier on curve arg towc_ecc_free_curve()(internal function).Tested with
with
-Wcast-qualadded to the non-FIPS.*Wconversion.*scenarios.