Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10203
Scan targets checked: wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-consttime, wolfcrypt-defaults, wolfcrypt-mutation, wolfcrypt-portability, wolfcrypt-proptest, wolfcrypt-src, wolfcrypt-zeroize
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
force-pushed
the
pkcs7_fixes
branch
2 times, most recently
from
52b6384 to
f381fda
Compare
|
Jenkins retest this please |
|
Frauschi
force-pushed
the
pkcs7_fixes
branch
2 times, most recently
from
5e42078 to
14c9823
Compare
|
Jenkins retest this please |
|
|
||
| /* free any buffers that may be allocated */ | ||
| if (pkcs7->stream->aad != NULL && pkcs7->stream->aadSz > 0) | ||
| ForceZero(pkcs7->stream->aad, pkcs7->stream->aadSz); |
There was a problem hiding this comment.
Since we are adding ForceZeros then we should put in wc_MemZero_Add calls in constructor.
If memory is dynamic then it will be checked in the free, otherwise you need a wc_MemZero_Check() in the destructor.
| } | ||
|
|
||
| if (ret != 0) { | ||
| ForceZero(secret, secretSz); |
There was a problem hiding this comment.
Add a wc_MemZero_Add after malloc.
| * whether RSA PKCS#1 v1.5 padding was valid. */ | ||
| { | ||
| byte haveSrc = ctMaskGTE(realLen, 1); | ||
| uintptr_t srcVal = |
There was a problem hiding this comment.
Old compilers may not know unitptr_t.
There was a problem hiding this comment.
fixed by modifying the code to only use our own wc_ptr_t
| * instead of src[j] to avoid out-of-bounds | ||
| * access. The result is masked to zero by | ||
| * inBounds anyway. */ | ||
| word32 safeJ = j & (word32)(0x00U - (word32)( |
There was a problem hiding this comment.
The pattern I use is to increment safeJ if it doesn't take us out of bounds.
Same result but cleaner I think.
There was a problem hiding this comment.
Fixed (hopefully) as suggested.
| * and real paths. Downstream decryption uses | ||
| * blockKeySz from the content algorithm, not this | ||
| * value, so the extra bytes are harmless. */ | ||
| *decryptedKeySz = (word32)sizeof(fakeKey); |
There was a problem hiding this comment.
This seems wrong.
Where does it hurt to use the real value?
There was a problem hiding this comment.
Fixed. realLen is now returned in case of success.
| ForceZero(realPad, sizeof(realPad)); | ||
| } | ||
| ForceZero(fakeKey, sizeof(fakeKey)); | ||
| ForceZero(encryptedKey, (word32)encryptedKeySz); |
There was a problem hiding this comment.
Why ForceZero the encrypted key?
There was a problem hiding this comment.
wc_RsaPrivateDecryptInline() decrypts the encrypted key inline in the buffer. So the buffer actually contains the decrypted key, so it must be zeroed.
There was a problem hiding this comment.
A comment is added that explains this in the code
| wc_FreeRng(rng); | ||
| } | ||
| if (ret != 0) { | ||
| ForceZero(seed, sizeof(seed)); |
There was a problem hiding this comment.
seed doesn't contain anything at this point
Also add missing ForceZero for ECDH shared secret on the heap.
4 participants
Fixes for various issues found in PKCS#7 code.
Fixes zd21593, F-2683, F-2684, F-2686, F-1552, F-1990, F-2681, F-2685, F-1991, F-1992, F-2679, F-2680. Also fixes a regression when building with
--enable-allandNO_PKCS7_STREAM.