Repository: bitcoin-core/secp256k1
Pull request #648 (2019-07-12 10:08:45)

Prevent ints from wrapping around in scratch space functions
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. 

real-or-random: If you want, you can cherry-pick https://github.com/real-or-random/secp256k1/commit/00076ab2347104f8ab6d88b8aa08865d0b3fd93b; this does not need a separate PR.
jonasnick: Squashed fixups and cherry-picked @real-or-random's commit such that tests pass now even with `#define ALIGNMENT 1` 

Patch:
diff --git a/src/scratch_impl.h b/src/scratch_impl.h
index 4cee700..b205620 100644
--- a/src/scratch_impl.h
+++ b/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");


Test patch:
diff --git a/src/tests.c b/src/tests.c
index 132df9b..0f46cf5 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -366,8 +366,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);
@@ -400,6 +400,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);