Clarify lifecycle of |BN_BLINDING|.

In |bn_blinding_update| the condition |b->e != NULL| would never be
true (probably), but the test made reasoning about the correctness of
the code confusing. That confusion was amplified by the circuitous and
unusual way in which |BN_BLINDING|s are constructed. Clarify all this
by simplifying the construction of |BN_BLINDING|s, making it more like
the construction of other structures.

Also, make counter unsigned as it is no longer ever negative.

Change-Id: I6161dcfeae19a80c780ccc6762314079fca1088b
Reviewed-on: https://boringssl-review.googlesource.com/7530
Reviewed-by: David Benjamin <davidben@google.com>

crypto/rsa/blinding.c

@@ -108,6 +108,7 @@
 
 #include <openssl/rsa.h>
 
+#include <assert.h>
 #include <string.h>
 
 #include <openssl/bn.h>
@@ -124,46 +125,57 @@ struct bn_blinding_st {
   BIGNUM *Ai;
   BIGNUM *e;
   BIGNUM *mod;
-  int counter;
+  unsigned counter;
 };
 
-static BN_BLINDING *bn_blinding_create_param(BN_BLINDING *b, const BIGNUM *e,
-                                             BIGNUM *m, BN_CTX *ctx,
-                                             const BN_MONT_CTX *mont_ctx);
+static BIGNUM *rsa_get_public_exp(const BIGNUM *d, const BIGNUM *p,
+                                  const BIGNUM *q, BN_CTX *ctx);
+static int bn_blinding_create_param(BN_BLINDING *b, BN_CTX *ctx,
+                                    const BN_MONT_CTX *mont_ctx);
 
