From 4edaf06fb02a9ac9cd115e0c967bb0ef35cae01d Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Fri, 12 Jul 2019 09:56:56 +0000 Subject: [PATCH 1/4] Add check preventing integer multiplication wrapping around in scratch_max_allocation --- src/scratch_impl.h | 4 ++++ src/tests.c | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/src/scratch_impl.h b/src/scratch_impl.h index 4cee7000..937e29a0 100644 --- a/src/scratch_impl.h +++ b/src/scratch_impl.h @@ -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; } diff --git a/src/tests.c b/src/tests.c index 132df9ba..990f7d65 100644 --- a/src/tests.c +++ b/src/tests.c @@ -400,6 +400,14 @@ 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)); + secp256k1_scratch_space_destroy(none, scratch); + /* cleanup */ secp256k1_scratch_space_destroy(none, NULL); /* no-op */ secp256k1_context_destroy(none); From 8ecc6ce50ead28a0b8bab2f1fb18a58ee5204a13 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Fri, 12 Jul 2019 10:00:39 +0000 Subject: [PATCH 2/4] Add check preventing rounding to alignment from wrapping around in scratch_alloc --- src/scratch_impl.h | 9 ++++++++- src/tests.c | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/scratch_impl.h b/src/scratch_impl.h index 937e29a0..f53de48b 100644 --- a/src/scratch_impl.h +++ b/src/scratch_impl.h @@ -72,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"); diff --git a/src/tests.c b/src/tests.c index 990f7d65..94deb4c1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -406,6 +406,10 @@ void run_scratch_tests(void) { * 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 */ From ada6361dece4265823478e0019a8c196e9285a26 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Mon, 29 Jul 2019 15:50:53 +0000 Subject: [PATCH 3/4] Use ROUND_TO_ALIGN in scratch_create --- src/scratch_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scratch_impl.h b/src/scratch_impl.h index f53de48b..b2056202 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) { From 60f7f2de5de917c2bee32a4cd79cc3818b6a94a0 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Tue, 30 Jul 2019 14:42:17 +0200 Subject: [PATCH 4/4] Don't assume that ALIGNMENT > 1 in tests --- src/tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests.c b/src/tests.c index 94deb4c1..0f46cf54 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);