Rewrite BN_CTX.

The current BN_CTX implementation is an incredibly overengineered piece of
code, which even includes its own debug system.

Rewrite BN_CTX from scratch, simplifying things things considerably by
having a "stack" of BIGNUM pointers and a matching array of group
assignments. This means that BN_CTX_start() and BN_CTX_end() effectively
do not fail. Unlike the previous implementation, if a failure occurs
nothing will work and the BN_CTX must be freed/recreated, instead of
trying to pick up at the point where the failure occurred (which does
not make sense given its intended usage).

Additionally, it has long been documented that BN_CTX_start() must be
called before BN_CTX_get() can be used, however the previous implementation
did not actually enforce this. Now that missing BN_CTX_start() and
BN_CTX_end() calls have been added to DSA and EC, we can actually make
this a hard requirement.

ok tb@

1 files changed, 98 insertions, 410 deletions

diff --git a/lib/libcrypto/bn/bn_ctx.c b/lib/libcrypto/bn/bn_ctx.c
index d2f5558b899..5b05e01878d 100644
--- a/lib/libcrypto/bn/bn_ctx.c
+++ b/lib/libcrypto/bn/bn_ctx.c
@@ -1,474 +1,162 @@
-/* $OpenBSD: bn_ctx.c,v 1.19 2022/11/30 01:47:19 jsing Exp $ */
-/* Written by Ulf Moeller for the OpenSSL project. */
-/* ====================================================================
- * Copyright (c) 1998-2004 The OpenSSL Project.  All rights reserved.
+/*	$OpenBSD: bn_ctx.c,v 1.20 2023/01/14 15:23:27 jsing Exp $ */
+/*
+ * Copyright (c) 2023 Joel Sing <jsing@openbsd.org>
  *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- *
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in
- *    the documentation and/or other materials provided with the
- *    distribution.
- *
- * 3. All advertising materials mentioning features or use of this
- *    software must display the following acknowledgment:
- *    "This product includes software developed by the OpenSSL Project
- *    for use in the OpenSSL Toolkit. (http://www.openssl.org/)"
- *
- * 4. The names "OpenSSL Toolkit" and "OpenSSL Project" must not be used to
- *    endorse or promote products derived from this software without
- *    prior written permission. For written permission, please contact
- *    openssl-core@openssl.org.
- *
- * 5. Products derived from this software may not be called "OpenSSL"
- *    nor may "OpenSSL" appear in their names without prior written
- *    permission of the OpenSSL Project.
- *
- * 6. Redistributions of any form whatsoever must retain the following
- *    acknowledgment:
- *    "This product includes software developed by the OpenSSL Project
- *    for use in the OpenSSL Toolkit (http://www.openssl.org/)"
- *
- * THIS SOFTWARE IS PROVIDED BY THE OpenSSL PROJECT ``AS IS'' AND ANY
- * EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE OpenSSL PROJECT OR
- * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
- * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
- * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
- * OF THE POSSIBILITY OF SUCH DAMAGE.
- * ====================================================================
- *
- * This product includes cryptographic software written by Eric Young
- * (eay@cryptsoft.com).  This product includes software written by Tim
- * Hudson (tjh@cryptsoft.com).
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
  *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include <stdio.h>
+#include <stddef.h>
 #include <string.h>
 
 #include <openssl/opensslconf.h>
-
 #include <openssl/err.h>
 
 #include "bn_local.h"
 
-/* TODO list
- *
- * 1. Check a bunch of "(words+1)" type hacks in various bignum functions and
- * check they can be safely removed.
- *  - Check +1 and other ugliness in BN_from_montgomery()
- *
- * 2. Consider allowing a BN_new_ex() that, at least, lets you specify an
- * appropriate 'block' size that will be honoured by bn_expand_internal() to
- * prevent piddly little reallocations. OTOH, profiling bignum expansions in
- * BN_CTX doesn't show this to be a big issue.
- */
-
-/* How many bignums are in each "pool item"; */
-#define BN_CTX_POOL_SIZE	16
-/* The stack frame info is resizing, set a first-time expansion size; */
-#define BN_CTX_START_FRAMES	32
-
-/***********/
-/* BN_POOL */
-/***********/
-
-/* A bundle of bignums that can be linked with other bundles */
-typedef struct bignum_pool_item {
-	/* The bignum values */
-	BIGNUM vals[BN_CTX_POOL_SIZE];
-	/* Linked-list admin */
-	struct bignum_pool_item *prev, *next;
-} BN_POOL_ITEM;
-
-/* A linked-list of bignums grouped in bundles */
-typedef struct bignum_pool {
-	/* Linked-list admin */
-	BN_POOL_ITEM *head, *current, *tail;
-	/* Stack depth and allocation size */
-	unsigned used, size;
-} BN_POOL;
-
-static void		BN_POOL_init(BN_POOL *);
-static void		BN_POOL_finish(BN_POOL *);
-#ifndef OPENSSL_NO_DEPRECATED
-static void		BN_POOL_reset(BN_POOL *);
-#endif
-static BIGNUM *		BN_POOL_get(BN_POOL *);
-static void		BN_POOL_release(BN_POOL *, unsigned int);
+#define BN_CTX_INITIAL_LEN	8
 
