Resolve SSL_PRIVATE_METHOD and certificate slots functionality

Author: skmcgrail

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 are will either and a new privatekey or will be setting
+  // a |privkey_method| which will affect all slots.
   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,164 @@ 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,
+};
+
+static bssl::UniquePtr<CRYPTO_BUFFER> x509_to_buffer(X509 *x509) {
+  uint8_t *buf = NULL;
+  int cert_len = i2d_X509(x509, &buf);
+  if (cert_len <= 0) {
+    return 0;
+  }
+
+  UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(buf, cert_len, NULL));
+  OPENSSL_free(buf);
+
+  return buffer;
+}
+
+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));
+
+    // 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()));
+
+    // 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);
+  }
+
+  {
+    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 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()));
+
+    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_ED25519);
+    ASSERT_EQ(server->config->cert->key_method, nullptr);
+  }
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

ssl/test/runner/runner.go

@@ -1227,11 +1227,11 @@ func gdbOf(path string, args ...string) *exec.Cmd {
 }
 
 func lldbOf(path string, args ...string) *exec.Cmd {
-	xtermArgs := []string{"-geometry", xtermSize, "-e", "lldb", "--"}
+	xtermArgs := []string{"--"}
 	xtermArgs = append(xtermArgs, path)
 	xtermArgs = append(xtermArgs, args...)
 
-	return exec.Command("xterm", xtermArgs...)
+	return exec.Command("lldb", xtermArgs...)
 }
 
 func rrOf(path string, args ...string) *exec.Cmd {