From b42bcaece30a75e14811151018c0214d63b1c194 Mon Sep 17 00:00:00 2001 From: tb Date: Fri, 11 Aug 2023 11:32:19 +0000 Subject: [PATCH] Align dh and dsa decoding functions with encoding This adds some missing error checks and fixes and unifies error codes which were (as usual) all over the place or just plain nonsense. Use an auxiliary variable for d2i invocations even though it is not really needed here. ok jsing --- lib/libcrypto/dh/dh_ameth.c | 142 +++++++++++++++------------- lib/libcrypto/dsa/dsa_ameth.c | 173 ++++++++++++++++++---------------- 2 files changed, 173 insertions(+), 142 deletions(-) diff --git a/lib/libcrypto/dh/dh_ameth.c b/lib/libcrypto/dh/dh_ameth.c index 4a600b3bbdd..cd4c130f108 100644 --- a/lib/libcrypto/dh/dh_ameth.c +++ b/lib/libcrypto/dh/dh_ameth.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dh_ameth.c,v 1.33 2023/08/10 16:57:15 tb Exp $ */ +/* $OpenBSD: dh_ameth.c,v 1.34 2023/08/11 11:32:19 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2006. */ @@ -78,53 +78,55 @@ int_dh_free(EVP_PKEY *pkey) static int dh_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) { - const unsigned char *p, *pm; - int pklen, pmlen; + X509_ALGOR *algor; int ptype; const void *pval; - const ASN1_STRING *pstr; - X509_ALGOR *palg; - ASN1_INTEGER *public_key = NULL; + const ASN1_STRING *params; + const unsigned char *key_der, *params_der, *p; + int key_len, params_len; + ASN1_INTEGER *key = NULL; DH *dh = NULL; + int ret = 0; - if (!X509_PUBKEY_get0_param(NULL, &p, &pklen, &palg, pubkey)) - return 0; - X509_ALGOR_get0(NULL, &ptype, &pval, palg); + if (!X509_PUBKEY_get0_param(NULL, &key_der, &key_len, &algor, pubkey)) + goto err; + X509_ALGOR_get0(NULL, &ptype, &pval, algor); if (ptype != V_ASN1_SEQUENCE) { DHerror(DH_R_PARAMETER_ENCODING_ERROR); goto err; } - pstr = pval; - pm = pstr->data; - pmlen = pstr->length; + params = pval; + params_der = params->data; + params_len = params->length; - if (!(dh = d2i_DHparams(NULL, &pm, pmlen))) { + p = params_der; + if ((dh = d2i_DHparams(NULL, &p, params_len)) == NULL) { DHerror(DH_R_DECODE_ERROR); goto err; } - - if (!(public_key=d2i_ASN1_INTEGER(NULL, &p, pklen))) { + p = key_der; + if ((key = d2i_ASN1_INTEGER(NULL, &p, key_len)) == NULL) { DHerror(DH_R_DECODE_ERROR); goto err; } - - /* We have parameters now set public key */ - if (!(dh->pub_key = ASN1_INTEGER_to_BN(public_key, NULL))) { + if ((dh->pub_key = ASN1_INTEGER_to_BN(key, NULL)) == NULL) { DHerror(DH_R_BN_DECODE_ERROR); goto err; } - ASN1_INTEGER_free(public_key); - EVP_PKEY_assign_DH(pkey, dh); - return 1; + if (!EVP_PKEY_assign_DH(pkey, dh)) + goto err; + dh = NULL; + + ret = 1; -err: - if (public_key) - ASN1_INTEGER_free(public_key); + err: + ASN1_INTEGER_free(key); DH_free(dh); - return 0; + + return ret; } static int @@ -188,52 +190,57 @@ dh_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey) static int dh_priv_decode(EVP_PKEY *pkey, const PKCS8_PRIV_KEY_INFO *p8) { - const unsigned char *p, *pm; - int pklen, pmlen; + const X509_ALGOR *algor; int ptype; const void *pval; - const ASN1_STRING *pstr; - const X509_ALGOR *palg; - ASN1_INTEGER *privkey = NULL; + const ASN1_STRING *params; + const unsigned char *key_der, *params_der, *p; + int key_len, params_len; + ASN1_INTEGER *key = NULL; DH *dh = NULL; + int ret = 0; - if (!PKCS8_pkey_get0(NULL, &p, &pklen, &palg, p8)) - return 0; - - X509_ALGOR_get0(NULL, &ptype, &pval, palg); + if (!PKCS8_pkey_get0(NULL, &key_der, &key_len, &algor, p8)) + goto err; + X509_ALGOR_get0(NULL, &ptype, &pval, algor); - if (ptype != V_ASN1_SEQUENCE) - goto decerr; + if (ptype != V_ASN1_SEQUENCE) { + DHerror(DH_R_PARAMETER_ENCODING_ERROR); + goto err; + } - if (!(privkey=d2i_ASN1_INTEGER(NULL, &p, pklen))) - goto decerr; + params = pval; + params_der = params->data; + params_len = params->length; - pstr = pval; - pm = pstr->data; - pmlen = pstr->length; - if (!(dh = d2i_DHparams(NULL, &pm, pmlen))) - goto decerr; - /* We have parameters now set private key */ - if (!(dh->priv_key = ASN1_INTEGER_to_BN(privkey, NULL))) { - DHerror(DH_R_BN_ERROR); - goto dherr; + p = params_der; + if ((dh = d2i_DHparams(NULL, &p, params_len)) == NULL) { + DHerror(DH_R_DECODE_ERROR); + goto err; + } + p = key_der; + if ((key = d2i_ASN1_INTEGER(NULL, &p, key_len)) == NULL) { + DHerror(DH_R_DECODE_ERROR); + goto err; + } + if ((dh->priv_key = ASN1_INTEGER_to_BN(key, NULL)) == NULL) { + DHerror(DH_R_BN_DECODE_ERROR); + goto err; } - /* Calculate public key */ if (!DH_generate_key(dh)) - goto dherr; - - EVP_PKEY_assign_DH(pkey, dh); + goto err; - ASN1_INTEGER_free(privkey); + if (!EVP_PKEY_assign_DH(pkey, dh)) + goto err; + dh = NULL; - return 1; + ret = 1; -decerr: - DHerror(EVP_R_DECODE_ERROR); -dherr: - ASN1_INTEGER_free(privkey); + err: + ASN1_INTEGER_free(key); DH_free(dh); - return 0; + + return ret; } static int @@ -293,14 +300,23 @@ dh_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey) static int dh_param_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen) { - DH *dh; + DH *dh = NULL; + int ret = 0; - if (!(dh = d2i_DHparams(NULL, pder, derlen))) { + if ((dh = d2i_DHparams(NULL, pder, derlen)) == NULL) { DHerror(ERR_R_DH_LIB); - return 0; + goto err; } - EVP_PKEY_assign_DH(pkey, dh); - return 1; + if (!EVP_PKEY_assign_DH(pkey, dh)) + goto err; + dh = NULL; + + ret = 1; + + err: + DH_free(dh); + + return ret; } static int diff --git a/lib/libcrypto/dsa/dsa_ameth.c b/lib/libcrypto/dsa/dsa_ameth.c index 28aafebc04f..badd2d25b51 100644 --- a/lib/libcrypto/dsa/dsa_ameth.c +++ b/lib/libcrypto/dsa/dsa_ameth.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dsa_ameth.c,v 1.46 2023/08/10 16:57:15 tb Exp $ */ +/* $OpenBSD: dsa_ameth.c,v 1.47 2023/08/11 11:32:19 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2006. */ @@ -75,31 +75,32 @@ static int dsa_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) { - const unsigned char *p, *pm; - int pklen, pmlen; + X509_ALGOR *algor; int ptype; const void *pval; - const ASN1_STRING *pstr; - X509_ALGOR *palg; - ASN1_INTEGER *public_key = NULL; - + const ASN1_STRING *params; + const unsigned char *key_der, *params_der, *p; + int key_len, params_len; + ASN1_INTEGER *key = NULL; DSA *dsa = NULL; + int ret = 0; - if (!X509_PUBKEY_get0_param(NULL, &p, &pklen, &palg, pubkey)) - return 0; - X509_ALGOR_get0(NULL, &ptype, &pval, palg); + if (!X509_PUBKEY_get0_param(NULL, &key_der, &key_len, &algor, pubkey)) + goto err; + X509_ALGOR_get0(NULL, &ptype, &pval, algor); if (ptype == V_ASN1_SEQUENCE) { - pstr = pval; - pm = pstr->data; - pmlen = pstr->length; + params = pval; + params_der = params->data; + params_len = params->length; - if (!(dsa = d2i_DSAparams(NULL, &pm, pmlen))) { + p = params_der; + if ((dsa = d2i_DSAparams(NULL, &p, params_len)) == NULL) { DSAerror(DSA_R_DECODE_ERROR); goto err; } } else if (ptype == V_ASN1_NULL || ptype == V_ASN1_UNDEF) { - if (!(dsa = DSA_new())) { + if ((dsa = DSA_new()) == NULL) { DSAerror(ERR_R_MALLOC_FAILURE); goto err; } @@ -108,31 +109,32 @@ dsa_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) goto err; } - if (!(public_key = d2i_ASN1_INTEGER(NULL, &p, pklen))) { + p = key_der; + if ((key = d2i_ASN1_INTEGER(NULL, &p, key_len)) == NULL) { DSAerror(DSA_R_DECODE_ERROR); goto err; } - - if (!(dsa->pub_key = ASN1_INTEGER_to_BN(public_key, NULL))) { + if ((dsa->pub_key = ASN1_INTEGER_to_BN(key, NULL)) == NULL) { DSAerror(DSA_R_BN_DECODE_ERROR); goto err; } - /* We can only check for key consistency if we have parameters. */ if (ptype == V_ASN1_SEQUENCE) { if (!dsa_check_key(dsa)) goto err; } - ASN1_INTEGER_free(public_key); - EVP_PKEY_assign_DSA(pkey, dsa); - return 1; + if (!EVP_PKEY_assign_DSA(pkey, dsa)) + goto err; + dsa = NULL; -err: - if (public_key) - ASN1_INTEGER_free(public_key); + ret = 1; + + err: + ASN1_INTEGER_free(key); DSA_free(dsa); - return 0; + + return ret; } static int @@ -192,81 +194,85 @@ dsa_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey) return ret; } -/* In PKCS#8 DSA: you just get a private key integer and parameters in the +/* + * In PKCS#8 DSA: you just get a private key integer and parameters in the * AlgorithmIdentifier the pubkey must be recalculated. */ static int dsa_priv_decode(EVP_PKEY *pkey, const PKCS8_PRIV_KEY_INFO *p8) { - const unsigned char *p, *pm; - int pklen, pmlen; + const X509_ALGOR *algor; int ptype; const void *pval; - const ASN1_STRING *pstr; - const X509_ALGOR *palg; - ASN1_INTEGER *privkey = NULL; + const ASN1_STRING *params; + const unsigned char *key_der, *params_der, *p; + int key_len, params_len; + ASN1_INTEGER *key = NULL; BN_CTX *ctx = NULL; DSA *dsa = NULL; int ret = 0; - if (!PKCS8_pkey_get0(NULL, &p, &pklen, &palg, p8)) - return 0; - X509_ALGOR_get0(NULL, &ptype, &pval, palg); - if (ptype != V_ASN1_SEQUENCE) - goto decerr; - - if ((privkey = d2i_ASN1_INTEGER(NULL, &p, pklen)) == NULL) - goto decerr; - if (privkey->type == V_ASN1_NEG_INTEGER) - goto decerr; - - pstr = pval; - pm = pstr->data; - pmlen = pstr->length; - if (!(dsa = d2i_DSAparams(NULL, &pm, pmlen))) - goto decerr; - /* We have parameters now set private key */ - if (!(dsa->priv_key = ASN1_INTEGER_to_BN(privkey, NULL))) { - DSAerror(DSA_R_BN_ERROR); - goto dsaerr; + if (!PKCS8_pkey_get0(NULL, &key_der, &key_len, &algor, p8)) + goto err; + X509_ALGOR_get0(NULL, &ptype, &pval, algor); + + if (ptype != V_ASN1_SEQUENCE) { + DSAerror(DSA_R_PARAMETER_ENCODING_ERROR); + goto err; + } + + params = pval; + params_der = params->data; + params_len = params->length; + + p = params_der; + if ((dsa = d2i_DSAparams(NULL, &p, params_len)) == NULL) { + DSAerror(DSA_R_DECODE_ERROR); + goto err; + } + p = key_der; + if ((key = d2i_ASN1_INTEGER(NULL, &p, key_len)) == NULL) { + DSAerror(DSA_R_DECODE_ERROR); + goto err; + } + if ((dsa->priv_key = ASN1_INTEGER_to_BN(key, NULL)) == NULL) { + DSAerror(DSA_R_BN_DECODE_ERROR); + goto err; } /* Check the key for basic consistency before doing expensive things. */ if (!dsa_check_key(dsa)) - goto dsaerr; + goto err; /* Calculate public key */ - if (!(dsa->pub_key = BN_new())) { + if ((dsa->pub_key = BN_new()) == NULL) { DSAerror(ERR_R_MALLOC_FAILURE); - goto dsaerr; + goto err; } if ((ctx = BN_CTX_new()) == NULL) { DSAerror(ERR_R_MALLOC_FAILURE); - goto dsaerr; + goto err; } BN_CTX_start(ctx); if (!BN_mod_exp_ct(dsa->pub_key, dsa->g, dsa->priv_key, dsa->p, ctx)) { DSAerror(DSA_R_BN_ERROR); - goto dsaerr; + goto err; } if (!EVP_PKEY_assign_DSA(pkey, dsa)) - goto decerr; + goto err; + dsa = NULL; ret = 1; - goto done; -decerr: - DSAerror(DSA_R_DECODE_ERROR); -dsaerr: + err: DSA_free(dsa); -done: BN_CTX_end(ctx); BN_CTX_free(ctx); - ASN1_INTEGER_free(privkey); + ASN1_INTEGER_free(key); return ret; } @@ -454,18 +460,25 @@ do_dsa_print(BIO *bp, const DSA *x, int off, int ptype) static int dsa_param_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen) { - DSA *dsa; + DSA *dsa = NULL; + int ret = 0; - if (!(dsa = d2i_DSAparams(NULL, pder, derlen))) { + if ((dsa = d2i_DSAparams(NULL, pder, derlen)) == NULL) { DSAerror(ERR_R_DSA_LIB); - return 0; - } - if (!dsa_check_key(dsa)) { - DSA_free(dsa); - return 0; + goto err; } - EVP_PKEY_assign_DSA(pkey, dsa); - return 1; + if (!dsa_check_key(dsa)) + goto err; + if (!EVP_PKEY_assign_DSA(pkey, dsa)) + goto err; + dsa = NULL; + + ret = 1; + + err: + DSA_free(dsa); + + return ret; } static int @@ -495,9 +508,10 @@ dsa_priv_print(BIO *bp, const EVP_PKEY *pkey, int indent, ASN1_PCTX *ctx) static int old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen) { - DSA *dsa; + DSA *dsa = NULL; BN_CTX *ctx = NULL; BIGNUM *result; + int ret = 0; if ((dsa = d2i_DSAPrivateKey(NULL, pder, derlen)) == NULL) { DSAerror(ERR_R_DSA_LIB); @@ -551,17 +565,18 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen) goto err; } - BN_CTX_end(ctx); - BN_CTX_free(ctx); + if (!EVP_PKEY_assign_DSA(pkey, dsa)) + goto err; + dsa = NULL; - EVP_PKEY_assign_DSA(pkey, dsa); - return 1; + ret = 1; err: BN_CTX_end(ctx); BN_CTX_free(ctx); DSA_free(dsa); - return 0; + + return ret; } static int -- 2.20.1