From 8fe63e56543982eae265fde8384a3a14a3b93eaf Mon Sep 17 00:00:00 2001 From: roconnor-blockstream Date: Wed, 3 Jul 2019 11:23:20 -0400 Subject: [PATCH 1/2] Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands. --- src/scalar_low_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index c80e70c5a2ad2..5dbc35604c21c 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -38,7 +38,7 @@ static int secp256k1_scalar_add(secp256k1_scalar *r, const secp256k1_scalar *a, static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int flag) { if (flag && bit < 32) - *r += (1 << bit); + *r += ((uint32_t)1 << bit); #ifdef VERIFY VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0); #endif From 0d82732a9a16cecc445e61c718ce9bdc2d228e76 Mon Sep 17 00:00:00 2001 From: Russell O'Connor Date: Fri, 5 Jul 2019 00:30:36 -0400 Subject: [PATCH 2/2] Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow. --- src/scalar_low_impl.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/scalar_low_impl.h b/src/scalar_low_impl.h index 5dbc35604c21c..910ce3f4933d9 100644 --- a/src/scalar_low_impl.h +++ b/src/scalar_low_impl.h @@ -40,6 +40,9 @@ static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int if (flag && bit < 32) *r += ((uint32_t)1 << bit); #ifdef VERIFY + VERIFY_CHECK(bit < 32); + /* Verify that adding (1 << bit) will not overflow any in-range scalar *r by overflowing the underlying uint32_t. */ + VERIFY_CHECK(((uint32_t)1 << bit) - 1 <= UINT32_MAX - EXHAUSTIVE_TEST_ORDER); VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0); #endif }