Eagerly compute the cached EVP_PKEY in X509_PUBKEY

Whenever the object is mutated, we can simply refresh the cached
EVP_PKEY. This aligns with OpenSSL, which computes it eagerly these
days. This removes the need to lock things, and also makes it easy to
implement the get0 versions of the functions from OpenSSL.

Change-Id: Ib17b654af694817edc43e4742d9baf9ed05c676e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65050
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
(cherry picked from commit 5b3dc49c1271554f73b976c2c625600d6bd912b0)

Author: davidben

crypto/x509/internal.h

@@ -368,12 +368,6 @@ ASN1_TYPE *ASN1_generate_v3(const char *str, const X509V3_CTX *cnf);
 
 int X509_CERT_AUX_print(BIO *bp, X509_CERT_AUX *x, int indent);
 
-// X509_PUBKEY_get0 decodes the public key in |key| and returns an |EVP_PKEY|
-// on success, or NULL on error. It is similar to |X509_PUBKEY_get|, but it
-// directly returns the reference to |pkey| of |key|. This means that the
-// caller must not free the result after use.
-EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key);
-
 int x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int suppress_error);
 
 // RSA-PSS functions.

crypto/x509/x509_cmp.c

@@ -225,11 +225,11 @@ EVP_PKEY *X509_get0_pubkey(const X509 *x) {
     return (X509_PUBKEY_get0(x->cert_info->key));
 }
 
