From 8795d8912c8a83aaf900c0260e252a35f64eb200 Mon Sep 17 00:00:00 2001 From: Norbert Pocs Date: Fri, 18 Nov 2022 17:22:46 +0100 Subject: [PATCH] Fix memory leaks of bignums when openssl >= 3.0 The openssl 3.0 support has introduced some memory leaks at key build as OSSL_PARAM_BLD_push_BN duplicates the bignum and does not save the pointer itself. Signed-off-by: Norbert Pocs Reviewed-by: Jakub Jelen --- include/libssh/dh.h | 2 +- src/dh_crypto.c | 28 ++--- src/pki_crypto.c | 262 ++++++++++++++++++++++++-------------------- 3 files changed, 151 insertions(+), 141 deletions(-) diff --git a/include/libssh/dh.h b/include/libssh/dh.h index 353dc233..9b9bb472 100644 --- a/include/libssh/dh.h +++ b/include/libssh/dh.h @@ -53,7 +53,7 @@ int ssh_dh_keypair_get_keys(struct dh_ctx *ctx, int peer, bignum *priv, bignum *pub); #endif /* OPENSSL_VERSION_NUMBER */ int ssh_dh_keypair_set_keys(struct dh_ctx *ctx, int peer, - const bignum priv, const bignum pub); + bignum priv, bignum pub); int ssh_dh_compute_shared_secret(struct dh_ctx *ctx, int local, int remote, bignum *dest); diff --git a/src/dh_crypto.c b/src/dh_crypto.c index a847c6a2..b578ddec 100644 --- a/src/dh_crypto.c +++ b/src/dh_crypto.c @@ -154,12 +154,9 @@ int ssh_dh_keypair_get_keys(struct dh_ctx *ctx, int peer, #endif /* OPENSSL_VERSION_NUMBER */ int ssh_dh_keypair_set_keys(struct dh_ctx *ctx, int peer, - const bignum priv, const bignum pub) + bignum priv, bignum pub) { -#if OPENSSL_VERSION_NUMBER < 0x30000000L - bignum priv_key = NULL; - bignum pub_key = NULL; -#else +#if OPENSSL_VERSION_NUMBER >= 0x30000000L int rc; OSSL_PARAM *params = NULL, *out_params = NULL, *merged_params = NULL; OSSL_PARAM_BLD *param_bld = NULL; @@ -172,7 +169,11 @@ int ssh_dh_keypair_set_keys(struct dh_ctx *ctx, int peer, return SSH_ERROR; } -#if OPENSSL_VERSION_NUMBER >= 0x30000000L +#if OPENSSL_VERSION_NUMBER < 0x30000000L + (void)DH_set0_key(ctx->keypair[peer], pub, priv); + + return SSH_OK; +#else rc = EVP_PKEY_todata(ctx->keypair[peer], EVP_PKEY_KEYPAIR, &out_params); if (rc != 1) { return SSH_ERROR; @@ -195,35 +196,22 @@ int ssh_dh_keypair_set_keys(struct dh_ctx *ctx, int peer, rc = SSH_ERROR; goto out; } -#endif /* OPENSSL_VERSION_NUMBER */ if (priv) { -#if OPENSSL_VERSION_NUMBER < 0x30000000L - priv_key = priv; -#else rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PRIV_KEY, priv); if (rc != 1) { rc = SSH_ERROR; goto out; } -#endif /* OPENSSL_VERSION_NUMBER */ } if (pub) { -#if OPENSSL_VERSION_NUMBER < 0x30000000L - pub_key = pub; -#else rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PUB_KEY, pub); if (rc != 1) { rc = SSH_ERROR; goto out; } -#endif /* OPENSSL_VERSION_NUMBER */ } -#if OPENSSL_VERSION_NUMBER < 0x30000000L - (void)DH_set0_key(ctx->keypair[peer], pub_key, priv_key); - return SSH_OK; -#else params = OSSL_PARAM_BLD_to_param(param_bld); if (params == NULL) { rc = SSH_ERROR; @@ -248,6 +236,8 @@ int ssh_dh_keypair_set_keys(struct dh_ctx *ctx, int peer, rc = SSH_OK; out: + bignum_safe_free(priv); + bignum_safe_free(pub); EVP_PKEY_CTX_free(evp_ctx); OSSL_PARAM_free(out_params); OSSL_PARAM_free(params); diff --git a/src/pki_crypto.c b/src/pki_crypto.c index 0a5003da..d3359e2d 100644 --- a/src/pki_crypto.c +++ b/src/pki_crypto.c @@ -1492,18 +1492,18 @@ int pki_privkey_build_dss(ssh_key key, ssh_string privkey) { int rc; -#if OPENSSL_VERSION_NUMBER < 0x30000000L BIGNUM *bp, *bq, *bg, *bpub_key, *bpriv_key; +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OSSL_PARAM_BLD *param_bld = OSSL_PARAM_BLD_new(); + if (param_bld == NULL) { + return SSH_ERROR; + } #else - const BIGNUM *pb, *qb, *gb, *pubb, *privb; - OSSL_PARAM_BLD *param_bld; -#endif /* OPENSSL_VERSION_NUMBER */ - -#if OPENSSL_VERSION_NUMBER < 0x30000000L key->dsa = DSA_new(); if (key->dsa == NULL) { return SSH_ERROR; } +#endif /* OPENSSL_VERSION_NUMBER */ bp = ssh_make_string_bn(p); bq = ssh_make_string_bn(q); @@ -1512,9 +1512,11 @@ int pki_privkey_build_dss(ssh_key key, bpriv_key = ssh_make_string_bn(privkey); if (bp == NULL || bq == NULL || bg == NULL || bpub_key == NULL) { + rc = SSH_ERROR; goto fail; } +#if OPENSSL_VERSION_NUMBER < 0x30000000L /* Memory management of bp, qq and bg is transferred to DSA object */ rc = DSA_set0_pqg(key->dsa, bp, bq, bg); if (rc == 0) { @@ -1532,39 +1534,43 @@ fail: DSA_free(key->dsa); return SSH_ERROR; #else - param_bld = OSSL_PARAM_BLD_new(); - if (param_bld == NULL) - goto err; - - pb = ssh_make_string_bn(p); - qb = ssh_make_string_bn(q); - gb = ssh_make_string_bn(g); - pubb = ssh_make_string_bn(pubkey); - privb = ssh_make_string_bn(privkey); - - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_P, pb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_Q, qb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_G, gb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PUB_KEY, pubb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PRIV_KEY, privb); - if (rc != 1) - goto err; + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_P, bp); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_Q, bq); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_G, bg); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PUB_KEY, bpub_key); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PRIV_KEY, bpriv_key); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } rc = evp_build_pkey("DSA", param_bld, &(key->key), EVP_PKEY_KEYPAIR); + +fail: OSSL_PARAM_BLD_free(param_bld); + bignum_safe_free(bp); + bignum_safe_free(bq); + bignum_safe_free(bg); + bignum_safe_free(bpub_key); + bignum_safe_free(bpriv_key); return rc; -err: - OSSL_PARAM_BLD_free(param_bld); - return -1; #endif /* OPENSSL_VERSION_NUMBER */ } @@ -1574,18 +1580,18 @@ int pki_pubkey_build_dss(ssh_key key, ssh_string g, ssh_string pubkey) { int rc; -#if OPENSSL_VERSION_NUMBER < 0x30000000L BIGNUM *bp = NULL, *bq = NULL, *bg = NULL, *bpub_key = NULL; +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OSSL_PARAM_BLD *param_bld = OSSL_PARAM_BLD_new(); + if (param_bld == NULL) { + return SSH_ERROR; + } #else - const BIGNUM *pb, *qb, *gb, *pubb; - OSSL_PARAM_BLD *param_bld; -#endif /* OPENSSL_VERSION_NUMBER */ - -#if OPENSSL_VERSION_NUMBER < 0x30000000L key->dsa = DSA_new(); if (key->dsa == NULL) { return SSH_ERROR; } +#endif /* OPENSSL_VERSION_NUMBER */ bp = ssh_make_string_bn(p); bq = ssh_make_string_bn(q); @@ -1593,9 +1599,11 @@ int pki_pubkey_build_dss(ssh_key key, bpub_key = ssh_make_string_bn(pubkey); if (bp == NULL || bq == NULL || bg == NULL || bpub_key == NULL) { + rc = SSH_ERROR; goto fail; } +#if OPENSSL_VERSION_NUMBER < 0x30000000L /* Memory management of bp, bq and bg is transferred to DSA object */ rc = DSA_set0_pqg(key->dsa, bp, bq, bg); if (rc == 0) { @@ -1613,35 +1621,37 @@ fail: DSA_free(key->dsa); return SSH_ERROR; #else - param_bld = OSSL_PARAM_BLD_new(); - if (param_bld == NULL) - goto err; - - pb = ssh_make_string_bn(p); - qb = ssh_make_string_bn(q); - gb = ssh_make_string_bn(g); - pubb = ssh_make_string_bn(pubkey); - - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_P, pb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_Q, qb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_G, gb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PUB_KEY, pubb); - if (rc != 1) - goto err; + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_P, bp); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_Q, bq); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_FFC_G, bg); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_PUB_KEY, bpub_key); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } rc = evp_build_pkey("DSA", param_bld, &(key->key), EVP_PKEY_PUBLIC_KEY); + +fail: OSSL_PARAM_BLD_free(param_bld); + bignum_safe_free(bp); + bignum_safe_free(bq); + bignum_safe_free(bg); + bignum_safe_free(bpub_key); return rc; -err: - OSSL_PARAM_BLD_free(param_bld); - return -1; #endif /* OPENSSL_VERSION_NUMBER */ } @@ -1654,18 +1664,18 @@ int pki_privkey_build_rsa(ssh_key key, ssh_string q) { int rc; -#if OPENSSL_VERSION_NUMBER < 0x30000000L BIGNUM *be, *bn, *bd/*, *biqmp*/, *bp, *bq; +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OSSL_PARAM_BLD *param_bld = OSSL_PARAM_BLD_new(); + if (param_bld == NULL) { + return SSH_ERROR; + } #else - const BIGNUM *nb, *eb, *db, *pb, *qb; - OSSL_PARAM_BLD *param_bld; -#endif /* OPENSSL_VERSION_NUMBER */ - -#if OPENSSL_VERSION_NUMBER < 0x30000000L key->rsa = RSA_new(); if (key->rsa == NULL) { return SSH_ERROR; } +#endif /* OPENSSL_VERSION_NUMBER */ bn = ssh_make_string_bn(n); be = ssh_make_string_bn(e); @@ -1675,9 +1685,11 @@ int pki_privkey_build_rsa(ssh_key key, bq = ssh_make_string_bn(q); if (be == NULL || bn == NULL || bd == NULL || /*biqmp == NULL ||*/ bp == NULL || bq == NULL) { + rc = SSH_ERROR; goto fail; } +#if OPENSSL_VERSION_NUMBER < 0x30000000L /* Memory management of be, bn and bd is transferred to RSA object */ rc = RSA_set0_key(key->rsa, bn, be, bd); if (rc == 0) { @@ -1702,41 +1714,49 @@ fail: RSA_free(key->rsa); return SSH_ERROR; #else - param_bld = OSSL_PARAM_BLD_new(); - if (param_bld == NULL) - goto err; - - nb = ssh_make_string_bn(n); - eb = ssh_make_string_bn(e); - db = ssh_make_string_bn(d); - pb = ssh_make_string_bn(p); - qb = ssh_make_string_bn(q); - - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_N, nb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_E, eb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_D, db); - if (rc != 1) - goto err; + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_N, bn); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_E, be); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_D, bd); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } rc = evp_build_pkey("RSA", param_bld, &(key->key), EVP_PKEY_KEYPAIR); - OSSL_PARAM_BLD_free(param_bld); + if (rc != SSH_OK) { + rc = SSH_ERROR; + goto fail; + } - rc = EVP_PKEY_set_bn_param(key->key, OSSL_PKEY_PARAM_RSA_FACTOR1, pb); - if (rc != 1) - goto err; + rc = EVP_PKEY_set_bn_param(key->key, OSSL_PKEY_PARAM_RSA_FACTOR1, bp); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } - rc = EVP_PKEY_set_bn_param(key->key, OSSL_PKEY_PARAM_RSA_FACTOR2, qb); - if (rc != 1) - goto err; + rc = EVP_PKEY_set_bn_param(key->key, OSSL_PKEY_PARAM_RSA_FACTOR2, bq); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } - return rc; -err: +fail: OSSL_PARAM_BLD_free(param_bld); - return -1; + bignum_safe_free(bn); + bignum_safe_free(be); + bignum_safe_free(bd); + bignum_safe_free(bp); + bignum_safe_free(bq); + + return rc; #endif /* OPENSSL_VERSION_NUMBER */ } @@ -1744,25 +1764,27 @@ int pki_pubkey_build_rsa(ssh_key key, ssh_string e, ssh_string n) { int rc; -#if OPENSSL_VERSION_NUMBER < 0x30000000L BIGNUM *be = NULL, *bn = NULL; +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OSSL_PARAM_BLD *param_bld = OSSL_PARAM_BLD_new(); + if (param_bld == NULL) { + return SSH_ERROR; + } #else - const BIGNUM *eb, *nb; - OSSL_PARAM_BLD *param_bld; -#endif /* OPENSSL_VERSION_NUMBER */ - -#if OPENSSL_VERSION_NUMBER < 0x30000000L key->rsa = RSA_new(); if (key->rsa == NULL) { return SSH_ERROR; } +#endif /* OPENSSL_VERSION_NUMBER */ be = ssh_make_string_bn(e); bn = ssh_make_string_bn(n); if (be == NULL || bn == NULL) { + rc = SSH_ERROR; goto fail; } +#if OPENSSL_VERSION_NUMBER < 0x30000000L /* Memory management of bn and be is transferred to RSA object */ rc = RSA_set0_key(key->rsa, bn, be, NULL); if (rc == 0) { @@ -1774,27 +1796,25 @@ fail: RSA_free(key->rsa); return SSH_ERROR; #else - nb = ssh_make_string_bn(n); - eb = ssh_make_string_bn(e); - - param_bld = OSSL_PARAM_BLD_new(); - if (param_bld == NULL) - goto err; - - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_N, nb); - if (rc != 1) - goto err; - rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_E, eb); - if (rc != 1) - goto err; + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_N, bn); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } + rc = OSSL_PARAM_BLD_push_BN(param_bld, OSSL_PKEY_PARAM_RSA_E, be); + if (rc != 1) { + rc = SSH_ERROR; + goto fail; + } rc = evp_build_pkey("RSA", param_bld, &(key->key), EVP_PKEY_PUBLIC_KEY); + +fail: OSSL_PARAM_BLD_free(param_bld); + bignum_safe_free(bn); + bignum_safe_free(be); return rc; -err: - OSSL_PARAM_BLD_free(param_bld); - return -1; #endif /* OPENSSL_VERSION_NUMBER */ } -- 2.38.1