Skip to content

Conversation

gojimmypi
Copy link
Contributor

@gojimmypi gojimmypi commented Mar 30, 2023

Description

A variety of minor cleanup and initialization issues to appease compiler complaints, addressed while developing the updates in my ED25519_SHA2_fix branch. Those changes have grown so numerous, I'm breaking down the updates into manageable PR's. 1/n

  • aes.c - return error if lengths for known-not-defined keylen values encountered.
  • asn.c - return zero (an error when dynamic memory allocation fails) for wc_EncodeSignature param lengths less than or equal to zero
  • cmac.c - check cmac->bufferSz > AES_BLOCK_SIZE in wc_CmacFinal
  • ed25519.c - cleanup. variable init.
  • md5.c - check md5 buffer length in wc_Md5Final.
  • random.c - include <esp_random.h> when ESP-IDF v5 detected. Add comments. Correct spelling of ENTROPY_NUM_WORDS_BITS
  • ripemd.c - check ripemd buffLen is not larger than RIPEMD_BLOCK_SIZE in wc_RipeMdFinal.

Fixes zd# n/a

Testing

I've been using these changes in the Espressif development for some time. Here's a script to run the wolfssl testwolfcrypt from WSL Ubuntu on Windows:

git clone https://github.com/gojimmypi/wolfssl.git wolfssl-pr1
cd wolfssl-pr1
git checkout wolfcrypt-updates

./autogen.sh

# enable-all test 
./configure CC=clang  --enable-all CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

Unit testing

git clone https://github.com/gojimmypi/wolfssl.git wolfssl-6249
cd wolfssl-6249
./autogen.sh
./configure --enable-all --enable-asn=original
make
./scripts/unit.test

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kaleb-himes
Copy link
Contributor

retest this please

@gojimmypi gojimmypi marked this pull request as draft March 30, 2023 16:10
@gojimmypi gojimmypi closed this Mar 31, 2023
@gojimmypi
Copy link
Contributor Author

inadvertently closed; sync with upstream & squashed commits.

re-opening for review.

@gojimmypi gojimmypi reopened this Mar 31, 2023
@gojimmypi gojimmypi self-assigned this Mar 31, 2023
@gojimmypi gojimmypi marked this pull request as ready for review April 1, 2023 04:39
@gojimmypi
Copy link
Contributor Author

Jenkins retest this please


#if !defined(WOLFSSL_AES_128)
if (keylen == 16) {
return BAD_FUNC_ARG;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indents should be 4 spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes. I know that well. My bad for letting this one slip by. I've pushed an update to fix

@dgarske dgarske removed their assignment Apr 11, 2023
{
int ret;
const byte* subKey;
word32 remainder = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You may get a set but not used with 0 init. Recommend removing the =0. This can also move into the else section. Variable declarations must be at top of function or brace section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the init = 0 and left the declaration at the top of the function.

@dgarske dgarske removed their assignment Apr 11, 2023
@dgarske
Copy link
Contributor

dgarske commented Apr 13, 2023

The Jenkins error is:

Testing DEFAULT: --enable-sp-math-all --enable-cryptonly --enable-keygen --enable-smallstack 
wolfcrypt/src/sp_int.c: In function ‘_sp_exptmod_nct’:
wolfcrypt/src/sp_int.c:277:39: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
  277 |                 for (n##ii = 1; n##ii < (c); n##ii++) {                        \
      |                                       ^
wolfcrypt/src/sp_int.c:13245:5: note: in expansion of macro ‘ALLOC_SP_INT_ARRAY’
13245 |     ALLOC_SP_INT_ARRAY(t, m->used * 2 + 1, (size_t)preCnt + 2, err, NULL);
      |     ^~~~~~~~~~~~~~~~~~

However your code doesn't touch it. Consider a rebase to latest master?

@dgarske dgarske removed their assignment Apr 13, 2023
@gojimmypi
Copy link
Contributor Author

The Jenkins error is:

Testing DEFAULT: --enable-sp-math-all --enable-cryptonly --enable-keygen --enable-smallstack 
wolfcrypt/src/sp_int.c: In function ‘_sp_exptmod_nct’:
wolfcrypt/src/sp_int.c:277:39: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
  277 |                 for (n##ii = 1; n##ii < (c); n##ii++) {                        \
      |                                       ^
wolfcrypt/src/sp_int.c:13245:5: note: in expansion of macro ‘ALLOC_SP_INT_ARRAY’
13245 |     ALLOC_SP_INT_ARRAY(t, m->used * 2 + 1, (size_t)preCnt + 2, err, NULL);
      |     ^~~~~~~~~~~~~~~~~~

However your code doesn't touch it. Consider a rebase to latest master?

Good idea. I've refreshed from upstream, but encountered a new error. I suspect trying again might be successful, but perhaps @bandi13 would be interested in reviewing this build timeout error first:

./configure --enable-jobserver=4 CC="clang -fsanitize=address,undefined -g" --enable-all
make[2]: warning: -j4 forced in submake: resetting jobserver mode.
make[3]: warning: -j4 forced in submake: resetting jobserver mode.
make[3]: warning: -j4 forced in submake: resetting jobserver mode.
make[4]: warning: -j4 forced in submake: resetting jobserver mode.
Build timed out (after 120 minutes). Marking the build as aborted.
$ ssh-agent -k
unset SSH_AUTH_SOCK;
unset SSH_AGENT_PID;
echo Agent pid 3837301 killed;
[ssh-agent] Stopped.
Build was aborted

@gojimmypi
Copy link
Contributor Author

Jenkins retest this please

@gojimmypi
Copy link
Contributor Author

Hi @dgarske the refresh from upstream worked. Looks like GitHub has been having some recently problems, perhaps this explains prior unexpected and odd Jenkins failures.

In any case, please review and let me know if you'd like any other changes.

@dgarske dgarske merged commit 98b718f into wolfSSL:master Apr 18, 2023
joeftiger pushed a commit to joeftiger/wolfssl that referenced this pull request Sep 2, 2023
* wolfcrypt polish: init, checks, corrections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants