Use X509_get0_pubkey to simplify things slightly

Also X509_REQ_check_private_key didn't handle unknown key type case.
Clean those up and align with X509_check_private_key.

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

Author: davidben

crypto/x509/t_req.c

@@ -80,7 +80,6 @@ int X509_REQ_print_fp(FILE *fp, X509_REQ *x) {
 int X509_REQ_print_ex(BIO *bio, X509_REQ *x, unsigned long nmflags,
                       unsigned long cflag) {
   long l;
-  EVP_PKEY *pkey;
   STACK_OF(X509_ATTRIBUTE) *sk;
   char mlch = ' ';
 
@@ -127,13 +126,12 @@ int X509_REQ_print_ex(BIO *bio, X509_REQ *x, unsigned long nmflags,
       goto err;
     }
 
-    pkey = X509_REQ_get_pubkey(x);
+    const EVP_PKEY *pkey = X509_REQ_get0_pubkey(x);
     if (pkey == NULL) {
       BIO_printf(bio, "%12sUnable to load Public Key\n", "");
       ERR_print_errors(bio);
     } else {
       EVP_PKEY_print_public(bio, pkey, 16, NULL);
-      EVP_PKEY_free(pkey);
     }
   }
 

crypto/x509/t_x509.c

@@ -212,13 +212,12 @@ int X509_print_ex(BIO *bp, X509 *x, unsigned long nmflags,
       return 0;
     }
 
-    EVP_PKEY *pkey = X509_get_pubkey(x);
+    const EVP_PKEY *pkey = X509_get0_pubkey(x);
     if (pkey == NULL) {
       BIO_printf(bp, "%12sUnable to load Public Key\n", "");
       ERR_print_errors(bp);
     } else {
       EVP_PKEY_print_public(bp, pkey, 16, NULL);
-      EVP_PKEY_free(pkey);
     }
   }
 

crypto/x509/x509_cmp.c

