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: Emilia Käsper <emilia@openssl.org>

diff --git a/crypto/bn/bn_gcd.c b/crypto/bn/bn_gcd.c
index 02643190ef71a0cc29111659737d876219c9cd48..d24360615f90b7d031840870da7f5d5ce75212fa 100644 (file)
--- a/crypto/bn/bn_gcd.c
+++ b/crypto/bn/bn_gcd.c
@@ -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))
-            goto err;
+         {
+            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))
-            goto err;
+            /* (D, M) := (A/B, A%B) ... */
+            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

diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c
index 2042920d3530d0dbec945add3e8771cc84f6a633..4a6480ba97c0f578a95f4955279f9240d5418b0e 100644 (file)
--- a/crypto/bn/bn_lib.c
+++ b/crypto/bn/bn_lib.c
@@ -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)

diff --git a/crypto/dh/dh_key.c b/crypto/dh/dh_key.c
index a5cac063aba38f67f40c9c7633e32cd901ef2e0e..271c6f4c6af7914146948e367cf8d7bc462644ca 100644 (file)
--- a/crypto/dh/dh_key.c
+++ b/crypto/dh/dh_key.c
@@ -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);
     }
 

diff --git a/crypto/dsa/dsa_key.c b/crypto/dsa/dsa_key.c
index d27d47a3b83e4f998481eebf34fc6d82b09146ed..0cde6d19a301453e7d8a589ef3a265b8cafef4ee 100644 (file)
--- a/crypto/dsa/dsa_key.c
+++ b/crypto/dsa/dsa_key.c
@@ -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);
     }
 

diff --git a/crypto/rsa/rsa_crpt.c b/crypto/rsa/rsa_crpt.c
index 4df1662320b70f8dc6b95c420f92307ac3aa5158..d08258e5e7003fba3fb0ed4048e8df25c86cef7c 100644 (file)
--- a/crypto/rsa/rsa_crpt.c
+++ b/crypto/rsa/rsa_crpt.c
@@ -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,19 +195,25 @@ BN_BLINDING *RSA_setup_blinding(RSA *rsa, BN_CTX *in_ctx)
                  0.0);
     }
 
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        /* Set BN_FLG_CONSTTIME flag */
-        local_n = n = BN_new();
-        if (local_n == NULL) {
-            RSAerr(RSA_F_RSA_SETUP_BLINDING, ERR_R_MALLOC_FAILURE);
-            goto err;
+    {
+        BIGNUM *local_n = NULL, *n;
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            /* Set BN_FLG_CONSTTIME flag */
+            local_n = n = BN_new();
+            if (local_n == NULL) {
+                RSAerr(RSA_F_RSA_SETUP_BLINDING, ERR_R_MALLOC_FAILURE);
+                goto err;
+            }
+            BN_with_flags(n, rsa->n, BN_FLG_CONSTTIME);
+        } else {
+            n = rsa->n;
         }
-        BN_with_flags(n, rsa->n, BN_FLG_CONSTTIME);
-    } 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;
 }

