Rework newpass_p12() a bit more

Split the bottom half that repacks the authsafes into a helper function.
This simplifies the curly exit path and makes it clearer what is being
done. PKCS12_pack_authsafes() is a very inconvenient  API and there are
some extra dances needed due to it.

ok jsing

diff --git a/lib/libcrypto/pkcs12/p12_npas.c b/lib/libcrypto/pkcs12/p12_npas.c
index 557457b..b0858b6 100644 (file)
--- a/lib/libcrypto/pkcs12/p12_npas.c
+++ b/lib/libcrypto/pkcs12/p12_npas.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: p12_npas.c,v 1.20 2024/01/25 09:40:09 tb Exp $ */
+/* $OpenBSD: p12_npas.c,v 1.21 2024/01/25 10:44:39 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -165,13 +165,54 @@ pkcs7_repack_encdata(PKCS7 *pkcs7, STACK_OF(PKCS7) *newsafes, const char *oldpas
 }
 
 static int
-newpass_p12(PKCS12 *p12, const char *oldpass, const char *newpass)
+pkcs12_repack_safe(PKCS12 *pkcs12, STACK_OF(PKCS7) *newsafes,
+    const char *newpass)
 {
-       STACK_OF(PKCS7) *asafes = NULL, *newsafes = NULL;
-       ASN1_OCTET_STRING *p12_data_tmp = NULL, *macnew = NULL;
+       ASN1_OCTET_STRING *old_data;
+       ASN1_OCTET_STRING *new_mac = NULL;
        unsigned char mac[EVP_MAX_MD_SIZE];
        unsigned int maclen;
+       int ret = 0;
+
+       if ((old_data = pkcs12->authsafes->d.data) == NULL)
+               goto err;
+       if ((pkcs12->authsafes->d.data = ASN1_OCTET_STRING_new()) == NULL)
+               goto err;
+       if (!PKCS12_pack_authsafes(pkcs12, newsafes))
+               goto err;
+       if (!PKCS12_gen_mac(pkcs12, newpass, -1, mac, &maclen))
+               goto err;
+       if ((new_mac = ASN1_OCTET_STRING_new()) == NULL)
+               goto err;
+       if (!ASN1_OCTET_STRING_set(new_mac, mac, maclen))
+               goto err;
+
+       ASN1_OCTET_STRING_free(pkcs12->mac->dinfo->digest);
+       pkcs12->mac->dinfo->digest = new_mac;
+       new_mac = NULL;
+
+       ASN1_OCTET_STRING_free(old_data);
+       old_data = NULL;
+
+       ret = 1;
+
+ err:
+       if (old_data != NULL) {
+               ASN1_OCTET_STRING_free(pkcs12->authsafes->d.data);
+               pkcs12->authsafes->d.data = old_data;
+       }
+       explicit_bzero(mac, sizeof(mac));
+       ASN1_OCTET_STRING_free(new_mac);
+
+       return ret;
+}
+
+static int
+newpass_p12(PKCS12 *p12, const char *oldpass, const char *newpass)
+{
+       STACK_OF(PKCS7) *asafes = NULL, *newsafes = NULL;
        int i;
+       int ret = 0;
 
        if ((asafes = PKCS12_unpack_authsafes(p12)) == NULL)
                goto err;
@@ -192,43 +233,17 @@ newpass_p12(PKCS12 *p12, const char *oldpass, const char *newpass)
                        break;
                }
        }
-       sk_PKCS7_pop_free(asafes, PKCS7_free);
-       asafes = NULL;
-
-       /* Repack safe: save old safe in case of error */
 
-       p12_data_tmp = p12->authsafes->d.data;
-       if (!(p12->authsafes->d.data = ASN1_OCTET_STRING_new())) {
-               p12->authsafes->d.data = p12_data_tmp;
+       if (!pkcs12_repack_safe(p12, newsafes, newpass))
                goto err;
-       }
-       if (!PKCS12_pack_authsafes(p12, newsafes))
-               goto saferr;
-
-       if (!PKCS12_gen_mac(p12, newpass, -1, mac, &maclen))
-               goto saferr;
-       if (!(macnew = ASN1_OCTET_STRING_new()))
-               goto saferr;
-       if (!ASN1_OCTET_STRING_set(macnew, mac, maclen))
-               goto saferr;
-       ASN1_OCTET_STRING_free(p12->mac->dinfo->digest);
-       p12->mac->dinfo->digest = macnew;
-       ASN1_OCTET_STRING_free(p12_data_tmp);
-
-       return 1;
 
-saferr:
-       /* Restore old safe */
-       ASN1_OCTET_STRING_free(p12->authsafes->d.data);
-       ASN1_OCTET_STRING_free(macnew);
-       p12->authsafes->d.data = p12_data_tmp;
-       return 0;
+       ret = 1;
 
-err:
+ err:
        sk_PKCS7_pop_free(asafes, PKCS7_free);
        sk_PKCS7_pop_free(newsafes, PKCS7_free);
 
-       return 0;
+       return ret;
 }