-/************/
-/* BN_STACK */
-/************/
-
-/* A wrapper to manage the "stack frames" */
-typedef struct bignum_ctx_stack {
-	/* Array of indexes into the bignum stack */
-	unsigned int *indexes;
-	/* Number of stack frames, and the size of the allocated array */
-	unsigned int depth, size;
-} BN_STACK;
-
-static void		BN_STACK_init(BN_STACK *);
-static void		BN_STACK_finish(BN_STACK *);
-#ifndef OPENSSL_NO_DEPRECATED
-static void		BN_STACK_reset(BN_STACK *);
-#endif
-static int		BN_STACK_push(BN_STACK *, unsigned int);
-static unsigned int	BN_STACK_pop(BN_STACK *);
-
-/**********/
-/* BN_CTX */
-/**********/
-
-/* The opaque BN_CTX type */
 struct bignum_ctx {
-	/* The bignum bundles */
-	BN_POOL pool;
-	/* The "stack frames", if you will */
-	BN_STACK stack;
-	/* The number of bignums currently assigned */
-	unsigned int used;
-	/* Depth of stack overflow */
-	int err_stack;
-	/* Block "gets" until an "end" (compatibility behaviour) */
-	int too_many;
-};
+	BIGNUM **bignums;
+	uint8_t *groups;
+	uint8_t group;
+	size_t index;
+	size_t len;
 
-/* Enable this to find BN_CTX bugs */
-#ifdef BN_CTX_DEBUG
-static const char *ctxdbg_cur = NULL;
+	int error;
+};
 
