From febf77190f5b32c9825786239f87dfa8a90a9922 Mon Sep 17 00:00:00 2001
From: Brian Smith <brian@briansmith.org>
Date: Mon, 21 Mar 2016 13:47:32 -1000
Subject: [PATCH] Verify consistency of RSA keys after generation & parsing.

Call |RSA_check_key| after parsing an RSA private key in order to
verify that the key is consistent. This is consistent with ECC key
parsing, which does a similar key check.

Call |RSA_check_key| after key generation mostly as a way of
double-checking the key generation was done correctly. A similar check
was not added to |EC_KEY_generate| because |EC_KEY_generate| is used
for generating ephemeral ECDH keys, and the check would be too
expensive for that use.

Change-Id: I5759d0d101c00711bbc30f81a3759f8bff01427c
Reviewed-on: https://boringssl-review.googlesource.com/7522
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
---
 crypto/rsa/rsa_asn1.c  |  5 +++++
 crypto/rsa/rsa_impl.c  |  9 ++++++++-
 crypto/rsa/rsa_test.cc | 13 +++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/crypto/rsa/rsa_asn1.c b/crypto/rsa/rsa_asn1.c
index 509f1aa4..4adb499f 100644
--- a/crypto/rsa/rsa_asn1.c
+++ b/crypto/rsa/rsa_asn1.c
@@ -280,6 +280,11 @@ RSA *RSA_parse_private_key(CBS *cbs) {
     goto err;
   }
 
+  if (!RSA_check_key(ret)) {
+    OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_RSA_PARAMETERS);
+    goto err;
+  }
+
   BN_CTX_free(ctx);
   BN_free(product_of_primes_so_far);
   return ret;
diff --git a/crypto/rsa/rsa_impl.c b/crypto/rsa/rsa_impl.c
index fb7a3686..1ee0d82e 100644
--- a/crypto/rsa/rsa_impl.c
+++ b/crypto/rsa/rsa_impl.c
@@ -1079,10 +1079,17 @@ int rsa_default_multi_prime_keygen(RSA *rsa, int bits, int num_primes,
     }
   }
 
-  ok = 1;
   rsa->additional_primes = additional_primes;
   additional_primes = NULL;
 
+  /* The key generation process is complex and thus error-prone. It could be
+   * disastrous to generate and then use a bad key so double-check that the key
+   * makes sense. */
+  ok = RSA_check_key(rsa);
+  if (!ok) {
+    OPENSSL_PUT_ERROR(RSA, RSA_R_INTERNAL_ERROR);
+  }
+
 err:
   if (ok == -1) {
     OPENSSL_PUT_ERROR(RSA, ERR_LIB_BN);
diff --git a/crypto/rsa/rsa_test.cc b/crypto/rsa/rsa_test.cc
index 8c4a7871..00fd1292 100644
--- a/crypto/rsa/rsa_test.cc
+++ b/crypto/rsa/rsa_test.cc
@@ -688,6 +688,19 @@ static bool TestBadKey() {
     return false;
   }
 
+  uint8_t *der;
+  size_t der_len;
+  if (!RSA_private_key_to_bytes(&der, &der_len, key.get())) {
+    fprintf(stderr, "RSA_private_key_to_bytes failed to serialize bad key\n.");
+    return false;
+  }
+  bssl::UniquePtr<uint8_t> delete_der(der);
+
+  key.reset(RSA_private_key_from_bytes(der, der_len));
+  if (key) {
+    fprintf(stderr, "RSA_private_key_from_bytes accepted bad key\n.");
+  }
+
   ERR_clear_error();
   return true;
 }