[openssl-dev] Concerns regarding bn_wexpand/bn_expand2/bn_expand_internal

[Brian Smith]

Although bn_wexpand is a private function within the crypto/bn module, it
is exposed to other modules through bn_int.h. It is used from outside
crypto/bn quite frequently.

bn_wexpand is implemented in terms of a function called bn_expand2:

/*
 * This is an internal function that should not be used in applications. It
 * ensures that 'b' has enough room for a 'words' word number and
initialises
 * any unused part of b->d with leading zeros. It is mostly used by the
 * various BIGNUM routines. If there is an error, NULL is returned. If not,
 * 'b' is returned.
 */

BIGNUM *bn_expand2(BIGNUM *b, int words)
{
    bn_check_top(b);

    if (words > b->dmax) {
        BN_ULONG *a = bn_expand_internal(b, words);
        if (!a)
            return NULL;
        if (b->d)
            OPENSSL_free(b->d);
        b->d = a;
        b->dmax = words;
    }

    bn_check_top(b);
    return b;
}

My understanding of "any unused part of b->d" would be something like
|memset(b->d + b->top, 0, words - b->top)|.

But, does bn_expand2 actually zeroize anything?

In bn_expand_internal, we see:

[...]

#ifdef PURIFY
    /*
     * Valgrind complains in BN_consttime_swap because we process the whole
     * array even if it's not initialised yet. This doesn't matter in that
     * function - what's important is constant time operation (we're not
     * actually going to use the data)
     */
    memset(a, 0, sizeof(BN_ULONG) * words);
#endif

It seems like when PURIFY is not set, then bn_expand2 is not meeting its
contract. I wrote a test like so:

BIGNUM b;
BN_init(&b);
bn_wexpand(&b, 1000);

I then iterated through all the words of b to verified that all 1000 words
were zero, but in fact they were not. That is concerning.

Here's the RT for this changeset:
https://rt.openssl.org/Ticket/Display.html?id=3415&user=guest&pass=guest

Quote: "This seems to be the most pragmatic way forward, although it does
have the disadvantage that this will mask any other future problems in the
bn library due to incorrect unlitialised reads of this data."

I think that masking such problems is too big a disadvantage to be allowed,
in general, because it effectively disables uninitialized memory checking
for BIGNUM, which is one of the most important things to have uninitialized
memory checking for.

But, if such a workaround is really needed for BN_consttime_swap, then I
think that workaround should be put into BN_consttime_swap instead of into
this lower-level function. Even more preferably, the fix would appear in
the caller of BN_consttime_swap.

Cheers,
Brian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mta.openssl.org/pipermail/openssl-dev/attachments/20150428/9277c407/attachment.html>