@@ -219,14 +219,14 @@ X509 *X509_find_by_subject(const STACK_OF(X509) *sk, X509_NAME *name) {
 }
 
 EVP_PKEY *X509_get0_pubkey(const X509 *x) {
-    if ((x == NULL) || (x->cert_info == NULL)) {
-        return NULL;
-    }
-    return (X509_PUBKEY_get0(x->cert_info->key));
+  if (x == NULL) {
+    return NULL;
+  }
+  return X509_PUBKEY_get0(x->cert_info->key);
 }
 
 EVP_PKEY *X509_get_pubkey(const X509 *x) {
-  if ((x == NULL) || (x->cert_info == NULL)) {
+  if (x == NULL) {
     return NULL;
   }
   return X509_PUBKEY_get(x->cert_info->key);
@@ -239,36 +239,29 @@ ASN1_BIT_STRING *X509_get0_pubkey_bitstr(const X509 *x) {
   return x->cert_info->key->public_key;
 }
 
-int X509_check_private_key(X509 *x, const EVP_PKEY *k) {
-  EVP_PKEY *xk;
-  int ret;
-
-  xk = X509_get_pubkey(x);
+int X509_check_private_key(const X509 *x, const EVP_PKEY *k) {
+  const EVP_PKEY *xk = X509_get0_pubkey(x);
+  if (xk == NULL) {
+    return 0;
+  }
 
-  if (xk) {
-    ret = EVP_PKEY_cmp(xk, k);
-  } else {
-    ret = -2;
+  int ret = EVP_PKEY_cmp(xk, k);
+  if (ret > 0) {
+    return 1;
   }
 
   switch (ret) {
-    case 1:
-      break;
     case 0:
       OPENSSL_PUT_ERROR(X509, X509_R_KEY_VALUES_MISMATCH);
-      break;
+      return 0;
     case -1:
       OPENSSL_PUT_ERROR(X509, X509_R_KEY_TYPE_MISMATCH);
-      break;
+      return 0;
     case -2:
       OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_KEY_TYPE);
+      return 0;
   }
-  if (xk) {
-    EVP_PKEY_free(xk);
-  }
-  if (ret > 0) {
-    return 1;
-  }
+
   return 0;
 }
 

crypto/x509/x509_req.c

@@ -77,46 +77,49 @@ X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req) {
 }
 
 EVP_PKEY *X509_REQ_get_pubkey(const X509_REQ *req) {
-  if ((req == NULL) || (req->req_info == NULL)) {
+  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));
+  return X509_PUBKEY_get(req->req_info->pubkey);
 }
 
 EVP_PKEY *X509_REQ_get0_pubkey(const X509_REQ *req) {
-  if ((req == NULL) || (req->req_info == NULL)) {
+  if (req == NULL || req->req_info == NULL) {
     OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
     return NULL;
   }
-  return (X509_PUBKEY_get0(req->req_info->pubkey));
+  return X509_PUBKEY_get0(req->req_info->pubkey);
 }
 
-int X509_REQ_check_private_key(X509_REQ *x, const EVP_PKEY *k) {
-  EVP_PKEY *xk = NULL;
-  int ok = 0;
+int X509_REQ_check_private_key(const X509_REQ *x, const EVP_PKEY *k) {
+  const EVP_PKEY *xk = X509_REQ_get0_pubkey(x);
+  if (xk == NULL) {
+    return 0;
+  }
+
+  int ret = EVP_PKEY_cmp(xk, k);
+  if (ret > 0) {
+    return 1;
+  }
 
-  xk = X509_REQ_get_pubkey(x);
-  switch (EVP_PKEY_cmp(xk, k)) {
-    case 1:
-      ok = 1;
-      break;
+  switch (ret) {
     case 0:
       OPENSSL_PUT_ERROR(X509, X509_R_KEY_VALUES_MISMATCH);
-      break;
+      return 0;
     case -1:
       OPENSSL_PUT_ERROR(X509, X509_R_KEY_TYPE_MISMATCH);
-      break;
+      return 0;
     case -2:
       if (EVP_PKEY_id(k) == EVP_PKEY_EC) {
         OPENSSL_PUT_ERROR(X509, ERR_R_EC_LIB);
-        break;
+      } else {
+        OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_KEY_TYPE);
       }
-      OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_KEY_TYPE);
+      return 0;
   }
 
-  EVP_PKEY_free(xk);
-  return ok;
+  return 0;
 }
 
 int X509_REQ_extension_nid(int req_nid) {

crypto/x509/x509_vfy.c

@@ -1182,8 +1182,6 @@ static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x) {
 // Check CRL validity
 static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) {
   X509 *issuer = NULL;
-  EVP_PKEY *ikey = NULL;
-  int ok = 0;
   int cnum = ctx->error_depth;
   int chnum = (int)sk_X509_num(ctx->chain) - 1;
   // if we have an alternative CRL issuer cert use that
@@ -1200,9 +1198,8 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) {
     // If not self signed, can't check signature
     if (!x509_check_issued_with_callback(ctx, issuer, issuer)) {
       ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL_ISSUER;
-      ok = ctx->verify_cb(0, ctx);
-      if (!ok) {
-        goto err;
+      if (!ctx->verify_cb(0, ctx)) {
+        return 0;
       }
     }
   }
@@ -1212,61 +1209,50 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) {
     if ((issuer->ex_flags & EXFLAG_KUSAGE) &&
         !(issuer->ex_kusage & X509v3_KU_CRL_SIGN)) {
       ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN;
-      ok = ctx->verify_cb(0, ctx);
-      if (!ok) {
-        goto err;
+      if (!ctx->verify_cb(0, ctx)) {
+        return 0;
       }
     }
 
     if (!(ctx->current_crl_score & CRL_SCORE_SCOPE)) {
       ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE;
-      ok = ctx->verify_cb(0, ctx);
-      if (!ok) {
-        goto err;
+      if (!ctx->verify_cb(0, ctx)) {
+        return 0;
       }
     }
 
     if (crl->idp_flags & IDP_INVALID) {
       ctx->error = X509_V_ERR_INVALID_EXTENSION;
-      ok = ctx->verify_cb(0, ctx);
-      if (!ok) {
-        goto err;
+      if (!ctx->verify_cb(0, ctx)) {
+        return 0;
       }
     }
 
     if (!(ctx->current_crl_score & CRL_SCORE_TIME)) {
-      ok = check_crl_time(ctx, crl, 1);
-      if (!ok) {
-        goto err;
+      if (!check_crl_time(ctx, crl, 1)) {
+        return 0;
       }
     }
 
     // Attempt to get issuer certificate public key
-    ikey = X509_get_pubkey(issuer);
-
+    EVP_PKEY *ikey = X509_get0_pubkey(issuer);
     if (!ikey) {
       ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
-      ok = ctx->verify_cb(0, ctx);
-      if (!ok) {
-        goto err;
+      if (!ctx->verify_cb(0, ctx)) {
+        return 0;
       }
     } else {
       // Verify CRL signature
       if (X509_CRL_verify(crl, ikey) <= 0) {
         ctx->error = X509_V_ERR_CRL_SIGNATURE_FAILURE;
-        ok = ctx->verify_cb(0, ctx);
-        if (!ok) {
-          goto err;
+        if (!ctx->verify_cb(0, ctx)) {
+          return 0;
         }
       }
     }
   }
 
-  ok = 1;
-
-err:
-  EVP_PKEY_free(ikey);
-  return ok;
+  return 1;
 }
 
 // Check certificate against CRL
@@ -1388,7 +1374,6 @@ int x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x509, int suppress_error) {
 static int internal_verify(X509_STORE_CTX *ctx) {
   int ok = 0;
   X509 *xs, *xi;
-  EVP_PKEY *pkey = NULL;
 
   int n = (int)sk_X509_num(ctx->chain);
   ctx->error_depth = n - 1;
@@ -1422,7 +1407,8 @@ static int internal_verify(X509_STORE_CTX *ctx) {
     // explicitly asked for. It doesn't add any security and just wastes
     // time.
     if (xs != xi || (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)) {
-      if ((pkey = X509_get_pubkey(xi)) == NULL) {
+      EVP_PKEY *pkey = X509_get0_pubkey(xi);
+      if (pkey == NULL) {
         ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
         ctx->current_cert = xi;
         ok = ctx->verify_cb(0, ctx);
@@ -1434,12 +1420,9 @@ static int internal_verify(X509_STORE_CTX *ctx) {
         ctx->current_cert = xs;
         ok = ctx->verify_cb(0, ctx);
         if (!ok) {
-          EVP_PKEY_free(pkey);
           goto end;
         }
       }
-      EVP_PKEY_free(pkey);
-      pkey = NULL;
     }
 
   check_cert:

include/openssl/x509.h

@@ -219,7 +219,8 @@ OPENSSL_EXPORT ASN1_BIT_STRING *X509_get0_pubkey_bitstr(const X509 *x509);
 
 // X509_check_private_key returns one if |x509|'s public key matches |pkey| and
 // zero otherwise.
-OPENSSL_EXPORT int X509_check_private_key(X509 *x509, const EVP_PKEY *pkey);
+OPENSSL_EXPORT int X509_check_private_key(const X509 *x509,
+                                          const EVP_PKEY *pkey);
 
 // X509_get0_uids sets |*out_issuer_uid| to a non-owning pointer to the
 // issuerUID field of |x509|, or NULL if |x509| has no issuerUID. It similarly
@@ -1163,7 +1164,7 @@ 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.
-OPENSSL_EXPORT int X509_REQ_check_private_key(X509_REQ *req,
+OPENSSL_EXPORT int X509_REQ_check_private_key(const X509_REQ *req,
                                               const EVP_PKEY *pkey);
 
 // X509_REQ_get_attr_count returns the number of attributes in |req|.