ECC Enc test: fix ecc_ctx_kdf_salt_test #6083
Conversation
Test had uninitialized memory read as the plaintext buffer wasn't memset to AES block size. Test leaked when HAVE_WOLF_BIGINT is defined as keys weren't freed before initialization. Test function only compiled when AES-CTR compiled in when it uses AES-CBC. Made function return a unique value. Other tidying up.
SparkiDev
force-pushed
the
ecc_enc_test_leak
branch
from
815966f
to
ce84c77
Compare
|
|
||
| ret = wc_ecc_make_key(rng, 32, a); |
There was a problem hiding this comment.
why isn't this conditional on (ret == 0)?
| } | ||
| plaintextLen = (((word32)XSTRLEN(message) + AES_BLOCK_SIZE - 1) / | ||
| AES_BLOCK_SIZE) * AES_BLOCK_SIZE; | ||
| XMEMSET(plaintext + XSTRLEN(message), 0, plaintextLen - XSTRLEN(message)); |
There was a problem hiding this comment.
sanitizer is not happy with this construction. probably just do what I did in #6081 -- pre-zero plaintext in its entirety, XMEMSET(plaintext, 0, 128); I retested with that change and it was fine.
In file included from /usr/include/string.h:535,
from ./wolfssl/wolfcrypt/types.h:623,
from wolfcrypt/test/test.c:34:
In function ‘memset’,
inlined from ‘ecc_ctx_kdf_salt_test’ at wolfcrypt/test/test.c:26666:5,
338d8db274 (<106998124+jpbland1@users.noreply.github.com> 2023-02-10 13:05:52 -0500 27333) ret = ecc_ctx_kdf_salt_test(&rng, userA, userB);
inlined from ‘ecc_encrypt_test’ at wolfcrypt/test/test.c:27333:15:
/usr/include/bits/string_fortified.h:59:10: error: ‘__builtin_memset’ offset [0, 1] is out of the bounds [0, 0] [-Werror=array-bounds]
59 | return __builtin___memset_chk (__dest, __ch, __len,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
60 | __glibc_objsize0 (__dest));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/bits/string_fortified.h:59:10: error: ‘__builtin_memset’ offset [0, 1] is out of the bounds [0, 0] [-Werror=array-bounds]
found by the sanitizer-all-asm-smallstack-async test, i.e. '--enable-all' '--enable-asynccrypt' '--enable-intelasm' '--enable-sp-math-all' '--enable-smallstack' '--enable-smallstackcache' 'CPPFLAGS=-pedantic' 'CC=gcc' 'CFLAGS=-g -fno-omit-frame-pointer -fsanitize=address,pointer-subtract,leak,undefined,float-cast-overflow,float-divide-by-zero,bounds-strict -fsanitize-recover=all --param=max-vartrack-size=128000000 -march=native' 'LDFLAGS=-g -fno-omit-frame-pointer -fsanitize-recover=all -fsanitize=address,pointer-subtract,leak,undefined,float-cast-overflow,float-divide-by-zero,bounds-strict -fno-sanitize-recover=all '
the async part has nothing to do with it AFAIK -- all about the smallstack.
There was a problem hiding this comment.
|
I've just reopened #6081 and updated it with everything from this PR. There's a crucial fips gating fix in #6081 so I'll close this one provisionally. Btw it turns out if you "reopen" a PR, it immediately kicks off a CI run even if the head of the PR already went through CI and everything passed. So, note to self, always push to a closed PR before reopening it. |
|
#6081 is merged now, with the additional fixes from #6083. I ran |
Description
Test had uninitialized memory read as the plaintext buffer wasn't memset to AES block size.
Test leaked when HAVE_WOLF_BIGINT is defined as keys weren't freed before initialization.
Test function only compiled when AES-CTR compiled in when it uses AES-CBC.
Made function return a unique value.
Other tidying up.
Testing
./configure '--disable-shared' 'CFLAGS=-DHAVE_WOLF_BIGINT' '--enable-eccencrypt' '--enable-debug' '--disable-dh' '--disable-rsa'
valgrind ./wolfcrypt/test/testwolfcrypt
Need --enable-aesctr to make unmodified code compile the function: ecc_ctx_kdf_salt_test()
Checklist