Commit fd7d2520 authored by Matt Caswell's avatar Matt Caswell
Browse files

Tighten up BN_with_flags usage and avoid a reachable assert 



The function rsa_ossl_mod_exp uses the function BN_with_flags to create a
temporary copy (local_r1) of a BIGNUM (r1) with modified flags. This
temporary copy shares some state with the original r1. If the state of r1
gets updated then local_r1's state will be stale. This was occurring in the
function so that when local_r1 was freed a call to bn_check_top was made
which failed an assert due to the stale state. To resolve this we must free
local_r1 immediately after we have finished using it and not wait until the
end of the function.

This problem prompted a review of all BN_with_flag usage within the
codebase. All other usage appears to be correct, although often not
obviously so. This commit refactors things to make it much clearer for
these other uses.

Reviewed-by: default avatarEmilia Käsper <emilia@openssl.org>
parent 6938c954

crypto/bn/bn_gcd.c

+17 −13
Original line number Diff line number Diff line
@@ -559,8 +559,6 @@ static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *in, 
                                        BN_CTX *ctx)

{

    BIGNUM *A, *B, *X, *Y, *M, *D, *T, *R = NULL;

    BIGNUM local_A, local_B;

    BIGNUM *pA, *pB;

    BIGNUM *ret = NULL;

    int sign;
@@ -598,11 +596,14 @@ static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *in, 
         * Turn BN_FLG_CONSTTIME flag on, so that when BN_div is invoked,

         * BN_div_no_branch will be called eventually.

         */

        pB = &local_B;

        local_B.flags = 0;

        BN_with_flags(pB, B, BN_FLG_CONSTTIME);

        if (!BN_nnmod(B, pB, A, ctx))

         {

            BIGNUM local_B;

            BN_init(&local_B);

            BN_with_flags(&local_B, B, BN_FLG_CONSTTIME);

            if (!BN_nnmod(B, &local_B, A, ctx))

                goto err;

            /* Ensure local_B goes out of scope before any further use of B */

        }

    }

    sign = -1;

    /*-
@@ -626,13 +627,16 @@ static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *in, 
         * Turn BN_FLG_CONSTTIME flag on, so that when BN_div is invoked,

         * BN_div_no_branch will be called eventually.

         */

        pA = &local_A;

        local_A.flags = 0;

        BN_with_flags(pA, A, BN_FLG_CONSTTIME);

        {

            BIGNUM local_A;

            BN_init(&local_A);

            BN_with_flags(&local_A, A, BN_FLG_CONSTTIME);



            /* (D, M) := (A/B, A%B) ... */

        if (!BN_div(D, M, pA, B, ctx))

            if (!BN_div(D, M, &local_A, B, ctx))

                goto err;

            /* Ensure local_A goes out of scope before any further use of A */

        }



        /*-

         * Now

crypto/bn/bn_lib.c

+2 −2
Original line number Diff line number Diff line
@@ -924,7 +924,7 @@ int BN_to_montgomery(BIGNUM *r, const BIGNUM *a, BN_MONT_CTX *mont, 
    return BN_mod_mul_montgomery(r, a, &(mont->RR), mont, ctx);

}



void BN_with_flags(BIGNUM *dest, const BIGNUM *b, int n)

void BN_with_flags(BIGNUM *dest, const BIGNUM *b, int flags)

{

    dest->d = b->d;

    dest->top = b->top;
@@ -932,7 +932,7 @@ void BN_with_flags(BIGNUM *dest, const BIGNUM *b, int n) 
    dest->neg = b->neg;

    dest->flags = ((dest->flags & BN_FLG_MALLOCED)

                   | (b->flags & ~BN_FLG_MALLOCED)

                   | BN_FLG_STATIC_DATA | n);

                   | BN_FLG_STATIC_DATA | flags);

}



BN_GENCB *BN_GENCB_new(void)

crypto/dh/dh_key.c

+3 −1
Original line number Diff line number Diff line
@@ -170,13 +170,15 @@ static int generate_key(DH *dh) 
            if (local_prk == NULL)

                goto err;

            BN_with_flags(prk, priv_key, BN_FLG_CONSTTIME);

        } else

        } else {

            prk = priv_key;

        }



        if (!dh->meth->bn_mod_exp(dh, pub_key, dh->g, prk, dh->p, ctx, mont)) {

            BN_free(local_prk);

            goto err;

        }

        /* We MUST free local_prk before any further use of priv_key */

        BN_free(local_prk);

    }

crypto/dsa/dsa_key.c

+3 −1
Original line number Diff line number Diff line
@@ -107,13 +107,15 @@ static int dsa_builtin_keygen(DSA *dsa) 
            if (local_prk == NULL)

                goto err;

            BN_with_flags(prk, priv_key, BN_FLG_CONSTTIME);

        } else

        } else {

            prk = priv_key;

        }



        if (!BN_mod_exp(pub_key, dsa->g, prk, dsa->p, ctx)) {

            BN_free(local_prk);

            goto err;

        }

        /* We MUST free local_prk before any further use of priv_key */

        BN_free(local_prk);

    }

crypto/rsa/rsa_crpt.c

+18 −14
Original line number Diff line number Diff line
@@ -159,8 +159,7 @@ static BIGNUM *rsa_get_public_exp(const BIGNUM *d, const BIGNUM *p, 


BN_BLINDING *RSA_setup_blinding(RSA *rsa, BN_CTX *in_ctx)

{

    BIGNUM *local_n = NULL;

    BIGNUM *e, *n;

    BIGNUM *e;

    BN_CTX *ctx;

    BN_BLINDING *ret = NULL;
@@ -196,6 +195,8 @@ BN_BLINDING *RSA_setup_blinding(RSA *rsa, BN_CTX *in_ctx) 
                 0.0);

    }



    {

        BIGNUM *local_n = NULL, *n;

        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {

            /* Set BN_FLG_CONSTTIME flag */

            local_n = n = BN_new();
@@ -204,11 +205,15 @@ BN_BLINDING *RSA_setup_blinding(RSA *rsa, BN_CTX *in_ctx) 
                goto err;

            }

            BN_with_flags(n, rsa->n, BN_FLG_CONSTTIME);

    } else

        } else {

            n = rsa->n;

        }



    ret = BN_BLINDING_create_param(NULL, e, n, ctx,

                                   rsa->meth->bn_mod_exp, rsa->_method_mod_n);

        ret = BN_BLINDING_create_param(NULL, e, n, ctx, rsa->meth->bn_mod_exp,

                                       rsa->_method_mod_n);

        /* We MUST free local_n before any further use of rsa->n */

        BN_free(local_n);

    }

    if (ret == NULL) {

        RSAerr(RSA_F_RSA_SETUP_BLINDING, ERR_R_BN_LIB);

        goto err;
@@ -220,7 +225,6 @@ BN_BLINDING *RSA_setup_blinding(RSA *rsa, BN_CTX *in_ctx) 
        BN_CTX_free(ctx);

    if (e != rsa->e)

        BN_free(e);

    BN_free(local_n);



    return ret;

}