-BN_BLINDING *BN_BLINDING_new(const BIGNUM *A, const BIGNUM *Ai, BIGNUM *mod) {
-  BN_BLINDING *ret = NULL;
+BN_BLINDING *BN_BLINDING_new(const RSA *rsa, BN_CTX *ctx) {
+  assert(ctx != NULL);
 
-  ret = OPENSSL_malloc(sizeof(BN_BLINDING));
+  BN_BLINDING *ret = OPENSSL_malloc(sizeof(BN_BLINDING));
   if (ret == NULL) {
     OPENSSL_PUT_ERROR(RSA, ERR_R_MALLOC_FAILURE);
     return NULL;
   }
   memset(ret, 0, sizeof(BN_BLINDING));
-  if (A != NULL) {
-    ret->A = BN_dup(A);
-    if (ret->A == NULL) {
+
+  ret->A = BN_new();
+  if (ret->A == NULL) {
+    goto err;
+  }
+
+  ret->Ai = BN_new();
+  if (ret->Ai == NULL) {
+    goto err;
+  }
+
+  if (rsa->e != NULL) {
+    ret->e = BN_dup(rsa->e);
+    if (ret->e == NULL) {
       goto err;
     }
-  }
-  if (Ai != NULL) {
-    ret->Ai = BN_dup(Ai);
-    if (ret->Ai == NULL) {
+  } else {
+    ret->e = rsa_get_public_exp(rsa->d, rsa->p, rsa->q, ctx);
+    if (ret->e == NULL) {
+      OPENSSL_PUT_ERROR(RSA, RSA_R_NO_PUBLIC_EXPONENT);
       goto err;
     }
   }
 
   /* save a copy of mod in the BN_BLINDING structure */
-  ret->mod = BN_dup(mod);
+  ret->mod = BN_dup(rsa->n);
   if (ret->mod == NULL) {
     goto err;
   }
   BN_set_flags(ret->mod, BN_FLG_CONSTTIME);
 
-  /* Set the counter to the special value -1
-   * to indicate that this is never-used fresh blinding
-   * that does not need updating before first use. */
-  ret->counter = -1;
+  /* The blinding values need to be created before this blinding can be used. */
+  ret->counter = BN_BLINDING_COUNTER - 1;
+
   return ret;
 
 err:
@@ -185,18 +197,9 @@ void BN_BLINDING_free(BN_BLINDING *r) {
 
 static int bn_blinding_update(BN_BLINDING *b, BN_CTX *ctx,
                               const BN_MONT_CTX *mont_ctx) {
-  if (b->A == NULL || b->Ai == NULL || b->e == NULL) {
-    OPENSSL_PUT_ERROR(RSA, RSA_R_BN_NOT_INITIALIZED);
-    goto err;
-  }
-
-  if (b->counter == -1) {
-    b->counter = 0;
-  }
-
   if (++b->counter == BN_BLINDING_COUNTER) {
     /* re-create blinding parameters */
-    if (!bn_blinding_create_param(b, NULL, NULL, ctx, mont_ctx)) {
+    if (!bn_blinding_create_param(b, ctx, mont_ctx)) {
       goto err;
     }
     b->counter = 0;
@@ -223,101 +226,52 @@ err:
 
 int BN_BLINDING_convert(BIGNUM *n, BN_BLINDING *b, BN_CTX *ctx,
                         const BN_MONT_CTX *mont_ctx) {
-  int ret = 1;
-
-  if (b->A == NULL || b->Ai == NULL) {
-    OPENSSL_PUT_ERROR(RSA, RSA_R_BN_NOT_INITIALIZED);
-    return 0;
-  }
-
-  if (b->counter == -1) {
-    /* Fresh blinding, doesn't need updating. */
-    b->counter = 0;
-  } else if (!bn_blinding_update(b, ctx, mont_ctx)) {
+  if (!bn_blinding_update(b, ctx, mont_ctx) ||
+      !BN_mod_mul(n, n, b->A, b->mod, ctx)) {
     return 0;
   }
 
-  if (!BN_mod_mul(n, n, b->A, b->mod, ctx)) {
-    ret = 0;
-  }
-
-  return ret;
+  return 1;
 }
 
 int BN_BLINDING_invert(BIGNUM *n, const BN_BLINDING *b, BN_CTX *ctx) {
-  if (b->Ai == NULL) {
-    OPENSSL_PUT_ERROR(RSA, RSA_R_BN_NOT_INITIALIZED);
-    return 0;
-  }
   return BN_mod_mul(n, n, b->Ai, b->mod, ctx);
 }
 
-static BN_BLINDING *bn_blinding_create_param(
-    BN_BLINDING *b, const BIGNUM *e, BIGNUM *m, BN_CTX *ctx,
-    const BN_MONT_CTX *mont_ctx) {
+static int bn_blinding_create_param(BN_BLINDING *b, BN_CTX *ctx,
+                                    const BN_MONT_CTX *mont_ctx) {
   int retry_counter = 32;
-  BN_BLINDING *ret = NULL;
-
-  if (b == NULL) {
-    ret = BN_BLINDING_new(NULL, NULL, m);
-  } else {
-    ret = b;
-  }
-
-  if (ret == NULL) {
-    goto err;
-  }
-
-  if (ret->A == NULL && (ret->A = BN_new()) == NULL) {
-    goto err;
-  }
-  if (ret->Ai == NULL && (ret->Ai = BN_new()) == NULL) {
-    goto err;
-  }
-
-  if (e != NULL) {
-    BN_free(ret->e);
-    ret->e = BN_dup(e);
-  }
-  if (ret->e == NULL) {
-    goto err;
-  }
 
   do {
-    if (!BN_rand_range(ret->A, ret->mod)) {
-      goto err;
+    if (!BN_rand_range(b->A, b->mod)) {
+      OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
+      return 0;
     }
 
     int no_inverse;
-    if (BN_mod_inverse_ex(ret->Ai, &no_inverse, ret->A, ret->mod, ctx) == NULL) {
+    if (BN_mod_inverse_ex(b->Ai, &no_inverse, b->A, b->mod, ctx) == NULL) {
       /* this should almost never happen for good RSA keys */
       if (no_inverse) {
         if (retry_counter-- == 0) {
           OPENSSL_PUT_ERROR(RSA, RSA_R_TOO_MANY_ITERATIONS);
-          goto err;
+          return 0;
         }
         ERR_clear_error();
       } else {
-        goto err;
+        OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
+        return 0;
       }
     } else {
       break;
     }
   } while (1);
 
-  if (!BN_mod_exp_mont(ret->A, ret->A, ret->e, ret->mod, ctx, mont_ctx)) {
-    goto err;
-  }
-
-  return ret;
-
-err:
-  if (b == NULL) {
-    BN_BLINDING_free(ret);
-    ret = NULL;
+  if (!BN_mod_exp_mont(b->A, b->A, b->e, b->mod, ctx, mont_ctx)) {
+    OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
+    return 0;
   }
 
-  return ret;
+  return 1;
 }
 
 static BIGNUM *rsa_get_public_exp(const BIGNUM *d, const BIGNUM *p,
@@ -352,56 +306,3 @@ err:
   BN_CTX_end(ctx);
   return ret;
 }
-
-BN_BLINDING *rsa_setup_blinding(RSA *rsa, BN_CTX *in_ctx) {
-  BIGNUM local_n;
-  BIGNUM *e, *n;
-  BN_CTX *ctx;
-  BN_BLINDING *ret = NULL;
-  BN_MONT_CTX *mont_ctx = NULL;
-
-  if (in_ctx == NULL) {
-    ctx = BN_CTX_new();
-    if (ctx == NULL) {
-      return 0;
-    }
-  } else {
-    ctx = in_ctx;
-  }
-
-  if (rsa->e == NULL) {
-    e = rsa_get_public_exp(rsa->d, rsa->p, rsa->q, ctx);
-    if (e == NULL) {
-      OPENSSL_PUT_ERROR(RSA, RSA_R_NO_PUBLIC_EXPONENT);
-      goto err;
-    }
-  } else {
-    e = rsa->e;
-  }
-
-  n = &local_n;
-  BN_with_flags(n, rsa->n, BN_FLG_CONSTTIME);
-
-  if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) {
-    mont_ctx = BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx);
-    if (mont_ctx == NULL) {
-      goto err;
-    }
-  }
-
-  ret = bn_blinding_create_param(NULL, e, n, ctx, mont_ctx);
-  if (ret == NULL) {
-    OPENSSL_PUT_ERROR(RSA, ERR_R_BN_LIB);
-    goto err;
-  }
-
-err:
-  if (in_ctx == NULL) {
-    BN_CTX_free(ctx);
-  }
-  if (rsa->e == NULL) {
-    BN_free(e);
-  }
-
-  return ret;
-}

crypto/rsa/internal.h

@@ -90,12 +90,11 @@ int rsa_default_keygen(RSA *rsa, int bits, BIGNUM *e_value, BN_GENCB *cb);
 #define RSA_PKCS1_PADDING_SIZE 11
 
 
-BN_BLINDING *BN_BLINDING_new(const BIGNUM *A, const BIGNUM *Ai, BIGNUM *mod);
+BN_BLINDING *BN_BLINDING_new(const RSA *rsa, BN_CTX *ctx);
 void BN_BLINDING_free(BN_BLINDING *b);
 int BN_BLINDING_convert(BIGNUM *n, BN_BLINDING *b, BN_CTX *ctx,
                         const BN_MONT_CTX *mont_ctx);
 int BN_BLINDING_invert(BIGNUM *n, const BN_BLINDING *b, BN_CTX *ctx);
-BN_BLINDING *rsa_setup_blinding(RSA *rsa, BN_CTX *in_ctx);
 
 
 int RSA_padding_add_PKCS1_type_1(uint8_t *to, unsigned to_len,

crypto/rsa/rsa_impl.c

@@ -213,6 +213,8 @@ err:
  * |*index_used| and must be passed to |rsa_blinding_release| when finished. */
 static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used,
                                      BN_CTX *ctx) {
+  assert(rsa->mont_n != NULL);
+
   BN_BLINDING *ret = NULL;
   BN_BLINDING **new_blindings;
   uint8_t *new_blindings_inuse;
@@ -241,7 +243,7 @@ static BN_BLINDING *rsa_blinding_get(RSA *rsa, unsigned *index_used,
    * the arrays by one and use the newly created element. */
 
   CRYPTO_MUTEX_unlock(&rsa->lock);
-  ret = rsa_setup_blinding(rsa, ctx);
+  ret = BN_BLINDING_new(rsa, ctx);
   if (ret == NULL) {
     return NULL;
   }
@@ -550,6 +552,11 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in,
   }
 
   if (!(rsa->flags & RSA_FLAG_NO_BLINDING)) {
+    if (BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx) == NULL) {
+      OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
+      goto err;
+    }
+
     blinding = rsa_blinding_get(rsa, &blinding_index, ctx);
     if (blinding == NULL) {
       OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);