From 9acf260b37c1f9e45fe2d92a76f05ca9a807d4fc Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 26 Oct 2020 14:38:30 +0100 Subject: [PATCH] [SECP256K1] Return NULL early in context_preallocated_create if flags invalid Summary: ``` If the user passes invalid flags to _context_create, and the default illegal callback does not abort the program (which is possible), then we work with the result of malloc(0), which may be undefined behavior. This violates the promise that a library function won't crash after the illegal callback has been called. This commit fixes this issue by returning NULL early in _context_create in that case. ``` Backport of secp256k1 [[https://github.com/bitcoin-core/secp256k1/pull/840 | PR840]]. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8202 --- src/secp256k1/src/secp256k1.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/secp256k1/src/secp256k1.c b/src/secp256k1/src/secp256k1.c index e94a1c6d21..b20ab425b5 100644 --- a/src/secp256k1/src/secp256k1.c +++ b/src/secp256k1/src/secp256k1.c @@ -86,6 +86,8 @@ const secp256k1_context *secp256k1_context_no_precomp = &secp256k1_context_no_pr size_t secp256k1_context_preallocated_size(unsigned int flags) { size_t ret = ROUND_TO_ALIGN(sizeof(secp256k1_context)); + /* A return value of 0 is reserved as an indicator for errors when we call this function internally. */ + VERIFY_CHECK(ret != 0); if (EXPECT((flags & SECP256K1_FLAGS_TYPE_MASK) != SECP256K1_FLAGS_TYPE_CONTEXT, 0)) { secp256k1_callback_call(&default_illegal_callback, @@ -122,21 +124,21 @@ secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigne if (!secp256k1_selftest()) { secp256k1_callback_call(&default_error_callback, "self test failed"); } - VERIFY_CHECK(prealloc != NULL); + prealloc_size = secp256k1_context_preallocated_size(flags); + if (prealloc_size == 0) { + return NULL; + } + VERIFY_CHECK(prealloc != NULL); ret = (secp256k1_context*)manual_alloc(&prealloc, sizeof(secp256k1_context), base, prealloc_size); ret->illegal_callback = default_illegal_callback; ret->error_callback = default_error_callback; - if (EXPECT((flags & SECP256K1_FLAGS_TYPE_MASK) != SECP256K1_FLAGS_TYPE_CONTEXT, 0)) { - secp256k1_callback_call(&ret->illegal_callback, - "Invalid flags"); - return NULL; - } - secp256k1_ecmult_context_init(&ret->ecmult_ctx); secp256k1_ecmult_gen_context_init(&ret->ecmult_gen_ctx); + /* Flags have been checked by secp256k1_context_preallocated_size. */ + VERIFY_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_CONTEXT); if (flags & SECP256K1_FLAGS_BIT_CONTEXT_SIGN) { secp256k1_ecmult_gen_context_build(&ret->ecmult_gen_ctx, &prealloc); }