From 7199b42c3982899c973c524f49dc2ca40c0e9162 Mon Sep 17 00:00:00 2001 From: tobhe Date: Mon, 13 Dec 2021 17:35:34 +0000 Subject: [PATCH] Cleanup libcrypto memory management. Remove redundant NULL checks before calling *_free() functions. Use 'get0' functions where it makes sense to avoid some frees. Feedback and ok tb@ --- sbin/iked/ca.c | 98 ++++++++++++++++------------------------------ sbin/iked/crypto.c | 36 +++++++---------- sbin/iked/ocsp.c | 48 ++++++++--------------- 3 files changed, 64 insertions(+), 118 deletions(-) diff --git a/sbin/iked/ca.c b/sbin/iked/ca.c index a2b419018d7..b06247136f9 100644 --- a/sbin/iked/ca.c +++ b/sbin/iked/ca.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ca.c,v 1.83 2021/12/08 19:17:35 tobhe Exp $ */ +/* $OpenBSD: ca.c,v 1.84 2021/12/13 17:35:34 tobhe Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -187,10 +187,8 @@ ca_reset(struct privsep *ps) store->ca_pubkey.id_type == IKEV2_ID_NONE) fatalx("ca_reset: keys not loaded"); - if (store->ca_cas != NULL) - X509_STORE_free(store->ca_cas); - if (store->ca_certs != NULL) - X509_STORE_free(store->ca_certs); + X509_STORE_free(store->ca_cas); + X509_STORE_free(store->ca_certs); if ((store->ca_cas = X509_STORE_new()) == NULL) fatalx("ca_reset: failed to get ca store"); @@ -483,9 +481,8 @@ ca_getcert(struct iked *env, struct imsg *imsg) cert = ca_by_subjectaltname(store->ca_certs, &id); if (cert) { log_debug("%s: found local cert", __func__); - if ((certkey = X509_get_pubkey(cert)) != NULL) { + if ((certkey = X509_get0_pubkey(cert)) != NULL) { ret = ca_pubkey_serialize(certkey, &key); - EVP_PKEY_free(certkey); if (ret == 0) { ptr = ibuf_data(key.id_buf); len = ibuf_length(key.id_buf); @@ -1045,7 +1042,7 @@ ca_cert_local(struct iked *env, X509 *cert) if ((localpub = ca_id_to_pkey(&store->ca_pubkey)) == NULL) goto done; - if ((certkey = X509_get_pubkey(cert)) == NULL) { + if ((certkey = X509_get0_pubkey(cert)) == NULL) { log_info("%s: no public key in cert", __func__); goto done; } @@ -1057,10 +1054,8 @@ ca_cert_local(struct iked *env, X509 *cert) ret = 1; done: - if (certkey != NULL) - EVP_PKEY_free(certkey); - if (localpub != NULL) - EVP_PKEY_free(localpub); + EVP_PKEY_free(localpub); + return (ret); } @@ -1092,8 +1087,7 @@ ca_cert_info(const char *msg, X509 *cert) log_info("%s: subject: %s", msg, buf); ca_x509_subjectaltname_log(cert, msg); out: - if (rawserial) - BIO_free(rawserial); + BIO_free(rawserial); } int @@ -1112,10 +1106,9 @@ ca_subjectpubkey_digest(X509 *x509, uint8_t *md, unsigned int *size) * that includes the public key type (eg. RSA) and the * public key value (see 3.7 of RFC7296). */ - if ((pkey = X509_get_pubkey(x509)) == NULL) + if ((pkey = X509_get0_pubkey(x509)) == NULL) return (-1); buflen = i2d_PUBKEY(pkey, &buf); - EVP_PKEY_free(pkey); if (buflen == 0) return (-1); if (!EVP_Digest(buf, buflen, md, size, EVP_sha1(), NULL)) { @@ -1196,7 +1189,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id) ibuf_release(id->id_buf); id->id_buf = NULL; - if ((rsa = EVP_PKEY_get1_RSA(key)) == NULL) + if ((rsa = EVP_PKEY_get0_RSA(key)) == NULL) goto done; if ((len = i2d_RSAPublicKey(rsa, NULL)) <= 0) goto done; @@ -1218,7 +1211,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id) ibuf_release(id->id_buf); id->id_buf = NULL; - if ((ec = EVP_PKEY_get1_EC_KEY(key)) == NULL) + if ((ec = EVP_PKEY_get0_EC_KEY(key)) == NULL) goto done; if ((len = i2d_EC_PUBKEY(ec, NULL)) <= 0) goto done; @@ -1245,10 +1238,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id) ret = 0; done: - if (rsa != NULL) - RSA_free(rsa); - if (ec != NULL) - EC_KEY_free(ec); + return (ret); } @@ -1268,7 +1258,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id) ibuf_release(id->id_buf); id->id_buf = NULL; - if ((rsa = EVP_PKEY_get1_RSA(key)) == NULL) + if ((rsa = EVP_PKEY_get0_RSA(key)) == NULL) goto done; if ((len = i2d_RSAPrivateKey(rsa, NULL)) <= 0) goto done; @@ -1290,7 +1280,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id) ibuf_release(id->id_buf); id->id_buf = NULL; - if ((ec = EVP_PKEY_get1_EC_KEY(key)) == NULL) + if ((ec = EVP_PKEY_get0_EC_KEY(key)) == NULL) goto done; if ((len = i2d_ECPrivateKey(ec, NULL)) <= 0) goto done; @@ -1317,10 +1307,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id) ret = 0; done: - if (rsa != NULL) - RSA_free(rsa); - if (ec != NULL) - EC_KEY_free(ec); + return (ret); } @@ -1354,14 +1341,11 @@ ca_id_to_pkey(struct iked_id *pubkey) out = localkey; localkey = NULL; done: - if (localkey != NULL) - EVP_PKEY_free(localkey); - if (localrsa != NULL) - RSA_free(localrsa); - if (localec != NULL) - EC_KEY_free(localec); - if (rawcert != NULL) - BIO_free(rawcert); + EVP_PKEY_free(localkey); + RSA_free(localrsa); + EC_KEY_free(localec); + BIO_free(rawcert); + return (out); } @@ -1403,11 +1387,8 @@ ca_privkey_to_method(struct iked_id *privkey) print_map(method, ikev2_auth_map)); out: - if (ec != NULL) - EC_KEY_free(ec); - if (rawcert != NULL) - BIO_free(rawcert); - + EC_KEY_free(ec); + BIO_free(rawcert); return (method); } @@ -1630,19 +1611,13 @@ ca_validate_pubkey(struct iked *env, struct iked_static_id *id, ca_sslerror(__func__); done: ibuf_release(idp.id_buf); - if (localkey != NULL) - EVP_PKEY_free(localkey); - if (peerrsa != NULL) - RSA_free(peerrsa); - if (peerec != NULL) - EC_KEY_free(peerec); - if (localrsa != NULL) - RSA_free(localrsa); - if (rawcert != NULL) { - BIO_free(rawcert); - if (peerkey != NULL) - EVP_PKEY_free(peerkey); - } + EVP_PKEY_free(localkey); + RSA_free(peerrsa); + EC_KEY_free(peerec); + RSA_free(localrsa); + BIO_free(rawcert); + if (len > 0) + EVP_PKEY_free(peerkey); return (ret); } @@ -1682,12 +1657,11 @@ ca_validate_cert(struct iked *env, struct iked_static_id *id, } if (id != NULL) { - if ((pkey = X509_get_pubkey(cert)) == NULL) { + if ((pkey = X509_get0_pubkey(cert)) == NULL) { errstr = "no public key in cert"; goto done; } ret = ca_validate_pubkey(env, id, pkey, 0, NULL); - EVP_PKEY_free(pkey); if (ret == 0) { errstr = "in public key file, ok"; goto done; @@ -1759,14 +1733,10 @@ ca_validate_cert(struct iked *env, struct iked_static_id *id, } err: - if (rawcert != NULL) { - BIO_free(rawcert); - if (cert != NULL) - X509_free(cert); - } - - if (csc != NULL) - X509_STORE_CTX_free(csc); + if (len > 0) + X509_free(cert); + BIO_free(rawcert); + X509_STORE_CTX_free(csc); return (ret); } diff --git a/sbin/iked/crypto.c b/sbin/iked/crypto.c index 59cac673029..87fb7650c3f 100644 --- a/sbin/iked/crypto.c +++ b/sbin/iked/crypto.c @@ -1,4 +1,4 @@ -/* $OpenBSD: crypto.c,v 1.38 2021/12/01 16:42:12 deraadt Exp $ */ +/* $OpenBSD: crypto.c,v 1.39 2021/12/13 17:35:34 tobhe Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -319,8 +319,7 @@ hash_free(struct iked_hash *hash) { if (hash == NULL) return; - if (hash->hash_ctx != NULL) - HMAC_CTX_free(hash->hash_ctx); + HMAC_CTX_free(hash->hash_ctx); ibuf_release(hash->hash_key); free(hash); } @@ -764,9 +763,8 @@ dsa_free(struct iked_dsa *dsa) if (dsa->dsa_hmac) { HMAC_CTX_free((HMAC_CTX *)dsa->dsa_ctx); } else { - EVP_MD_CTX_destroy((EVP_MD_CTX *)dsa->dsa_ctx); - if (dsa->dsa_key) - EVP_PKEY_free(dsa->dsa_key); + EVP_MD_CTX_free((EVP_MD_CTX *)dsa->dsa_ctx); + EVP_PKEY_free(dsa->dsa_key); } ibuf_release(dsa->dsa_keydata); @@ -842,8 +840,7 @@ dsa_setkey(struct iked_dsa *dsa, void *key, size_t keylen, uint8_t type) goto err; } - if (cert != NULL) - X509_free(cert); + X509_free(cert); BIO_free(rawcert); /* temporary for parsing */ return (dsa->dsa_keydata); @@ -853,16 +850,11 @@ dsa_setkey(struct iked_dsa *dsa, void *key, size_t keylen, uint8_t type) err: log_debug("%s: error", __func__); - if (rsa != NULL) - RSA_free(rsa); - if (ec != NULL) - EC_KEY_free(ec); - if (pkey != NULL) - EVP_PKEY_free(pkey); - if (cert != NULL) - X509_free(cert); - if (rawcert != NULL) - BIO_free(rawcert); + RSA_free(rsa); + EC_KEY_free(ec); + EVP_PKEY_free(pkey); + X509_free(cert); + BIO_free(rawcert); ibuf_release(dsa->dsa_keydata); dsa->dsa_keydata = NULL; return (NULL); @@ -1078,8 +1070,8 @@ _dsa_sign_ecdsa(struct iked_dsa *dsa, uint8_t *ptr, size_t len) ret = 0; done: free(tmp); - if (obj) - ECDSA_SIG_free(obj); + ECDSA_SIG_free(obj); + return (ret); } @@ -1180,8 +1172,8 @@ _dsa_verify_prepare(struct iked_dsa *dsa, uint8_t **sigp, size_t *lenp, BN_clear_free(r); BN_clear_free(s); free(ptr); - if (obj) - ECDSA_SIG_free(obj); + ECDSA_SIG_free(obj); + return (ret); } diff --git a/sbin/iked/ocsp.c b/sbin/iked/ocsp.c index 06d77af3996..0b05a620ffa 100644 --- a/sbin/iked/ocsp.c +++ b/sbin/iked/ocsp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ocsp.c,v 1.22 2021/11/19 21:16:25 tobhe Exp $ */ +/* $OpenBSD: ocsp.c,v 1.23 2021/12/13 17:35:34 tobhe Exp $ */ /* * Copyright (c) 2014 Markus Friedl @@ -322,20 +322,16 @@ ocsp_validate_cert(struct iked *env, void *data, size_t len, ret = proc_composev(&env->sc_ps, PROC_PARENT, IMSG_OCSP_FD, iov, iovcnt); - if (aia) - X509_email_free(aia); /* free stack of openssl strings */ + X509_email_free(aia); /* free stack of openssl strings */ return (ret); err: ca_sslerror(__func__); free(ioe); - if (rawcert != NULL) - BIO_free(rawcert); - if (cert != NULL) - X509_free(cert); - if (id != NULL) - OCSP_CERTID_free(id); + BIO_free(rawcert); + X509_free(cert); + OCSP_CERTID_free(id); ocsp_validate_finish(ocsp, 0); /* failed */ return (-1); } @@ -349,15 +345,9 @@ ocsp_free(struct iked_ocsp *ocsp) close(ocsp->ocsp_sock->sock_fd); free(ocsp->ocsp_sock); } - if (ocsp->ocsp_cbio != NULL) - BIO_free_all(ocsp->ocsp_cbio); - - if (ocsp->ocsp_req_ctx != NULL) - OCSP_REQ_CTX_free(ocsp->ocsp_req_ctx); - - if (ocsp->ocsp_req != NULL) - OCSP_REQUEST_free(ocsp->ocsp_req); - + BIO_free_all(ocsp->ocsp_cbio); + OCSP_REQ_CTX_free(ocsp->ocsp_req_ctx); + OCSP_REQUEST_free(ocsp->ocsp_req); free(ocsp); } } @@ -471,12 +461,10 @@ ocsp_load_certs(const char *file) } done: - if (bio) - BIO_free(bio); - if (xis) - sk_X509_INFO_pop_free(xis, X509_INFO_free); - if (certs && sk_X509_num(certs) <= 0) { - sk_X509_pop_free(certs, X509_free); + BIO_free(bio); + sk_X509_INFO_pop_free(xis, X509_INFO_free); + if (sk_X509_num(certs) <= 0) { + sk_X509_free(certs); certs = NULL; } return (certs); @@ -599,14 +587,10 @@ ocsp_parse_response(struct iked_ocsp *ocsp, OCSP_RESPONSE *resp) if (!valid) { log_debug("%s: status: %s", __func__, errstr); } - if (store) - X509_STORE_free(store); - if (verify_other) - sk_X509_pop_free(verify_other, X509_free); - if (resp) - OCSP_RESPONSE_free(resp); - if (bs) - OCSP_BASICRESP_free(bs); + X509_STORE_free(store); + sk_X509_pop_free(verify_other, X509_free); + OCSP_RESPONSE_free(resp); + OCSP_BASICRESP_free(bs); ocsp_validate_finish(ocsp, valid); } -- 2.20.1