-EVP_PKEY *X509_get_pubkey(X509 *x) {
+EVP_PKEY *X509_get_pubkey(const X509 *x) {
   if ((x == NULL) || (x->cert_info == NULL)) {
     return NULL;
   }
-  return (X509_PUBKEY_get(x->cert_info->key));
+  return X509_PUBKEY_get(x->cert_info->key);
 }
 
 ASN1_BIT_STRING *X509_get0_pubkey_bitstr(const X509 *x) {

crypto/x509/x509_req.c

@@ -76,15 +76,15 @@ X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req) {
   return req->req_info->subject;
 }
 
-EVP_PKEY *X509_REQ_get_pubkey(X509_REQ *req) {
+EVP_PKEY *X509_REQ_get_pubkey(const X509_REQ *req) {
   if ((req == NULL) || (req->req_info == NULL)) {
     OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
     return NULL;
   }
   return (X509_PUBKEY_get(req->req_info->pubkey));
 }
 
-EVP_PKEY *X509_REQ_get0_pubkey(X509_REQ *req) {
+EVP_PKEY *X509_REQ_get0_pubkey(const X509_REQ *req) {
   if ((req == NULL) || (req->req_info == NULL)) {
     OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
     return NULL;

crypto/x509/x509spki.c

@@ -68,7 +68,7 @@ int NETSCAPE_SPKI_set_pubkey(NETSCAPE_SPKI *x, EVP_PKEY *pkey) {
   return (X509_PUBKEY_set(&(x->spkac->pubkey), pkey));
 }
 
-EVP_PKEY *NETSCAPE_SPKI_get_pubkey(NETSCAPE_SPKI *x) {
+EVP_PKEY *NETSCAPE_SPKI_get_pubkey(const NETSCAPE_SPKI *x) {
   if ((x == NULL) || (x->spkac == NULL)) {
     return NULL;
   }

crypto/x509/x_pubkey.c

@@ -65,26 +65,46 @@
 #include <openssl/evp.h>
 #include <openssl/mem.h>
 #include <openssl/obj.h>
-#include <openssl/thread.h>
 
 #include "../internal.h"
 #include "internal.h"
 
 
 static void x509_pubkey_changed(X509_PUBKEY *pub) {
-  // TODO(davidben): Instead of just dropping the key, also compute the new
-  // cached key. This will let us implement |X509_get0_pubkey| and remove the
-  // need for a mutex.
   EVP_PKEY_free(pub->pkey);
   pub->pkey = NULL;
+
+  // Re-encode the |X509_PUBKEY| to DER and parse it with EVP's APIs.
+  uint8_t *spki = NULL;
+  int spki_len = i2d_X509_PUBKEY(pub, &spki);
+  if (spki_len < 0) {
+    goto err;
+  }
+
+  CBS cbs;
+  CBS_init(&cbs, spki, (size_t)spki_len);
+  EVP_PKEY *pkey = EVP_parse_public_key(&cbs);
+  if (pkey == NULL || CBS_len(&cbs) != 0) {
+    EVP_PKEY_free(pkey);
+    goto err;
+  }
+
+  pub->pkey = pkey;
+
+err:
+  OPENSSL_free(spki);
+  // If the operation failed, clear errors. An |X509_PUBKEY| whose key we cannot
+  // parse is still a valid SPKI. It just cannot be converted to an |EVP_PKEY|.
+  ERR_clear_error();
 }
 
-// Minor tweak to operation: free up EVP_PKEY
 static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
                      void *exarg) {
+  X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval;
   if (operation == ASN1_OP_FREE_POST) {
-    X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval;
     EVP_PKEY_free(pubkey->pkey);
+  } else if (operation == ASN1_OP_D2I_POST) {
+    x509_pubkey_changed(pubkey);
   }
   return 1;
 }
@@ -133,76 +153,25 @@ int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey) {
   return 0;
 }
 
-// g_pubkey_lock is used to protect the initialisation of the |pkey| member of
-// |X509_PUBKEY| objects. Really |X509_PUBKEY| should have a |CRYPTO_once_t|
-// inside it for this, but |CRYPTO_once_t| is private and |X509_PUBKEY| is
-// not.
-static struct CRYPTO_STATIC_MUTEX g_pubkey_lock = CRYPTO_STATIC_MUTEX_INIT;
-
-// x509_pubkey_decode decodes |key| into |pkey|. It returns one on success and
-// zero on error.
-static int x509_pubkey_decode(EVP_PKEY **pkey, X509_PUBKEY *key) {
-  CRYPTO_STATIC_MUTEX_lock_read(&g_pubkey_lock);
-  if (key->pkey != NULL) {
-    CRYPTO_STATIC_MUTEX_unlock_read(&g_pubkey_lock);
-    *pkey = key->pkey;
-    return 1;
+EVP_PKEY *X509_PUBKEY_get0(const X509_PUBKEY *key) {
+  if (key == NULL) {
+    return NULL;
   }
-  CRYPTO_STATIC_MUTEX_unlock_read(&g_pubkey_lock);
 
-  uint8_t *spki = NULL;
-  // Re-encode the |X509_PUBKEY| to DER and parse it.
-  int spki_len = i2d_X509_PUBKEY(key, &spki);
-  if (spki_len < 0) {
-    goto error;
-  }
-  CBS cbs;
-  CBS_init(&cbs, spki, (size_t)spki_len);
-  *pkey = EVP_parse_public_key(&cbs);
-  if (*pkey == NULL || CBS_len(&cbs) != 0) {
+  if (key->pkey == NULL) {
     OPENSSL_PUT_ERROR(X509, X509_R_PUBLIC_KEY_DECODE_ERROR);
-    goto error;
-  }
-
-  // Check to see if another thread set key->pkey first
-  CRYPTO_STATIC_MUTEX_lock_write(&g_pubkey_lock);
-  if (key->pkey) {
-    CRYPTO_STATIC_MUTEX_unlock_write(&g_pubkey_lock);
-    EVP_PKEY_free(*pkey);
-    *pkey = key->pkey;
-  } else {
-    key->pkey = *pkey;
-    CRYPTO_STATIC_MUTEX_unlock_write(&g_pubkey_lock);
-  }
-  OPENSSL_free(spki);
-  return 1;
-error:
-  OPENSSL_free(spki);
-  return 0;
-}
-
-EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key) {
-  if (key == NULL) {
-    OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
     return NULL;
   }
 
-  EVP_PKEY *ret = NULL;
-  if (!x509_pubkey_decode(&ret, key)) {
-    EVP_PKEY_free(ret);
-    return NULL;
-  };
-  return ret;
+  return key->pkey;
 }
 
-EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) {
-  EVP_PKEY *ret = X509_PUBKEY_get0(key);
-
-  if (ret != NULL && !EVP_PKEY_up_ref(ret)) {
-    OPENSSL_PUT_ERROR(X509, ERR_R_INTERNAL_ERROR);
-    ret = NULL;
+EVP_PKEY *X509_PUBKEY_get(const X509_PUBKEY *key) {
+  EVP_PKEY *pkey = X509_PUBKEY_get0(key);
+  if (pkey != NULL) {
+    EVP_PKEY_up_ref(pkey);
   }
-  return ret;
+  return pkey;
 }
 
 int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *obj, int param_type,

include/openssl/x509.h

@@ -199,17 +199,15 @@ OPENSSL_EXPORT X509_NAME *X509_get_subject_name(const X509 *x509);
 OPENSSL_EXPORT X509_PUBKEY *X509_get_X509_PUBKEY(const X509 *x509);
 
 // X509_get0_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the
-// public key was unsupported or could not be decoded. It is similar to
-// |X509_get_pubkey|, but it does not increment the reference count of the
-// returned |EVP_PKEY|. This means that the caller must not free the result
-// after use.
-OPENSSL_EXPORT EVP_PKEY *X509_get0_pubkey(const X509 *x);
-
-// X509_get_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the
-// public key was unsupported or could not be decoded. This function returns a
-// reference to the |EVP_PKEY|. The caller must release the result with
-// |EVP_PKEY_free| when done.
-OPENSSL_EXPORT EVP_PKEY *X509_get_pubkey(X509 *x509);
+// public key was unsupported or could not be decoded. The |EVP_PKEY| is cached
+// in |x509|, so callers must not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_get0_pubkey(const X509 *x509);
+
+// X509_get_pubkey behaves like |X509_get0_pubkey| but increments the reference
+// count on the |EVP_PKEY|. The caller must release the result with
+// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |x509|, so callers
+// must not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_get_pubkey(const X509 *x509);
 
 // X509_get0_pubkey_bitstr returns the BIT STRING portion of |x509|'s public
 // key. Note this does not contain the AlgorithmIdentifier portion.
@@ -1152,15 +1150,16 @@ OPENSSL_EXPORT long X509_REQ_get_version(const X509_REQ *req);
 // not const-correct for legacy reasons.
 OPENSSL_EXPORT X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req);
 
-// X509_REQ_get_pubkey returns |req|'s public key as an |EVP_PKEY|, or NULL if
-// the public key was unsupported or could not be decoded. This function returns
-// a reference to the |EVP_PKEY|. The caller must release the result with
-// |EVP_PKEY_free| when done.
-OPENSSL_EXPORT EVP_PKEY *X509_REQ_get_pubkey(X509_REQ *req);
+// X509_REQ_get0_pubkey returns |req|'s public key as an |EVP_PKEY|, or NULL if
+// the public key was unsupported or could not be decoded. The |EVP_PKEY| is
+// cached in |req|, so callers must not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_REQ_get0_pubkey(const X509_REQ *req);
 
-// X509_REQ_get0_pubkey is like |X509_REQ_get_pubkey|, but directly returns the
-// reference to |req|. The caller must not free the result after use.
-OPENSSL_EXPORT EVP_PKEY *X509_REQ_get0_pubkey(X509_REQ *req);
+// X509_REQ_get_pubkey behaves like |X509_REQ_get0_pubkey| but increments the
+// reference count on the |EVP_PKEY|. The caller must release the result with
+// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |req|, so callers must
+// not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_REQ_get_pubkey(const X509_REQ *req);
 
 // X509_REQ_check_private_key returns one if |req|'s public key matches |pkey|
 // and zero otherwise.
@@ -1643,11 +1642,16 @@ OPENSSL_EXPORT int i2d_X509_PUBKEY(const X509_PUBKEY *key, uint8_t **outp);
 // object, and returns one. Otherwise, it returns zero.
 OPENSSL_EXPORT int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey);
 
-// X509_PUBKEY_get decodes the public key in |key| and returns an |EVP_PKEY| on
-// success, or NULL on error or unrecognized algorithm. The caller must release
-// the result with |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |key|,
-// so callers must not mutate the result.
-OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key);
+// X509_PUBKEY_get0 returns |key| as an |EVP_PKEY|, or NULL if |key| either
+// could not be parsed or is an unrecognized algorithm. The |EVP_PKEY| is cached
+// in |key|, so callers must not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get0(const X509_PUBKEY *key);
+
+// X509_PUBKEY_get behaves like |X509_PUBKEY_get0| but increments the reference
+// count on the |EVP_PKEY|. The caller must release the result with
+// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |key|, so callers must
+// not mutate the result.
+OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get(const X509_PUBKEY *key);
 
 // X509_PUBKEY_set0_param sets |pub| to a key with AlgorithmIdentifier
 // determined by |obj|, |param_type|, and |param_value|, and an encoded
@@ -2295,7 +2299,7 @@ OPENSSL_EXPORT char *NETSCAPE_SPKI_b64_encode(NETSCAPE_SPKI *spki);
 // NETSCAPE_SPKI_get_pubkey decodes and returns the public key in |spki| as an
 // |EVP_PKEY|, or NULL on error. The caller takes ownership of the resulting
 // pointer and must call |EVP_PKEY_free| when done.
-OPENSSL_EXPORT EVP_PKEY *NETSCAPE_SPKI_get_pubkey(NETSCAPE_SPKI *spki);
+OPENSSL_EXPORT EVP_PKEY *NETSCAPE_SPKI_get_pubkey(const NETSCAPE_SPKI *spki);
 
 // NETSCAPE_SPKI_set_pubkey sets |spki|'s public key to |pkey|. It returns one
 // on success or zero on error. This function does not take ownership of |pkey|,