diff --git a/crypto/rsa/rsa_gen.c b/crypto/rsa/rsa_gen.c
index dc3e5d3dcdb22d0f530ce238478eab18a2699f13..d23d47d03e45ca4c5ded54e750c43246487724b0 100644 (file)
--- a/crypto/rsa/rsa_gen.c
+++ b/crypto/rsa/rsa_gen.c
@@ -89,17 +89,9 @@ static int rsa_builtin_keygen(RSA *rsa, int bits, BIGNUM *e_value,
                               BN_GENCB *cb)
 {
     BIGNUM *r0 = NULL, *r1 = NULL, *r2 = NULL, *r3 = NULL, *tmp;
-    BIGNUM *local_r0, *local_d, *local_p;
-    BIGNUM *pr0, *d, *p;
     int bitsp, bitsq, ok = -1, n = 0;
     BN_CTX *ctx = NULL;
 
-    local_r0 = BN_new();
-    local_d = BN_new();
-    local_p = BN_new();
-    if (local_r0 == NULL || local_d == NULL || local_p == NULL)
-        goto err;
-
     ctx = BN_CTX_new();
     if (ctx == NULL)
         goto err;
@@ -193,43 +185,69 @@ static int rsa_builtin_keygen(RSA *rsa, int bits, BIGNUM *e_value,
         goto err;               /* q-1 */
     if (!BN_mul(r0, r1, r2, ctx))
         goto err;               /* (p-1)(q-1) */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        pr0 = local_r0;
-        BN_with_flags(pr0, r0, BN_FLG_CONSTTIME);
-    } else
-        pr0 = r0;
-    if (!BN_mod_inverse(rsa->d, rsa->e, pr0, ctx))
-        goto err;               /* d */
-
-    /* set up d for correct BN_FLG_CONSTTIME flag */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        d = local_d;
-        BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
-    } else
-        d = rsa->d;
-
-    /* calculate d mod (p-1) */
-    if (!BN_mod(rsa->dmp1, d, r1, ctx))
-        goto err;
+    {
+        BIGNUM *local_r0 = NULL, *pr0;
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            pr0 = local_r0 = BN_new();
+            if (local_r0 == NULL)
+                goto err;
+            BN_with_flags(pr0, r0, BN_FLG_CONSTTIME);
+        } else {
+            pr0 = r0;
+        }
+        if (!BN_mod_inverse(rsa->d, rsa->e, pr0, ctx)) {
+            BN_free(local_r0);
+            goto err;               /* d */
+        }
+        /* We MUST free local_r0 before any further use of r0 */
+        BN_free(local_r0);
+    }
 
-    /* calculate d mod (q-1) */
-    if (!BN_mod(rsa->dmq1, d, r2, ctx))
-        goto err;
+    {
+        BIGNUM *local_d = NULL, *d;
+        /* set up d for correct BN_FLG_CONSTTIME flag */
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            d = local_d = BN_new();
+            if (local_d == NULL)
+                goto err;
+            BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
+        } else {
+            d = rsa->d;
+        }
 
-    /* calculate inverse of q mod p */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        p = local_p;
-        BN_with_flags(p, rsa->p, BN_FLG_CONSTTIME);
-    } else
-        p = rsa->p;
-    if (!BN_mod_inverse(rsa->iqmp, rsa->q, p, ctx))
-        goto err;
+        if (   /* calculate d mod (p-1) */
+               !BN_mod(rsa->dmp1, d, r1, ctx)
+               /* calculate d mod (q-1) */
+            || !BN_mod(rsa->dmq1, d, r2, ctx)) {
+            BN_free(local_d);
+            goto err;
+        }
+        /* We MUST free local_d before any further use of rsa->d */
+        BN_free(local_d);
+    }
+
+    {
+        BIGNUM *local_p = NULL, *p;
+
+        /* calculate inverse of q mod p */
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            p = local_p = BN_new();
+            if (local_p == NULL)
+                goto err;
+            BN_with_flags(p, rsa->p, BN_FLG_CONSTTIME);
+        } else {
+            p = rsa->p;
+        }
+        if (!BN_mod_inverse(rsa->iqmp, rsa->q, p, ctx)) {
+            BN_free(local_p);
+            goto err;
+        }
+        /* We MUST free local_p before any further use of rsa->p */
+        BN_free(local_p);
+    }
 
     ok = 1;
  err:
