crypto.c: Use of -O3 option for compiling produces warnings

[mrdeep1]

With Ubuntu 22.04, doing

$ ./autogen.sh
$ ./configure CFLAGS=-O3

produces warnings about potentially uninitialized variables as per sample

$ make clean ; make crypto.o
gcc -Wall -pedantic -std=c99 -DSHA2_USE_INTTYPES_H -O3 -fPIC -pedantic -Wall -Wextra -Wformat-security -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wunused   -DDTLSv12 -DWITH_SHA256 -DDTLS_CHECK_CONTENTTYPE -I.  -c -o crypto.o crypto.c
In file included from crypto.c:34:
In function ‘ecc_ecdh’,
    inlined from ‘dtls_ecdh_pre_master_secret’ at crypto.c:455:3:
ecc/ecc.h:50:9: warning: ‘pub_x’ may be used uninitialized [-Wmaybe-uninitialized]
   50 |         ecc_ec_mult(px, py, secret, resultx, resulty);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from crypto.c:34:
crypto.c: In function ‘dtls_ecdh_pre_master_secret’:
ecc/ecc.h:47:6: note: by argument 1 of type ‘const uint32_t *’ {aka ‘const unsigned int *’} to ‘ecc_ec_mult’ declared here
   47 | void ecc_ec_mult(const uint32_t *px, const uint32_t *py, const uint32_t *secret, uint32_t *resultx, uint32_t *resulty);
      |      ^~~~~~~~~~~
crypto.c:442:12: note: ‘pub_x’ declared here
  442 |   uint32_t pub_x[8];
      |            ^~~~~
.....

but using -O2 (the default), all is fine.

[boaks]

Not sure, why the compiler warns.

AFAIK,

dtls_ec_key_to_uint32(pub_key_y, key_size, pub_y);

initialize pub_x. That may be not too easy to detect, though key_size is passed in.

[mrdeep1]

I wonder if it thinks that there is no guarantee on the value of key_size, and hence possibly not all the 8 words of pub_x get preset ahead of time.

[boaks]

Yes, the assumptions about the key_size may be not clear.

[mrdeep1]

Several thoughts here

Need to assert that key_size is not bigger than sizeof(pub_x) etc.
Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates
Is the key size going to only ever going to be 32 (256 bits)?
Is there a need for other key size support?

[boaks]

AFAIK, only secp256r1is supported.
The encoding may be an issue, see PR #58 .

[boaks]

Need to assert that key_size is not bigger than sizeof(pub_x) etc.

Yes.

Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates

Not sure, see my comment above. Requires more analysis.

Is the key size going to only ever going to be 32 (256 bits)?
Is there a need for other key size support?

Not sure. I would consider to add ed25519/x25519 before any other stronger ecc.

[mrdeep1]

Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates

Not sure, see my comment above. Requires more analysis.

Agreed that it does need further checking, but I would think that we are talking about raw keys here, as opposed to the asn.1 representation.

As we have hard-coded the ec key size (in bytes) to be DTLS_EC_KEY_SIZE elsewhere, it should be fine to replace the usage of 8 for size of variables with (DTLS_EC_KEY_SIZE / sizeof(uint32_t)) in the appropriate places and assert(key_size == DTLS_EC_KEY_SIZE) to see if that removes the compile complaints (may also need to replace parameter usage of key_size with DTLS_EC_KEY_SIZE).

[boaks]

We may use ecc.h - keyLengthInBytes / arrayLength consequently. Maybe rename it first?

[mrdeep1]

keyLengthInBytes is not currently in use (but arrayLength is). I certainly prefer upper case #define for clarity that it is not a variable etc. Having 2 (or more) definitions for the same entity is likely to create downstream issues when things get (partially) updated.

[boaks]

Is this still pending or also fixed with #208 ?

[mrdeep1]

Current compilation issue is fixed with #208. I had forgotten about this Issue, but if keysizes get changed, then this needs to get revisited.

[mrdeep1]

Closing for now.