Merge bitcoin#648: Prevent ints from wrapping around in scratch space functions

60f7f2d Don't assume that ALIGNMENT > 1 in tests (Tim Ruffing)
ada6361 Use ROUND_TO_ALIGN in scratch_create (Jonas Nick)
8ecc6ce Add check preventing rounding to alignment from wrapping around in scratch_alloc (Jonas Nick)
4edaf06 Add check preventing integer multiplication wrapping around in scratch_max_allocation (Jonas Nick)

Pull request description:

  This PR increases the general robustness of scratch spaces. It does not fix an existing vulnerability because scratch spaces aren't used anywhere in master. Additionally,  it must be prevented anyway that an attacker has (indirect) control over the arguments touched in this PR.

ACKs for top commit:
  sipa:
    ACK 60f7f2d

Tree-SHA512: ecdd794b55a01d1d6d24098f3abff34cb8bb6f33737ec4ec93714aa631c9d397b213cc3603a916ad79f4b09d6b2f8973bf87fc07b81b25a530cc72c4dbafaba9

Author: real-or-random

src/scratch_impl.h

@@ -11,7 +11,7 @@
 #include "scratch.h"
 
 static secp256k1_scratch* secp256k1_scratch_create(const secp256k1_callback* error_callback, size_t size) {
-    const size_t base_alloc = ((sizeof(secp256k1_scratch) + ALIGNMENT - 1) / ALIGNMENT) * ALIGNMENT;
+    const size_t base_alloc = ROUND_TO_ALIGN(sizeof(secp256k1_scratch));
     void *alloc = checked_malloc(error_callback, base_alloc + size);
     secp256k1_scratch* ret = (secp256k1_scratch *)alloc;
     if (ret != NULL) {
@@ -60,6 +60,10 @@ static size_t secp256k1_scratch_max_allocation(const secp256k1_callback* error_c
         secp256k1_callback_call(error_callback, "invalid scratch space");
         return 0;
     }
+    /* Ensure that multiplication will not wrap around */
+    if (ALIGNMENT > 1 && objects > SIZE_MAX/(ALIGNMENT - 1)) {
+        return 0;
+    }
     if (scratch->max_size - scratch->alloc_size <= objects * (ALIGNMENT - 1)) {
         return 0;
     }
@@ -68,7 +72,14 @@ static size_t secp256k1_scratch_max_allocation(const secp256k1_callback* error_c
 
 static void *secp256k1_scratch_alloc(const secp256k1_callback* error_callback, secp256k1_scratch* scratch, size_t size) {
     void *ret;
-    size = ROUND_TO_ALIGN(size);
+    size_t rounded_size;
+
+    rounded_size = ROUND_TO_ALIGN(size);
+    /* Check that rounding did not wrap around */
+    if (rounded_size < size) {
+        return NULL;
+    }
+    size = rounded_size;
 
     if (memcmp(scratch->magic, "scratch", 8) != 0) {
         secp256k1_callback_call(error_callback, "invalid scratch space");

src/tests.c

@@ -364,8 +364,8 @@ void run_scratch_tests(void) {
     CHECK(scratch->alloc_size != 0);
     CHECK(scratch->alloc_size % ALIGNMENT == 0);
 
-    /* Allocating another 500 bytes fails */
-    CHECK(secp256k1_scratch_alloc(&none->error_callback, scratch, 500) == NULL);
+    /* Allocating another 501 bytes fails */
+    CHECK(secp256k1_scratch_alloc(&none->error_callback, scratch, 501) == NULL);
     CHECK(secp256k1_scratch_max_allocation(&none->error_callback, scratch, 0) == 1000 - adj_alloc);
     CHECK(secp256k1_scratch_max_allocation(&none->error_callback, scratch, 1) == 1000 - adj_alloc - (ALIGNMENT - 1));
     CHECK(scratch->alloc_size != 0);
@@ -398,6 +398,18 @@ void run_scratch_tests(void) {
     secp256k1_scratch_space_destroy(none, scratch);
     CHECK(ecount == 5);
 
+    /* Test that large integers do not wrap around in a bad way */
+    scratch = secp256k1_scratch_space_create(none, 1000);
+    /* Try max allocation with a large number of objects. Only makes sense if
+     * ALIGNMENT is greater than 1 because otherwise the objects take no extra
+     * space. */
+    CHECK(ALIGNMENT <= 1 || !secp256k1_scratch_max_allocation(&none->error_callback, scratch, (SIZE_MAX / (ALIGNMENT - 1)) + 1));
+    /* Try allocating SIZE_MAX to test wrap around which only happens if
+     * ALIGNMENT > 1, otherwise it returns NULL anyway because the scratch
+     * space is too small. */
+    CHECK(secp256k1_scratch_alloc(&none->error_callback, scratch, SIZE_MAX) == NULL);
+    secp256k1_scratch_space_destroy(none, scratch);
+
     /* cleanup */
     secp256k1_scratch_space_destroy(none, NULL); /* no-op */
     secp256k1_context_destroy(none);