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

diff --git a/lib/libcrypto/dh/dh_ameth.c b/lib/libcrypto/dh/dh_ameth.c
index 4a600b3..cd4c130 100644 (file)
--- 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 28aafeb..badd2d2 100644 (file)
--- 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