Resolve SSL_PRIVATE_METHOD and certificate slots functionality (#2429)

### Description of changes: 
There was an issue with trying to use `SSL_PRIVATE_KEY_METHOD`
functionality that was working for the most part, but had some edge
scenarios that had stopped working. For example setting the
`privkey_method` argument when calling `SSL_CTX_set_chain_and_key` or
`SSL_set_chain_and_key`. Originally I thought this feature had been in a
complete broken state, but upon further investigation realized that the
bssl runner framework was using this functionality for testing async and
hints functionality. The caveat was the framework was using alternative
functions to directly set the certificate and private key method without
using this entrypoint. In theory this entrypoint should work as it is
for the most part coded correctly, but had a bug in that is tried to use
the private key to determine the slot index when it could possibly be
null. This corrects that behavior to correctly use the public key, to
allow the chain to be set correctly, but still delegate the private key
operations to the registered `SSL_PRIVATE_KEY_METHOD` provided.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

Author: skmcgrail

ssl/internal.h

@@ -461,6 +461,8 @@ class GrowableArray {
 // CBBFinishArray behaves like |CBB_finish| but stores the result in an Array.
 OPENSSL_EXPORT bool CBBFinishArray(CBB *cbb, Array<uint8_t> *out);
 
+OPENSSL_EXPORT UniquePtr<CRYPTO_BUFFER> x509_to_buffer(X509 *x509);
+
 // GetAllNames helps to implement |*_get_all_*_names| style functions. It
 // writes at most |max_out| string pointers to |out| and returns the number that
 // it would have liked to have written. The strings written consist of

ssl/ssl_cert.cc

@@ -262,15 +262,24 @@ static enum leaf_cert_and_privkey_result_t do_leaf_cert_and_privkey_checks(
 // returns |leaf_cert_and_privkey_mismatch|. Otherwise it returns
 // |leaf_cert_and_privkey_ok|.
 static enum leaf_cert_and_privkey_result_t check_leaf_cert_and_privkey(
-    CRYPTO_BUFFER *leaf_buffer, EVP_PKEY *privkey) {
+    CRYPTO_BUFFER *leaf_buffer, EVP_PKEY *privkey, int *out_pubslot_idx) {
   CBS cert_cbs;
   CRYPTO_BUFFER_init_CBS(leaf_buffer, &cert_cbs);
   UniquePtr<EVP_PKEY> pubkey = ssl_cert_parse_pubkey(&cert_cbs);
   if (pubkey == nullptr) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     return leaf_cert_and_privkey_error;
   }
-   return do_leaf_cert_and_privkey_checks(&cert_cbs, pubkey.get(), privkey);
+  enum leaf_cert_and_privkey_result_t result = do_leaf_cert_and_privkey_checks(
+      &cert_cbs, pubkey.get(), privkey);
+  if(out_pubslot_idx != nullptr) {
+    if(result == leaf_cert_and_privkey_ok) {
+      *out_pubslot_idx = ssl_get_certificate_slot_index(pubkey.get());
+    } else {
+      *out_pubslot_idx = -1;
+    }
+  }
+  return result;
 }
 
 static int cert_set_chain_and_key(
@@ -291,7 +300,9 @@ static int cert_set_chain_and_key(
     return 0;
   }
 
-  switch (check_leaf_cert_and_privkey(leaf_buf, privkey)) {
+  int slot_idx = -1;
+
+  switch (check_leaf_cert_and_privkey(leaf_buf, privkey, &slot_idx)) {
     case leaf_cert_and_privkey_error:
       return 0;
     case leaf_cert_and_privkey_mismatch:
@@ -300,27 +311,35 @@ static int cert_set_chain_and_key(
     case leaf_cert_and_privkey_ok:
       break;
   }
+  assert(slot_idx >= 0);
 
   if (!ssl_cert_check_cert_private_keys_usage(cert)) {
     return 0;
   }
 
-  // Certificate slot validity already checked in |check_leaf_cert_and_privkey|.
-  int idx = ssl_get_certificate_slot_index(privkey);
-  assert(idx >= 0);
-  CERT_PKEY *cert_pkey = &cert->cert_private_keys[idx];
+  // Certificate slot validity already checked and set by
+  // |check_leaf_cert_and_privkey|.
+  CERT_PKEY *cert_pkey = &cert->cert_private_keys[slot_idx];
 
-  // Update certificate slot index once all checks have passed.
+  // If privatekey is currently set then reset it.
+  // We either set a new |privatekey| or |privkey_method|
+  // later below.
   if (cert_pkey->privatekey) {
     cert_pkey->privatekey.reset();
   }
-  cert_pkey->privatekey = UpRef(privkey);
-  cert->key_method = privkey_method;
+
+  if(privkey != nullptr) {
+    cert_pkey->privatekey = UpRef(privkey);
+  } else {
+    cert->key_method = privkey_method;
+  }
+
   if (cert_pkey->chain) {
     cert_pkey->chain.reset();
   }
   cert_pkey->chain = std::move(certs);
-  cert->cert_private_key_idx = idx;
+  cert->cert_private_key_idx = slot_idx;
+
   return 1;
 }
 

ssl/ssl_privkey.cc

@@ -104,6 +104,7 @@ static bool ssl_set_pkey(CERT *cert, EVP_PKEY *pkey) {
 
   // Update certificate slot index once all checks have passed.
   cert->cert_private_keys[idx].privatekey = UpRef(pkey);
+  cert->key_method = nullptr; // key_method should be cleared since we've set a private key
   cert->cert_private_key_idx = idx;
   return true;
 }

ssl/ssl_test.cc

@@ -13637,5 +13637,160 @@ TEST(SSLTest, MixContextAndConnection) {
   EXPECT_FALSE(SSL_get_privatekey(ssl2.get()));
 }
 
+static size_t test_ecc_privkey_calls = 0;
+
+static enum ssl_private_key_result_t test_ecc_privkey_complete(SSL *ssl,
+                                                           uint8_t *out,
+                                                           size_t *out_len,
+                                                           size_t max_out) {
+  test_ecc_privkey_calls += 1;
+  return ssl_private_key_success;
+}
+
+static enum ssl_private_key_result_t test_ecc_privkey_sign(
+    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) {
+  bssl::UniquePtr<EVP_PKEY> pkey(GetECDSATestKey());
+
+  if (EVP_PKEY_id(pkey.get()) !=
+      SSL_get_signature_algorithm_key_type(signature_algorithm)) {
+      return ssl_private_key_failure;
+  }
+
+  const EVP_MD *md = SSL_get_signature_algorithm_digest(signature_algorithm);
+  bssl::ScopedEVP_MD_CTX ctx;
+  EVP_PKEY_CTX *pctx = nullptr;
+  if (!EVP_DigestSignInit(ctx.get(), &pctx, md, nullptr,
+                          pkey.get())) {
+    return ssl_private_key_failure;
+  }
+
+  size_t len = 0;
+  if (!EVP_DigestSign(ctx.get(), nullptr, &len, in, in_len) || len > max_out) {
+    return ssl_private_key_failure;
+  }
+
+  *out_len = max_out;
+
+  if (!EVP_DigestSign(ctx.get(), out, out_len, in, in_len)) {
+    return ssl_private_key_failure;
+  }
+
+  return test_ecc_privkey_complete(ssl, out, out_len, max_out);
+}
+
+static enum ssl_private_key_result_t test_ecc_privkey_decrypt(
+    SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, const uint8_t *in,
+    size_t in_len) {
+  return ssl_private_key_failure;
+}
+
+static const SSL_PRIVATE_KEY_METHOD test_ecc_private_key_method = {
+    test_ecc_privkey_sign,
+    test_ecc_privkey_decrypt,
+    test_ecc_privkey_complete,
+};
+
+TEST(SSLTest, SSLPrivateKeyMethod) {
+  {
+    bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+    bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
+
+    bssl::UniquePtr<X509> ecdsa_cert(GetECDSATestCertificate());
+    bssl::UniquePtr<CRYPTO_BUFFER> ecdsa_leaf =
+        x509_to_buffer(ecdsa_cert.get());
+    std::vector<CRYPTO_BUFFER *> chain = {
+        ecdsa_leaf.get(),
+    };
+
+    // Index should be have been set to default, 0, but no key loaded
+    EXPECT_EQ(server_ctx->cert->cert_private_key_idx, SSL_PKEY_RSA);
+    EXPECT_EQ(
+        server_ctx->cert->cert_private_keys[SSL_PKEY_RSA].privatekey.get(),
+        nullptr);
+    EXPECT_EQ(server_ctx->cert->key_method, nullptr);
+
+
+    // Load a certificate chain containg the leaf but set private key method
+    ASSERT_TRUE(SSL_CTX_set_chain_and_key(server_ctx.get(), &chain[0],
+                                          chain.size(), nullptr,
+                                          &test_ecc_private_key_method));
+
+    // Should be initiall zero
+    ASSERT_EQ(test_ecc_privkey_calls, (size_t)0);
+
+    // Index must be ECC key now, but key_method must be set.
+    ASSERT_EQ(server_ctx->cert->cert_private_key_idx, SSL_PKEY_ECC);
+    ASSERT_EQ(server_ctx->cert->key_method, &test_ecc_private_key_method);
+
+    bssl::UniquePtr<SSL> client, server;
+    ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
+                                       server_ctx.get(), ClientConfig(),
+                                       false));
+
+    ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
+
+    ASSERT_EQ(test_ecc_privkey_calls, (size_t)1);
+
+    // Check the internal slot index to verify that the correct slot was used
+    // during the handshake.
+    ASSERT_EQ(server->config->cert->cert_private_key_idx, SSL_PKEY_ECC);
+    ASSERT_EQ(server->config->cert->key_method, &test_ecc_private_key_method);
+  }
+
+  {
+    size_t current_invoke_count = test_ecc_privkey_calls;
+
+    bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+    bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
+
+    // Index should be have been set to default, 0, but no key loaded
+    EXPECT_EQ(server_ctx->cert->cert_private_key_idx, SSL_PKEY_RSA);
+    EXPECT_EQ(
+        server_ctx->cert->cert_private_keys[SSL_PKEY_RSA].privatekey.get(),
+        nullptr);
+    EXPECT_EQ(server_ctx->cert->key_method, nullptr);
+
+    bssl::UniquePtr<X509> ed_cert(GetED25519TestCertificate());
+    bssl::UniquePtr<EVP_PKEY> ed_key(GetED25519TestKey());
+    bssl::UniquePtr<CRYPTO_BUFFER> ed_leaf = x509_to_buffer(ed_cert.get());
+    std::vector<CRYPTO_BUFFER *> ed_chain = {
+        ed_leaf.get(),
+    };
+
+    // Load a certificate chain containg the leaf but set private key method
+    ASSERT_TRUE(SSL_CTX_set_chain_and_key(server_ctx.get(), &ed_chain[0],
+                                          ed_chain.size(), ed_key.get(),
+                                          nullptr));
+
+    // Index must be ECC key now, but key_method must not be set.
+    ASSERT_EQ(server_ctx->cert->cert_private_key_idx, SSL_PKEY_ED25519);
+    ASSERT_EQ(server_ctx->cert->key_method, nullptr);
+
+    std::vector<uint16_t> sigalgs = {SSL_SIGN_ED25519};
+
+    ASSERT_TRUE(SSL_CTX_set_signing_algorithm_prefs(
+        client_ctx.get(), sigalgs.data(), sigalgs.size()));
+    ASSERT_TRUE(SSL_CTX_set_verify_algorithm_prefs(
+        client_ctx.get(), sigalgs.data(), sigalgs.size()));
+
+    bssl::UniquePtr<SSL> client, server;
+    ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
+                                       server_ctx.get(), ClientConfig(),
+                                       false));
+
+    ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
+
+    // This should still be the same, as we didn't use the private key method
+    // functionality, so it shouldn't have incremented.
+    ASSERT_EQ(test_ecc_privkey_calls, current_invoke_count);
+
+    // Check the internal slot index to verify that the correct slot was used
+    // during the handshake and that key_method was not set.
+    ASSERT_EQ(server->config->cert->cert_private_key_idx, SSL_PKEY_ED25519);
+    ASSERT_EQ(server->config->cert->key_method, nullptr);
+  }
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

ssl/ssl_x509.cc

@@ -170,7 +170,7 @@ static void check_ssl_ctx_x509_method(const SSL_CTX *ctx) {
 
 // x509_to_buffer returns a |CRYPTO_BUFFER| that contains the serialised
 // contents of |x509|.
-static UniquePtr<CRYPTO_BUFFER> x509_to_buffer(X509 *x509) {
+UniquePtr<CRYPTO_BUFFER> x509_to_buffer(X509 *x509) {
   uint8_t *buf = NULL;
   int cert_len = i2d_X509(x509, &buf);
   if (cert_len <= 0) {