-    BN_free(local_r0);
-    BN_free(local_d);
-    BN_free(local_p);
     if (ok == -1) {
         RSAerr(RSA_F_RSA_BUILTIN_KEYGEN, ERR_LIB_BN);
         ok = 0;

diff --git a/crypto/rsa/rsa_ossl.c b/crypto/rsa/rsa_ossl.c
index 09a65b80b1f1203719b07a26c44c7a01d71f2f27..0752f5fd8b4cdc0d00ebfff6ed4e8b43cc95211c 100644 (file)
--- a/crypto/rsa/rsa_ossl.c
+++ b/crypto/rsa/rsa_ossl.c
@@ -426,8 +426,9 @@ static int rsa_ossl_private_encrypt(int flen, const unsigned char *from,
                 goto err;
             }
             BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
-        } else
+        } else {
             d = rsa->d;
+        }
 
         if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
             if (!BN_MONT_CTX_set_locked
@@ -441,6 +442,7 @@ static int rsa_ossl_private_encrypt(int flen, const unsigned char *from,
             BN_free(local_d);
             goto err;
         }
+        /* We MUST free local_d before any further use of rsa->d */
         BN_free(local_d);
     }
 
@@ -558,8 +560,9 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
                 goto err;
             }
             BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
-        } else
+        } else {
             d = rsa->d;
+        }
 
         if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
             if (!BN_MONT_CTX_set_locked
@@ -572,6 +575,7 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
             BN_free(local_d);
             goto err;
         }
+        /* We MUST free local_d before any further use of rsa->d */
         BN_free(local_d);
     }
 
@@ -712,20 +716,10 @@ static int rsa_ossl_public_decrypt(int flen, const unsigned char *from,
 static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx)
 {
     BIGNUM *r1, *m1, *vrfy;
-    BIGNUM *local_dmp1, *local_dmq1, *local_c, *local_r1;
-    BIGNUM *dmp1, *dmq1, *c, *pr1;
     int ret = 0;
 
     BN_CTX_start(ctx);
 
-    local_dmp1 = BN_new();
-    local_dmq1 = BN_new();
-    local_c = BN_new();
-    local_r1 = BN_new();
-    if (local_dmp1 == NULL
-            || local_dmq1 == NULL || local_c == NULL || local_r1 == NULL)
-        goto err;
-
     r1 = BN_CTX_get(ctx);
     m1 = BN_CTX_get(ctx);
     vrfy = BN_CTX_get(ctx);
@@ -765,6 +759,10 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx)
                 goto err;
             }
         }
+        /*
+         * We MUST free local_p and local_q before any further use of rsa->p and
+         * rsa->q
+         */
         BN_free(local_p);
         BN_free(local_q);
     }
@@ -775,44 +773,74 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx)
             goto err;
 
     /* compute I mod q */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        c = local_c;
-        BN_with_flags(c, I, BN_FLG_CONSTTIME);
-        if (!BN_mod(r1, c, rsa->q, ctx))
-            goto err;
-    } else {
-        if (!BN_mod(r1, I, rsa->q, ctx))
+    {
+        BIGNUM *local_c = NULL;
+        const BIGNUM *c;
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            local_c = BN_new();
+            if (local_c == NULL)
+                goto err;
+            BN_with_flags(local_c, I, BN_FLG_CONSTTIME);
+            c = local_c;
+        } else {
+            c = I;
+        }
+        if (!BN_mod(r1, c, rsa->q, ctx)) {
+            BN_free(local_c);
             goto err;
-    }
+        }
 
-    /* compute r1^dmq1 mod q */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        dmq1 = local_dmq1;
-        BN_with_flags(dmq1, rsa->dmq1, BN_FLG_CONSTTIME);
-    } else
-        dmq1 = rsa->dmq1;
-    if (!rsa->meth->bn_mod_exp(m1, r1, dmq1, rsa->q, ctx, rsa->_method_mod_q))
-        goto err;
+        {
+            BIGNUM *local_dmq1 = NULL, *dmq1;
+            /* compute r1^dmq1 mod q */
+            if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+                dmq1 = local_dmq1 = BN_new();
+                if (local_dmq1 == NULL) {
+                    BN_free(local_c);
+                    goto err;
+                }
+                BN_with_flags(dmq1, rsa->dmq1, BN_FLG_CONSTTIME);
+            } else {
+                dmq1 = rsa->dmq1;
+            }
+            if (!rsa->meth->bn_mod_exp(m1, r1, dmq1, rsa->q, ctx,
+                rsa->_method_mod_q)) {
+                BN_free(local_c);
+                BN_free(local_dmq1);
+                goto err;
+            }
+            /* We MUST free local_dmq1 before any further use of rsa->dmq1 */
+            BN_free(local_dmq1);
+        }
 
