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/src/lib/libcrypto/bn/bn_ctx.c b/src/lib/libcrypto/bn/bn_ctx.c
index d2f5558b89..5b05e01878 100644
--- a/src/lib/libcrypto/bn/bn_ctx.c
+++ b/src/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--;
 }