RSA: handle gracefully when a SSL_PRIVATE_KEY_METHOD has NULL methods.

Instead of UB, this now returns an error.

This makes sense, given these can both sign and decrypt, but there are
conceivable applications where only one is necessary.

Change-Id: Idec6c00d17594aa604170b978370be396a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95607
Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Xiangfei Ding <xfding@google.com>
Reviewed-by: Xiangfei Ding <xfding@google.com>

ssl/ssl_privkey.cc

diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index 2947529..b48becd 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -261,8 +261,16 @@
   assert(!hs->can_release_private_key);
 
   if (key_method != nullptr) {
+    if (key_method->sign == nullptr) {
+      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+      return ssl_private_key_failure;
+    }
     enum ssl_private_key_result_t ret;
     if (hs->pending_private_key_op) {
+      if (key_method->complete == nullptr) {
+        OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+        return ssl_private_key_failure;
+      }
       ret = key_method->complete(ssl, out, out_len, max_out);
     } else {
       ret = key_method->sign(ssl, out, out_len, max_out, sigalg, in.data(),
@@ -321,8 +329,16 @@
   const SSLCredential *const cred = hs->credential.get();
   assert(!hs->can_release_private_key);
   if (cred->key_method != nullptr) {
+    if (cred->key_method->decrypt == nullptr) {
+      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+      return ssl_private_key_failure;
+    }
     enum ssl_private_key_result_t ret;
     if (hs->pending_private_key_op) {
+      if (cred->key_method->complete == nullptr) {
+        OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+        return ssl_private_key_failure;
+      }
       ret = cred->key_method->complete(ssl, out, out_len, max_out);
     } else {
       ret = cred->key_method->decrypt(ssl, out, out_len, max_out, in.data(),

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index a876718..a91a626 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5504,6 +5504,57 @@
   ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get()));
 }
 
+TEST(SSLTest, NullDecryptPrivateKeyMethod) {
+  bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+  ASSERT_TRUE(key);
+  bssl::UniquePtr<X509> leaf = GetTestCertificate();
+  ASSERT_TRUE(leaf);
+
+  bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+  ASSERT_TRUE(client_ctx);
+  bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
+  ASSERT_TRUE(server_ctx);
+
+  ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), leaf.get()));
+
+  // Configure an SSL_PRIVATE_KEY_METHOD on the server with sign and complete,
+  // but NULL decrypt.
+  static const SSL_PRIVATE_KEY_METHOD kNullDecryptMethod = {
+      [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out,
+         uint16_t signature_algorithm, const uint8_t *in,
+         size_t in_len) { return ssl_private_key_failure; },
+      nullptr,  // decrypt
+      [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out) {
+        return ssl_private_key_failure;
+      },
+  };
+
+  SSL_CTX_set_private_key_method(server_ctx.get(), &kNullDecryptMethod);
+
+  // Negotiate RSA key exchange (SSL_kRSA). We do this by restricting both
+  // client and server to TLS 1.2 and configuring an RSA key exchange cipher
+  // suite.
+  ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_2_VERSION));
+  ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), TLS1_2_VERSION));
+
+  ASSERT_TRUE(SSL_CTX_set_cipher_list(client_ctx.get(), "AES128-GCM-SHA256"));
+  ASSERT_TRUE(SSL_CTX_set_cipher_list(server_ctx.get(), "AES128-GCM-SHA256"));
+
+  // Use a custom verify on client to accept the self-signed test cert.
+  SSL_CTX_set_custom_verify(client_ctx.get(), SSL_VERIFY_PEER,
+                            AcceptAnyCertificate);
+
+  bssl::UniquePtr<SSL> client, server;
+  // Since the decrypt hook is NULL, the server's decryption during the RSA key
+  // exchange should fail cleanly.
+  EXPECT_FALSE(ConnectClientAndServer(&client, &server, client_ctx.get(),
+                                      server_ctx.get()));
+  uint32_t err = ERR_get_error();
+  EXPECT_TRUE(ErrorEquals(err, ERR_LIB_SSL, ERR_R_INTERNAL_ERROR));
+  EXPECT_EQ(0u, ERR_get_error());
+}
+
+
 // Configuring a chain and then overwriting it with a different chain should
 // clear the old one.
 TEST(SSLTest, OverrideChain) {