-    /* compute I mod p */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        c = local_c;
-        BN_with_flags(c, I, BN_FLG_CONSTTIME);
-        if (!BN_mod(r1, c, rsa->p, ctx))
-            goto err;
-    } else {
-        if (!BN_mod(r1, I, rsa->p, ctx))
+        /* compute I mod p */
+        if (!BN_mod(r1, c, rsa->p, ctx)) {
+            BN_free(local_c);
             goto err;
+        }
+        /* We MUST free local_c before any further use of I */
+        BN_free(local_c);
     }
 
-    /* compute r1^dmp1 mod p */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        dmp1 = local_dmp1;
-        BN_with_flags(dmp1, rsa->dmp1, BN_FLG_CONSTTIME);
-    } else
-        dmp1 = rsa->dmp1;
-    if (!rsa->meth->bn_mod_exp(r0, r1, dmp1, rsa->p, ctx, rsa->_method_mod_p))
-        goto err;
+    {
+        BIGNUM *local_dmp1 = NULL, *dmp1;
+        /* compute r1^dmp1 mod p */
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            dmp1 = local_dmp1 = BN_new();
+            if (local_dmp1 == NULL)
+                goto err;
+            BN_with_flags(dmp1, rsa->dmp1, BN_FLG_CONSTTIME);
+        } else {
+            dmp1 = rsa->dmp1;
+        }
+        if (!rsa->meth->bn_mod_exp(r0, r1, dmp1, rsa->p, ctx,
+                                   rsa->_method_mod_p)) {
+            BN_free(local_dmp1);
+            goto err;
+        }
+        /* We MUST free local_dmp1 before any further use of rsa->dmp1 */
+        BN_free(local_dmp1);
+    }
 
     if (!BN_sub(r0, r0, m1))
         goto err;
@@ -827,14 +855,24 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx)
     if (!BN_mul(r1, r0, rsa->iqmp, ctx))
         goto err;
 
-    /* Turn BN_FLG_CONSTTIME flag on before division operation */
-    if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
-        pr1 = local_r1;
-        BN_with_flags(pr1, r1, BN_FLG_CONSTTIME);
-    } else
-        pr1 = r1;
-    if (!BN_mod(r0, pr1, rsa->p, ctx))
-        goto err;
+    {
+        BIGNUM *local_r1 = NULL, *pr1;
+        /* Turn BN_FLG_CONSTTIME flag on before division operation */
+        if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) {
+            pr1 = local_r1 = BN_new();
+            if (local_r1 == NULL)
+                goto err;
+            BN_with_flags(pr1, r1, BN_FLG_CONSTTIME);
+        } else {
+            pr1 = r1;
+        }
+        if (!BN_mod(r0, pr1, rsa->p, ctx)) {
+            BN_free(local_r1);
+            goto err;
+        }
+        /* We MUST free local_r1 before any further use of r1 */
+        BN_free(local_r1);
+    }
 
     /*
      * If p < q it is occasionally possible for the correction of adding 'p'
@@ -883,23 +921,20 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx)
                 if (d == NULL)
                     goto err;
                 BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
-            } else
+            } else {
                 d = rsa->d;
+            }
             if (!rsa->meth->bn_mod_exp(r0, I, d, rsa->n, ctx,
                                        rsa->_method_mod_n)) {
                 BN_free(local_d);
                 goto err;
             }
-
+            /* We MUST free local_d before any further use of rsa->d */
             BN_free(local_d);
         }
     }
     ret = 1;
  err:
-    BN_free(local_dmp1);
-    BN_free(local_dmq1);
-    BN_free(local_c);
-    BN_free(local_r1);
     BN_CTX_end(ctx);
     return (ret);
 }