From 7f53bcc4d09ceee231f6f93f54ce4b6958c838f9 Mon Sep 17 00:00:00 2001 From: Daniel Pouzzner Date: Thu, 4 Jan 2024 15:53:04 -0600 Subject: [PATCH] fixes for clang-tidy reported defects and misstylings --with-liboqs: * readability-named-parameter (style) * bugprone-sizeof-expression (true bugs) * clang-analyzer-deadcode.DeadStores (true bugs) * clang-analyzer-core.NonNullParamChecker (true bug) * clang-diagnostic-newline-eof (style) * clang-diagnostic-shorten-64-to-32 (true but benign in practice) fixes for sanitizer reported defects --with-liboqs: null pointer memcpy()s in TLSX_KeyShare_GenPqcKey() and server_generate_pqc_ciphertext(). fixes for silent crypto-critical failure in wolfSSL_liboqsGetRandomData(): refactor to accommodate oversize numOfBytes, and abort() if wc_RNG_GenerateBlock() returns failure. --- src/tls.c | 6 ++++-- wolfcrypt/benchmark/benchmark.c | 4 +++- wolfcrypt/src/dilithium.c | 4 ++-- wolfcrypt/src/falcon.c | 4 ++-- wolfcrypt/src/port/liboqs/liboqs.c | 22 +++++++++++++++++++--- wolfcrypt/src/sphincs.c | 10 +++++----- wolfssl/wolfcrypt/dilithium.h | 2 +- wolfssl/wolfcrypt/falcon.h | 2 +- wolfssl/wolfcrypt/kyber.h | 6 +++--- wolfssl/wolfcrypt/port/liboqs/liboqs.h | 2 +- wolfssl/wolfcrypt/sphincs.h | 2 +- 11 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/tls.c b/src/tls.c index a2f0e8782..ddd0e9599 100644 --- a/src/tls.c +++ b/src/tls.c @@ -7722,7 +7722,8 @@ static int TLSX_KeyShare_GenPqcKey(WOLFSSL *ssl, KeyShareEntry* kse) ret = wc_KyberKey_EncodePrivateKey(kem, privKey, privSz); } if (ret == 0) { - XMEMCPY(pubKey, ecc_kse->pubKey, ecc_kse->pubKeyLen); + if (ecc_kse->pubKeyLen > 0) + XMEMCPY(pubKey, ecc_kse->pubKey, ecc_kse->pubKeyLen); kse->pubKey = pubKey; kse->pubKeyLen = ecc_kse->pubKeyLen + pubSz; pubKey = NULL; @@ -9010,7 +9011,8 @@ static int server_generate_pqc_ciphertext(WOLFSSL* ssl, keyShareEntry->keLen = outlen + ssSz; sharedSecret = NULL; - XMEMCPY(ciphertext, ecc_kse->pubKey, ecc_kse->pubKeyLen); + if (ecc_kse->pubKeyLen > 0) + XMEMCPY(ciphertext, ecc_kse->pubKey, ecc_kse->pubKeyLen); keyShareEntry->pubKey = ciphertext; keyShareEntry->pubKeyLen = (word32)(ecc_kse->pubKeyLen + ctSz); ciphertext = NULL; diff --git a/wolfcrypt/benchmark/benchmark.c b/wolfcrypt/benchmark/benchmark.c index 2fd474dbc..05414b486 100644 --- a/wolfcrypt/benchmark/benchmark.c +++ b/wolfcrypt/benchmark/benchmark.c @@ -12939,7 +12939,9 @@ int wolfcrypt_benchmark_main(int argc, char** argv) /* Both bench_pq_asym_opt and bench_pq_asym_opt2 are looking for * -pq, so we need to do a special case for -pq since optMatched * was set to 1 just above. */ - if (string_matches(argv[1], bench_pq_asym_opt[0].str)) { + if ((bench_pq_asym_opt[0].str != NULL) && + string_matches(argv[1], bench_pq_asym_opt[0].str)) + { bench_pq_asym_algs2 |= bench_pq_asym_opt2[0].val; bench_all = 0; optMatched = 1; diff --git a/wolfcrypt/src/dilithium.c b/wolfcrypt/src/dilithium.c index f03e8b6f4..93e79e965 100644 --- a/wolfcrypt/src/dilithium.c +++ b/wolfcrypt/src/dilithium.c @@ -205,7 +205,7 @@ int wc_dilithium_init(dilithium_key* key) return BAD_FUNC_ARG; } - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); return 0; } @@ -258,7 +258,7 @@ int wc_dilithium_get_level(dilithium_key* key, byte* level) void wc_dilithium_free(dilithium_key* key) { if (key != NULL) { - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); } } diff --git a/wolfcrypt/src/falcon.c b/wolfcrypt/src/falcon.c index b1cb22949..ae9947ef8 100644 --- a/wolfcrypt/src/falcon.c +++ b/wolfcrypt/src/falcon.c @@ -197,7 +197,7 @@ int wc_falcon_init(falcon_key* key) return BAD_FUNC_ARG; } - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); return 0; } @@ -250,7 +250,7 @@ int wc_falcon_get_level(falcon_key* key, byte* level) void wc_falcon_free(falcon_key* key) { if (key != NULL) { - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); } } diff --git a/wolfcrypt/src/port/liboqs/liboqs.c b/wolfcrypt/src/port/liboqs/liboqs.c index 41dd81a42..256833052 100644 --- a/wolfcrypt/src/port/liboqs/liboqs.c +++ b/wolfcrypt/src/port/liboqs/liboqs.c @@ -33,6 +33,7 @@ implementations for Post-Quantum cryptography algorithms. #include #include +#include #include #include @@ -50,9 +51,24 @@ static int liboqs_init = 0; static void wolfSSL_liboqsGetRandomData(uint8_t* buffer, size_t numOfBytes) { - int ret = wc_RNG_GenerateBlock(liboqsCurrentRNG, buffer, (word32)numOfBytes); - if (ret != 0) { - // ToDo: liboqs exits programm if RNG fails, not sure what to do here + int ret; + word32 numOfBytes_word32; + + while (numOfBytes > 0) { + numOfBytes_word32 = (word32)numOfBytes; + numOfBytes -= numOfBytes_word32; + ret = wc_RNG_GenerateBlock(liboqsCurrentRNG, buffer, + numOfBytes_word32); + if (ret != 0) { + /* ToDo: liboqs exits programm if RNG fails, + * not sure what to do here + */ + WOLFSSL_MSG_EX( + "wc_RNG_GenerateBlock(..., %u) failed with ret %d " + "in wolfSSL_liboqsGetRandomData().", numOfBytes_word32, ret + ); + abort(); + } } } diff --git a/wolfcrypt/src/sphincs.c b/wolfcrypt/src/sphincs.c index 695e8aa8e..e26ab5181 100644 --- a/wolfcrypt/src/sphincs.c +++ b/wolfcrypt/src/sphincs.c @@ -243,7 +243,7 @@ int wc_sphincs_init(sphincs_key* key) return BAD_FUNC_ARG; } - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); return 0; } @@ -308,7 +308,7 @@ int wc_sphincs_get_level_and_optim(sphincs_key* key, byte* level, byte* optim) void wc_sphincs_free(sphincs_key* key) { if (key != NULL) { - ForceZero(key, sizeof(key)); + ForceZero(key, sizeof(*key)); } } @@ -857,7 +857,7 @@ int wc_Sphincs_PrivateKeyDecode(const byte* input, word32* inOutIdx, else if ((key->level == 5) && (key->optim == FAST_VARIANT)) { keytype = SPHINCS_FAST_LEVEL5k; } - if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { + else if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { keytype = SPHINCS_SMALL_LEVEL1k; } else if ((key->level == 3) && (key->optim == SMALL_VARIANT)) { @@ -905,7 +905,7 @@ int wc_Sphincs_PublicKeyDecode(const byte* input, word32* inOutIdx, else if ((key->level == 5) && (key->optim == FAST_VARIANT)) { keytype = SPHINCS_FAST_LEVEL5k; } - if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { + else if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { keytype = SPHINCS_SMALL_LEVEL1k; } else if ((key->level == 3) && (key->optim == SMALL_VARIANT)) { @@ -960,7 +960,7 @@ int wc_Sphincs_PublicKeyToDer(sphincs_key* key, byte* output, word32 inLen, else if ((key->level == 5) && (key->optim == FAST_VARIANT)) { keytype = SPHINCS_FAST_LEVEL5k; } - if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { + else if ((key->level == 1) && (key->optim == SMALL_VARIANT)) { keytype = SPHINCS_SMALL_LEVEL1k; } else if ((key->level == 3) && (key->optim == SMALL_VARIANT)) { diff --git a/wolfssl/wolfcrypt/dilithium.h b/wolfssl/wolfcrypt/dilithium.h index 896d06ac6..2b5998ffd 100644 --- a/wolfssl/wolfcrypt/dilithium.h +++ b/wolfssl/wolfcrypt/dilithium.h @@ -110,7 +110,7 @@ int wc_dilithium_import_private_key(const byte* priv, word32 privSz, dilithium_key* key); WOLFSSL_API -int wc_dilithium_export_public(dilithium_key*, byte* out, word32* outLen); +int wc_dilithium_export_public(dilithium_key* key, byte* out, word32* outLen); WOLFSSL_API int wc_dilithium_export_private_only(dilithium_key* key, byte* out, word32* outLen); WOLFSSL_API diff --git a/wolfssl/wolfcrypt/falcon.h b/wolfssl/wolfcrypt/falcon.h index e15fc9544..c444a302b 100644 --- a/wolfssl/wolfcrypt/falcon.h +++ b/wolfssl/wolfcrypt/falcon.h @@ -105,7 +105,7 @@ int wc_falcon_import_private_key(const byte* priv, word32 privSz, falcon_key* key); WOLFSSL_API -int wc_falcon_export_public(falcon_key*, byte* out, word32* outLen); +int wc_falcon_export_public(falcon_key* key, byte* out, word32* outLen); WOLFSSL_API int wc_falcon_export_private_only(falcon_key* key, byte* out, word32* outLen); WOLFSSL_API diff --git a/wolfssl/wolfcrypt/kyber.h b/wolfssl/wolfcrypt/kyber.h index 7ac4ebd6e..5132e1276 100644 --- a/wolfssl/wolfcrypt/kyber.h +++ b/wolfssl/wolfcrypt/kyber.h @@ -49,12 +49,12 @@ /* Size of a polynomial vector based on dimensions. */ -#define KYBER_POLY_VEC_SZ(k) (k * KYBER_POLY_SIZE) +#define KYBER_POLY_VEC_SZ(k) ((k) * KYBER_POLY_SIZE) /* Size of a compressed polynomial based on bits per coefficient. */ -#define KYBER_POLY_COMPRESSED_SZ(b) (b * (KYBER_N / 8)) +#define KYBER_POLY_COMPRESSED_SZ(b) ((b) * (KYBER_N / 8)) /* Size of a compressed vector polynomial based on dimensions and bits per * coefficient. */ -#define KYBER_POLY_VEC_COMPRESSED_SZ(k, b) (k * (b * (KYBER_N / 8))) +#define KYBER_POLY_VEC_COMPRESSED_SZ(k, b) ((k) * ((b) * (KYBER_N / 8))) /* Kyber-512 parameters */ diff --git a/wolfssl/wolfcrypt/port/liboqs/liboqs.h b/wolfssl/wolfcrypt/port/liboqs/liboqs.h index b7a57ca0d..d93ec2f2b 100644 --- a/wolfssl/wolfcrypt/port/liboqs/liboqs.h +++ b/wolfssl/wolfcrypt/port/liboqs/liboqs.h @@ -57,4 +57,4 @@ int wolfSSL_liboqsRngMutexUnlock(void); } /* extern "C" */ #endif -#endif /* WOLF_CRYPT_LIBOQS_H */ \ No newline at end of file +#endif /* WOLF_CRYPT_LIBOQS_H */ diff --git a/wolfssl/wolfcrypt/sphincs.h b/wolfssl/wolfcrypt/sphincs.h index b1533bee4..84871f538 100644 --- a/wolfssl/wolfcrypt/sphincs.h +++ b/wolfssl/wolfcrypt/sphincs.h @@ -125,7 +125,7 @@ int wc_sphincs_import_private_key(const byte* priv, word32 privSz, sphincs_key* key); WOLFSSL_API -int wc_sphincs_export_public(sphincs_key*, byte* out, word32* outLen); +int wc_sphincs_export_public(sphincs_key* key, byte* out, word32* outLen); WOLFSSL_API int wc_sphincs_export_private_only(sphincs_key* key, byte* out, word32* outLen); WOLFSSL_API