From d257525360cb98af1b4897e183c45465338becab Mon Sep 17 00:00:00 2001 From: Thom Wiggers Date: Fri, 13 Mar 2020 15:02:24 -0400 Subject: [PATCH] Fix UB in qTESLA Lots of shifts left of signed integers --- crypto_sign/qtesla-p-I/clean/gauss.c | 2 +- crypto_sign/qtesla-p-I/clean/pack.c | 16 ++++++++-------- crypto_sign/qtesla-p-I/clean/poly.c | 24 ++++++++++++------------ crypto_sign/qtesla-p-I/clean/sign.c | 2 +- crypto_sign/qtesla-p-III/clean/gauss.c | 2 +- crypto_sign/qtesla-p-III/clean/pack.c | 20 ++++++++++---------- crypto_sign/qtesla-p-III/clean/poly.c | 24 ++++++++++++------------ crypto_sign/qtesla-p-III/clean/sign.c | 2 +- 8 files changed, 46 insertions(+), 46 deletions(-) diff --git a/crypto_sign/qtesla-p-I/clean/gauss.c b/crypto_sign/qtesla-p-I/clean/gauss.c index 600c90db..452a09d4 100644 --- a/crypto_sign/qtesla-p-I/clean/gauss.c +++ b/crypto_sign/qtesla-p-I/clean/gauss.c @@ -23,7 +23,7 @@ void PQCLEAN_QTESLAPI_CLEAN_sample_gauss_poly(poly z, const uint8_t *seed, uint1 cSHAKE(buf, CHUNK_SIZE * CDT_COLS * sizeof(int32_t), (uint8_t *)NULL, 0, dmsp_bytes, 2, seed, CRYPTO_RANDOMBYTES); ++dmsp; for (size_t i = 0, j = 0; i < CHUNK_SIZE * CDT_COLS; i += 1, j += 4) { - samp[i] = (int32_t)(buf[j] | (buf[j + 1] << 8) | (buf[j + 2] << 16) | (buf[j + 3] << 24)); + samp[i] = (int32_t)(buf[j] | (buf[j + 1] << 8) | (buf[j + 2] << 16) | (int32_t)((uint32_t)buf[j + 3] << 24)); } for (size_t i = 0; i < CHUNK_SIZE; i++) { z[chunk + i] = 0; diff --git a/crypto_sign/qtesla-p-I/clean/pack.c b/crypto_sign/qtesla-p-I/clean/pack.c index 28c2feae..80595dd8 100644 --- a/crypto_sign/qtesla-p-I/clean/pack.c +++ b/crypto_sign/qtesla-p-I/clean/pack.c @@ -75,13 +75,13 @@ void PQCLEAN_QTESLAPI_CLEAN_decode_pk(int32_t *pk, uint8_t *seedA, const uint8_t const uint8_t *a = pk_in; for (i = 0, j = 0; i < PARAM_N * PARAM_K; i += 8, j += 29) { - pk[i ] = (int32_t)(( a[j ] | (a[j + 1] << 8) | (a[j + 2] << 16) | (a[j + 3] << 24) ) & mask29); - pk[i + 1] = (int32_t)(((a[j + 3] >> 5) | (a[j + 4] << 3) | (a[j + 5] << 11) | (a[j + 6] << 19) | (a[j + 7] << 27)) & mask29); + pk[i ] = (int32_t)(( a[j ] | (a[j + 1] << 8) | (a[j + 2] << 16) | (int32_t)((uint32_t)a[j + 3] << 24) ) & mask29); + pk[i + 1] = (int32_t)(((a[j + 3] >> 5) | (a[j + 4] << 3) | (a[j + 5] << 11) | (a[j + 6] << 19) | (int32_t)((uint32_t)a[j + 7] << 27)) & mask29); pk[i + 2] = (int32_t)(((a[j + 7] >> 2) | (a[j + 8] << 6) | (a[j + 9] << 14) | (a[j + 10] << 22) ) & mask29); - pk[i + 3] = (int32_t)(((a[j + 10] >> 7) | (a[j + 11] << 1) | (a[j + 12] << 9) | (a[j + 13] << 17) | (a[j + 14] << 25)) & mask29); - pk[i + 4] = (int32_t)(((a[j + 14] >> 4) | (a[j + 15] << 4) | (a[j + 16] << 12) | (a[j + 17] << 20) | (a[j + 18] << 28)) & mask29); + pk[i + 3] = (int32_t)(((a[j + 10] >> 7) | (a[j + 11] << 1) | (a[j + 12] << 9) | (a[j + 13] << 17) | (int32_t)((uint32_t)a[j + 14] << 25)) & mask29); + pk[i + 4] = (int32_t)(((a[j + 14] >> 4) | (a[j + 15] << 4) | (a[j + 16] << 12) | (a[j + 17] << 20) | (int32_t)((uint32_t)a[j + 18] << 28)) & mask29); pk[i + 5] = (int32_t)(((a[j + 18] >> 1) | (a[j + 19] << 7) | (a[j + 20] << 15) | (a[j + 21] << 23) ) & mask29); - pk[i + 6] = (int32_t)(((a[j + 21] >> 6) | (a[j + 22] << 2) | (a[j + 23] << 10) | (a[j + 24] << 18) | (a[j + 25] << 26)) & mask29); + pk[i + 6] = (int32_t)(((a[j + 21] >> 6) | (a[j + 22] << 2) | (a[j + 23] << 10) | (a[j + 24] << 18) | (int32_t)((uint32_t)a[j + 25] << 26)) & mask29); pk[i + 7] = (int32_t)( (a[j + 25] >> 3) | (a[j + 26] << 5) | (a[j + 27] << 13) | (a[j + 28] << 21) ); } @@ -96,7 +96,7 @@ void PQCLEAN_QTESLAPI_CLEAN_encode_sig(uint8_t *sm, uint8_t *c, const poly z) { for (i = 0, j = 0; i < PARAM_N; i += 2, j += 5) { sm[j ] = (uint8_t)( z[i ] ); sm[j + 1] = (uint8_t)( z[i ] >> 8); - sm[j + 2] = (uint8_t)(((z[i ] >> 16) & 0x0F) | (z[i + 1] << 4)); + sm[j + 2] = (uint8_t)(((z[i ] >> 16) & 0x0F) | (int64_t)((uint64_t)z[i + 1] << 4)); sm[j + 3] = (uint8_t)( z[i + 1] >> 4); sm[j + 4] = (uint8_t)( z[i + 1] >> 12); } @@ -109,8 +109,8 @@ void PQCLEAN_QTESLAPI_CLEAN_decode_sig(uint8_t *c, poly z, const uint8_t *sm) { size_t i, j; for (i = 0, j = 0; i < PARAM_N; i += 2, j += 5) { - z[i ] = sm[j ] | (sm[j + 1] << 8) | (((int64_t)sm[j + 2] << 60) >> 44); - z[i + 1] = (sm[j + 2] >> 4) | (sm[j + 3] << 4) | (((int64_t)sm[j + 4] << 56) >> 44); + z[i ] = sm[j ] | (sm[j + 1] << 8) | ((int64_t)((uint64_t)sm[j + 2] << 60) >> 44); + z[i + 1] = (sm[j + 2] >> 4) | (sm[j + 3] << 4) | ((int64_t)((uint64_t)sm[j + 4] << 56) >> 44); } memcpy(c, &sm[j], CRYPTO_C_BYTES); diff --git a/crypto_sign/qtesla-p-I/clean/poly.c b/crypto_sign/qtesla-p-I/clean/poly.c index 4ab069b7..0eb3d96b 100644 --- a/crypto_sign/qtesla-p-I/clean/poly.c +++ b/crypto_sign/qtesla-p-I/clean/poly.c @@ -215,27 +215,27 @@ void PQCLEAN_QTESLAPI_CLEAN_poly_uniform(poly_k a, const uint8_t *seed) { pos = 0; } val1 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val2 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val3 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val4 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; if (val1 < PARAM_Q && i < PARAM_K * PARAM_N) { diff --git a/crypto_sign/qtesla-p-I/clean/sign.c b/crypto_sign/qtesla-p-I/clean/sign.c index 5f9c6a7c..a1b5b80a 100644 --- a/crypto_sign/qtesla-p-I/clean/sign.c +++ b/crypto_sign/qtesla-p-I/clean/sign.c @@ -77,7 +77,7 @@ static int test_correctness(const poly v) { left = val; val = (val + (1 << (PARAM_D - 1)) - 1) >> PARAM_D; - val = left - (val << PARAM_D); + val = left - (int32_t)((uint32_t)val << PARAM_D); // If (Abs(val) < (1<<(PARAM_D-1))-PARAM_E) then t1 = 0, else t1 = 1 t1 = (uint32_t)(~(Abs(val) - ((1 << (PARAM_D - 1)) - PARAM_E))) >> (RADIX32 - 1); diff --git a/crypto_sign/qtesla-p-III/clean/gauss.c b/crypto_sign/qtesla-p-III/clean/gauss.c index c9cd0f1d..9e0fe05d 100644 --- a/crypto_sign/qtesla-p-III/clean/gauss.c +++ b/crypto_sign/qtesla-p-III/clean/gauss.c @@ -23,7 +23,7 @@ void PQCLEAN_QTESLAPIII_CLEAN_sample_gauss_poly(poly z, const uint8_t *seed, uin cSHAKE(buf, CHUNK_SIZE * CDT_COLS * sizeof(int32_t), (uint8_t *)NULL, 0, dmsp_bytes, 2, seed, CRYPTO_RANDOMBYTES); ++dmsp; for (size_t i = 0, j = 0; i < CHUNK_SIZE * CDT_COLS; i += 1, j += 4) { - samp[i] = (int32_t)(buf[j] | (buf[j + 1] << 8) | (buf[j + 2] << 16) | (buf[j + 3] << 24)); + samp[i] = (int32_t)(buf[j] | (buf[j + 1] << 8) | (buf[j + 2] << 16) | (int32_t)((uint32_t)buf[j + 3] << 24)); } for (size_t i = 0; i < CHUNK_SIZE; i++) { z[chunk + i] = 0; diff --git a/crypto_sign/qtesla-p-III/clean/pack.c b/crypto_sign/qtesla-p-III/clean/pack.c index e3e4cc5f..379ab20d 100644 --- a/crypto_sign/qtesla-p-III/clean/pack.c +++ b/crypto_sign/qtesla-p-III/clean/pack.c @@ -61,9 +61,9 @@ void PQCLEAN_QTESLAPIII_CLEAN_decode_pk(int32_t *pk, uint8_t *seedA, const uint8 const uint8_t *a = pk_in; for (i = 0, j = 0; i < PARAM_N * PARAM_K; i += 4, j += 15) { - pk[i ] = (int32_t)(( a[j ] | (a[j + 1] << 8) | (a[j + 2] << 16) | (a[j + 3] << 24) ) & mask30); - pk[i + 1] = (int32_t)(((a[j + 3] >> 6) | (a[j + 4] << 2) | (a[j + 5] << 10) | (a[j + 6] << 18) | (a[j + 7] << 26)) & mask30); - pk[i + 2] = (int32_t)(((a[j + 7] >> 4) | (a[j + 8] << 4) | (a[j + 9] << 12) | (a[j + 10] << 20) | (a[j + 11] << 28)) & mask30); + pk[i ] = (int32_t)(( a[j ] | (a[j + 1] << 8) | (a[j + 2] << 16) | (int32_t)((uint32_t)a[j + 3] << 24) ) & mask30); + pk[i + 1] = (int32_t)(((a[j + 3] >> 6) | (a[j + 4] << 2) | (a[j + 5] << 10) | (a[j + 6] << 18) | (int32_t)((uint32_t)a[j + 7] << 26)) & mask30); + pk[i + 2] = (int32_t)(((a[j + 7] >> 4) | (a[j + 8] << 4) | (a[j + 9] << 12) | (a[j + 10] << 20) | (int32_t)((uint32_t)a[j + 11] << 28)) & mask30); pk[i + 3] = (int32_t)( (a[j + 11] >> 2) | (a[j + 12] << 6) | (a[j + 13] << 14) | (a[j + 14] << 22) ); } @@ -78,13 +78,13 @@ void PQCLEAN_QTESLAPIII_CLEAN_encode_sig(uint8_t *sm, uint8_t *c, const poly z) for (i = 0, j = 0; i < PARAM_N; i += 4, j += 11) { sm[j ] = (uint8_t)( z[i ] ); sm[j + 1] = (uint8_t)( z[i ] >> 8); - sm[j + 2] = (uint8_t)(((z[i ] >> 16) & 0x3F) | (z[i + 1] << 6)); + sm[j + 2] = (uint8_t)(((z[i ] >> 16) & 0x3F) | ((uint64_t)z[i + 1] << 6)); sm[j + 3] = (uint8_t)( z[i + 1] >> 2); sm[j + 4] = (uint8_t)( z[i + 1] >> 10); - sm[j + 5] = (uint8_t)(((z[i + 1] >> 18) & 0x0F) | (z[i + 2] << 4)); + sm[j + 5] = (uint8_t)(((z[i + 1] >> 18) & 0x0F) | ((uint64_t)z[i + 2] << 4)); sm[j + 6] = (uint8_t)( z[i + 2] >> 4); sm[j + 7] = (uint8_t)( z[i + 2] >> 12); - sm[j + 8] = (uint8_t)(((z[i + 2] >> 20) & 0x03) | (z[i + 3] << 2)); + sm[j + 8] = (uint8_t)(((z[i + 2] >> 20) & 0x03) | ((uint64_t)z[i + 3] << 2)); sm[j + 9] = (uint8_t)( z[i + 3] >> 6); sm[j + 10] = (uint8_t)( z[i + 3] >> 14); } @@ -98,10 +98,10 @@ void PQCLEAN_QTESLAPIII_CLEAN_decode_sig(uint8_t *c, poly z, const uint8_t *sm) size_t i, j; for (i = 0, j = 0; i < PARAM_N; i += 4, j += 11) { - z[i ] = sm[j ] | (sm[j + 1] << 8) | (((int64_t)sm[j + 2] << 58) >> 42); - z[i + 1] = (sm[j + 2] >> 6) | (sm[j + 3] << 2) | (sm[j + 4] << 10) | (((int64_t)sm[j + 5] << 60) >> 42); - z[i + 2] = (sm[j + 5] >> 4) | (sm[j + 6] << 4) | (sm[j + 7] << 12) | (((int64_t)sm[j + 8] << 62) >> 42); - z[i + 3] = (sm[j + 8] >> 2) | (sm[j + 9] << 6) | (((int64_t)sm[j + 10] << 56) >> 42); + z[i ] = sm[j ] | (sm[j + 1] << 8) | ((int64_t)((uint64_t)sm[j + 2] << 58) >> 42); + z[i + 1] = (sm[j + 2] >> 6) | (sm[j + 3] << 2) | (sm[j + 4] << 10) | ((int64_t)((uint64_t)sm[j + 5] << 60) >> 42); + z[i + 2] = (sm[j + 5] >> 4) | (sm[j + 6] << 4) | (sm[j + 7] << 12) | ((int64_t)((uint64_t)sm[j + 8] << 62) >> 42); + z[i + 3] = (sm[j + 8] >> 2) | (sm[j + 9] << 6) | ((int64_t)((uint64_t)sm[j + 10] << 56) >> 42); } memcpy(c, &sm[j], CRYPTO_C_BYTES); diff --git a/crypto_sign/qtesla-p-III/clean/poly.c b/crypto_sign/qtesla-p-III/clean/poly.c index fa6777f3..e0cb51f4 100644 --- a/crypto_sign/qtesla-p-III/clean/poly.c +++ b/crypto_sign/qtesla-p-III/clean/poly.c @@ -206,27 +206,27 @@ void PQCLEAN_QTESLAPIII_CLEAN_poly_uniform(poly_k a, const uint8_t *seed) { pos = 0; } val1 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val2 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val3 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; val4 = ((uint32_t)(buf[pos]) - | (uint32_t)(buf[pos + 1] << 8) - | (uint32_t)(buf[pos + 2] << 16) - | (uint32_t)(buf[pos + 3] << 24)) + | ((uint32_t)buf[pos + 1] << 8) + | ((uint32_t)buf[pos + 2] << 16) + | ((uint32_t)buf[pos + 3] << 24)) & mask; pos += nbytes; if (val1 < PARAM_Q && i < PARAM_K * PARAM_N) { diff --git a/crypto_sign/qtesla-p-III/clean/sign.c b/crypto_sign/qtesla-p-III/clean/sign.c index d548f7e0..ab37d18e 100644 --- a/crypto_sign/qtesla-p-III/clean/sign.c +++ b/crypto_sign/qtesla-p-III/clean/sign.c @@ -77,7 +77,7 @@ static int test_correctness(const poly v) { left = val; val = (val + (1 << (PARAM_D - 1)) - 1) >> PARAM_D; - val = left - (val << PARAM_D); + val = left - (int32_t)((uint32_t)val << PARAM_D); // If (Abs(val) < (1<<(PARAM_D-1))-PARAM_E) then t1 = 0, else t1 = 1 t1 = (uint32_t)(~(Abs(val) - ((1 << (PARAM_D - 1)) - PARAM_E))) >> (RADIX32 - 1);