Merge dashpay#647: Increase robustness against UB in secp256k1_scalar…

…_cadd_bit

0d82732 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. (Russell O'Connor)
8fe63e5 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. (roconnor-blockstream)

Pull request description:

  Avoid possible, but unlikely undefined behaviour in `scalar_low_impl`'s `secp256k1_scalar_cadd_bit`.
  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.

  Using the unsigned literal `1u` addresses the issue.

ACKs for commit 0d8273:
  real-or-random:
    ACK 0d82732
  jonasnick:
    ACK 0d82732

Tree-SHA512: 905be3b8b00aa5cc9bd6dabb543745119da8f34181d37765071f28abbc1d6ff3659e3f195b72c2f2d003006678823919668bc0d169ac8b8d4bcc5da671813c99

src/scalar_low_impl.h

@@ -38,8 +38,11 @@ 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(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
 }