-static void
-ctxdbg(BN_CTX *ctx)
+static int
+bn_ctx_grow(BN_CTX *bctx)
 {
-	unsigned int bnidx = 0, fpidx = 0;
-	BN_POOL_ITEM *item = ctx->pool.head;
-	BN_STACK *stack = &ctx->stack;
+	BIGNUM **bignums = NULL;
+	uint8_t *groups = NULL;
+	size_t len;
 
-	fprintf(stderr, "(%08x): ", (unsigned int)ctx);
-	while (bnidx < ctx->used) {
-		fprintf(stderr, "%03x ",
-		    item->vals[bnidx++ % BN_CTX_POOL_SIZE].dmax);
-		if (!(bnidx % BN_CTX_POOL_SIZE))
-			item = item->next;
-	}
-	fprintf(stderr, "\n");
-	bnidx = 0;
-	fprintf(stderr, "          : ");
-	while (fpidx < stack->depth) {
-		while (bnidx++ < stack->indexes[fpidx])
-			fprintf(stderr, "    ");
-		fprintf(stderr, "^^^ ");
-		bnidx++;
-		fpidx++;
+	if ((len = bctx->len) == 0) {
+		len = BN_CTX_INITIAL_LEN;
+	} else {
+		if (SIZE_MAX - len < len)
+			return 0;
+		len *= 2;
 	}
-	fprintf(stderr, "\n");
-}
-#define CTXDBG_ENTRY(str, ctx) \
-	do { \
-		ctxdbg_cur = (str); \
-		fprintf(stderr, "Starting %s\n", ctxdbg_cur); \
-		ctxdbg(ctx); \
-	} while(0)
 
-#define CTXDBG_EXIT(ctx) \
-	do { \
-		fprintf(stderr, "Ending %s\n", ctxdbg_cur); \
-		ctxdbg(ctx); \
-	} while(0)
+	if ((bignums = recallocarray(bctx->bignums, bctx->len, len,
+	    sizeof(bctx->bignums[0]))) == NULL)
+		return 0;
+	bctx->bignums = bignums;
 
-#define CTXDBG_RET(ctx,ret)
-#else
-#define CTXDBG_ENTRY(str, ctx)
-#define CTXDBG_EXIT(ctx)
-#define CTXDBG_RET(ctx,ret)
-#endif
+	if ((groups = reallocarray(bctx->groups, len,
+	    sizeof(bctx->groups[0]))) == NULL)
+		return 0;
+	bctx->groups = groups;
 
-/* This function is an evil legacy and should not be used. This implementation
- * is WYSIWYG, though I've done my best. */
-#ifndef OPENSSL_NO_DEPRECATED
-void
-BN_CTX_init(BN_CTX *ctx)
-{
-	/* Assume the caller obtained the context via BN_CTX_new() and so is
-	 * trying to reset it for use. Nothing else makes sense, least of all
-	 * binary compatibility from a time when they could declare a static
-	 * variable. */
-	BN_POOL_reset(&ctx->pool);
-	BN_STACK_reset(&ctx->stack);
-	ctx->used = 0;
-	ctx->err_stack = 0;
-	ctx->too_many = 0;
+	bctx->len = len;
+
+	return 1;
 }
-#endif
 
 BN_CTX *
 BN_CTX_new(void)
 {
-	BN_CTX *ret = malloc(sizeof(BN_CTX));
-	if (!ret) {
-		BNerror(ERR_R_MALLOC_FAILURE);
-		return NULL;
-	}
-
-	/* Initialise the structure */
-	BN_POOL_init(&ret->pool);
-	BN_STACK_init(&ret->stack);
-	ret->used = 0;
-	ret->err_stack = 0;
-	ret->too_many = 0;
-	return ret;
+	return calloc(1, sizeof(struct bignum_ctx));
 }
 
 void
-BN_CTX_free(BN_CTX *ctx)
+BN_CTX_init(BN_CTX *bctx)
 {
-	if (ctx == NULL)
-		return;
-#ifdef BN_CTX_DEBUG
-	{
-		BN_POOL_ITEM *pool = ctx->pool.head;
-		fprintf(stderr, "BN_CTX_free, stack-size=%d, pool-bignums=%d\n",
-		    ctx->stack.size, ctx->pool.size);
-		fprintf(stderr, "dmaxs: ");
-		while (pool) {
-			unsigned loop = 0;
-			while (loop < BN_CTX_POOL_SIZE)
-				fprintf(stderr, "%02x ",
-				    pool->vals[loop++].dmax);
-			pool = pool->next;
-		}
-		fprintf(stderr, "\n");
-	}
-#endif
-	BN_STACK_finish(&ctx->stack);
-	BN_POOL_finish(&ctx->pool);
-	free(ctx);
+	memset(bctx, 0, sizeof(*bctx));
 }
 
 void
-BN_CTX_start(BN_CTX *ctx)
+BN_CTX_free(BN_CTX *bctx)
 {
-	CTXDBG_ENTRY("BN_CTX_start", ctx);
-
-	/* If we're already overflowing ... */
-	if (ctx->err_stack || ctx->too_many)
-		ctx->err_stack++;
-	/* (Try to) get a new frame pointer */
-	else if (!BN_STACK_push(&ctx->stack, ctx->used)) {
-		BNerror(BN_R_TOO_MANY_TEMPORARY_VARIABLES);
-		ctx->err_stack++;
-	}
-	CTXDBG_EXIT(ctx);
-}
+	size_t i;
 
-void
-BN_CTX_end(BN_CTX *ctx)
-{
-	if (ctx == NULL)
+	if (bctx == NULL)
 		return;
 
-	CTXDBG_ENTRY("BN_CTX_end", ctx);
-
-	if (ctx->err_stack)
-		ctx->err_stack--;
-	else {
-		unsigned int fp = BN_STACK_pop(&ctx->stack);
-		/* Does this stack frame have anything to release? */
-		if (fp < ctx->used)
-			BN_POOL_release(&ctx->pool, ctx->used - fp);
-		ctx->used = fp;
-		/* Unjam "too_many" in case "get" had failed */
-		ctx->too_many = 0;
-	}
-	CTXDBG_EXIT(ctx);
-}
-
-BIGNUM *
-BN_CTX_get(BN_CTX *ctx)
-{
-	BIGNUM *ret;
-
-	CTXDBG_ENTRY("BN_CTX_get", ctx);
-
-	if (ctx->err_stack || ctx->too_many)
-		return NULL;
-	if ((ret = BN_POOL_get(&ctx->pool)) == NULL) {
-		/* Setting too_many prevents repeated "get" attempts from
-		 * cluttering the error stack. */
-		ctx->too_many = 1;
-		BNerror(BN_R_TOO_MANY_TEMPORARY_VARIABLES);
-		return NULL;
+	for (i = 0; i < bctx->len; i++) {
+		BN_free(bctx->bignums[i]);
+		bctx->bignums[i] = NULL;
 	}
-	/* OK, make sure the returned bignum is "zero" */
-	BN_zero(ret);
-	ctx->used++;
-	CTXDBG_RET(ctx, ret);
-	return ret;
-}
 
-/************/
-/* BN_STACK */
-/************/
+	free(bctx->bignums);
+	free(bctx->groups);
 
-static void
-BN_STACK_init(BN_STACK *st)
-{
-	st->indexes = NULL;
-	st->depth = st->size = 0;
+	freezero(bctx, sizeof(*bctx));
 }
 
-static void
-BN_STACK_finish(BN_STACK *st)
+void
+BN_CTX_start(BN_CTX *bctx)
 {
-	if (st->size)
-		free(st->indexes);
-}
+	bctx->group++;
 
-#ifndef OPENSSL_NO_DEPRECATED
-static void
-BN_STACK_reset(BN_STACK *st)
-{
-	st->depth = 0;
-}
-#endif
-
-static int
-BN_STACK_push(BN_STACK *st, unsigned int idx)
-{
-	if (st->depth == st->size)
-		/* Need to expand */
-	{
-		unsigned int newsize = (st->size ?
-		    (st->size * 3 / 2) : BN_CTX_START_FRAMES);
-		unsigned int *newitems = reallocarray(NULL,
-		    newsize, sizeof(unsigned int));
-		if (!newitems)
-			return 0;
-		if (st->depth)
-			memcpy(newitems, st->indexes, st->depth *
-			    sizeof(unsigned int));
-		if (st->size)
-			free(st->indexes);
-		st->indexes = newitems;
-		st->size = newsize;
+	if (bctx->group == 0) {
+		BNerror(BN_R_TOO_MANY_TEMPORARY_VARIABLES);
+		bctx->error = 1;
+		return;
 	}
-	st->indexes[(st->depth)++] = idx;
-	return 1;
 }
 
-static unsigned int
-BN_STACK_pop(BN_STACK *st)
+BIGNUM *
+BN_CTX_get(BN_CTX *bctx)
 {
-	return st->indexes[--(st->depth)];
-}
-
-/***********/
-/* BN_POOL */
-/***********/
+	BIGNUM *bn = NULL;
 
-static void
-BN_POOL_init(BN_POOL *p)
-{
-	p->head = p->current = p->tail = NULL;
-	p->used = p->size = 0;
-}
+	if (bctx->error)
+		return NULL;
 
-static void
-BN_POOL_finish(BN_POOL *p)
-{
-	while (p->head) {
-		unsigned int loop = 0;
-		BIGNUM *bn = p->head->vals;
-		while (loop++ < BN_CTX_POOL_SIZE) {
-			if (bn->d)
-				BN_clear_free(bn);
-			bn++;
-		}
-		p->current = p->head->next;
-		free(p->head);
-		p->head = p->current;
+	if (bctx->group == 0) {
+		BNerror(ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+		bctx->error = 1;
+		return NULL;
 	}
-}
 
-#ifndef OPENSSL_NO_DEPRECATED
-static void
-BN_POOL_reset(BN_POOL *p)
-{
-	BN_POOL_ITEM *item = p->head;
-	while (item) {
-		unsigned int loop = 0;
-		BIGNUM *bn = item->vals;
-		while (loop++ < BN_CTX_POOL_SIZE) {
-			if (bn->d)
-				BN_clear(bn);
-			bn++;
+	if (bctx->index == bctx->len) {
+		if (!bn_ctx_grow(bctx)) {
+			BNerror(BN_R_TOO_MANY_TEMPORARY_VARIABLES);
+			bctx->error = 1;
+			return NULL;
 		}
-		item = item->next;
 	}
-	p->current = p->head;
-	p->used = 0;
-}
-#endif
 
-static BIGNUM *
-BN_POOL_get(BN_POOL *p)
-{
-	if (p->used == p->size) {
-		BIGNUM *bn;
-		unsigned int loop = 0;
-		BN_POOL_ITEM *item = malloc(sizeof(BN_POOL_ITEM));
-		if (!item)
+	if ((bn = bctx->bignums[bctx->index]) == NULL) {
+		if ((bn = BN_new()) == NULL) {
+			BNerror(BN_R_TOO_MANY_TEMPORARY_VARIABLES);
+			bctx->error = 1;
 			return NULL;
-		/* Initialise the structure */
-		bn = item->vals;
-		while (loop++ < BN_CTX_POOL_SIZE)
-			BN_init(bn++);
-		item->prev = p->tail;
-		item->next = NULL;
-		/* Link it in */
-		if (!p->head)
-			p->head = p->current = p->tail = item;
-		else {
-			p->tail->next = item;
-			p->tail = item;
-			p->current = item;
 		}
-		p->size += BN_CTX_POOL_SIZE;
-		p->used++;
-		/* Return the first bignum from the new pool */
-		return item->vals;
+		bctx->bignums[bctx->index] = bn;
 	}
-	if (!p->used)
-		p->current = p->head;
-	else if ((p->used % BN_CTX_POOL_SIZE) == 0)
-		p->current = p->current->next;
-	return p->current->vals + ((p->used++) % BN_CTX_POOL_SIZE);
+	bctx->groups[bctx->index] = bctx->group;
+	bctx->index++;
+
+	BN_zero(bn);
+
+	return bn;
 }
 
-static void
-BN_POOL_release(BN_POOL *p, unsigned int num)
+void
+BN_CTX_end(BN_CTX *bctx)
 {
-	unsigned int offset = (p->used - 1) % BN_CTX_POOL_SIZE;
+	if (bctx == NULL || bctx->error || bctx->group == 0)
+		return;
 
-	p->used -= num;
-	while (num--) {
-		if (!offset) {
-			offset = BN_CTX_POOL_SIZE - 1;
-			p->current = p->current->prev;
-		} else
-			offset--;
+	while (bctx->index > 0 && bctx->groups[bctx->index - 1] == bctx->group) {
+		BN_zero(bctx->bignums[bctx->index - 1]);
+		bctx->groups[bctx->index - 1] = 0;
+		bctx->index--;
 	}
+
+	bctx->group--;
 }