Remove last internal use of ASN1_STRING_data()

PKCS5_pbe_set0_algor() is no longer public, but its parameters are
provided directly via public API, namely the wonderful PKCS8_encrypt()
and PKCS12_pack_p7encdata().

Muppetry abounds. To wit:

If saltlen < 0, the call to ASN1_STRING_set(pbe->salt, NULL, saltlen)
would error. Let's do that up front in a more obvious way. We don't
care about side-effects to pbe->salt since we free it on error anyway.

If saltlen == 0, we default it to PKCS5_PBE1_SALT_LEN. This is
particularly funky in case the caller passed in salt != NULL, in
which case we can only hope and pray this buffer is long enough.

If the caller passed a salt, copy it to pbe->salt via ASN1_STRING_set().

If there's no salt, allocate a buffer of the appropriate length, fill it
with random and transfer ownership to pbe->salt via ASN1_STRING_set0().
There's a change of behavior in that this will not be NUL-terminated
(why should it be?). If we wanted to preserve behavior, we'd just use
calloc(1, saltlen + 1) instead of the malloc().

The exit path is quite special, too, but I didn't want to change this
right now.

tweaks/ok kenjiro

1 files changed, 19 insertions, 13 deletions

diff --git a/src/lib/libcrypto/asn1/p5_pbe.c b/src/lib/libcrypto/asn1/p5_pbe.c
index 668bf5d7c1..feccf8af58 100644
--- a/src/lib/libcrypto/asn1/p5_pbe.c
+++ b/src/lib/libcrypto/asn1/p5_pbe.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: p5_pbe.c,v 1.30 2025/05/24 02:57:14 tb Exp $ */
+/* $OpenBSD: p5_pbe.c,v 1.31 2025/12/07 09:27:02 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -129,7 +129,6 @@ PKCS5_pbe_set0_algor(X509_ALGOR *algor, int alg, int iter,
 {
         PBEPARAM *pbe = NULL;
         ASN1_STRING *pbe_str = NULL;
-        unsigned char *sstr;
 
         if ((pbe = PBEPARAM_new()) == NULL) {
                 ASN1error(ERR_R_MALLOC_FAILURE);
@@ -141,17 +140,24 @@ PKCS5_pbe_set0_algor(X509_ALGOR *algor, int alg, int iter,
                 ASN1error(ERR_R_MALLOC_FAILURE);
                 goto err;
         }
-        if (!saltlen)
-                saltlen = PKCS5_PBE1_SALT_LEN;
-        if (!ASN1_STRING_set(pbe->salt, NULL, saltlen)) {
-                ASN1error(ERR_R_MALLOC_FAILURE);
+        if (saltlen < 0)
                 goto err;
-        }
-        sstr = ASN1_STRING_data(pbe->salt);
-        if (salt)
-                memcpy(sstr, salt, saltlen);
-        else
+        if (saltlen == 0)
+                saltlen = PKCS5_PBE1_SALT_LEN;
+        if (salt != NULL) {
+                if (!ASN1_STRING_set(pbe->salt, salt, saltlen))
+                        goto err;
+        } else {
+                unsigned char *sstr = NULL;
+
+                if ((sstr = malloc(saltlen)) == NULL) {
+                        ASN1error(ERR_R_MALLOC_FAILURE);
+                        goto err;
+                }
                 arc4random_buf(sstr, saltlen);
+                ASN1_STRING_set0(pbe->salt, sstr, saltlen);
+                sstr = NULL;
+        }
 
         if (!ASN1_item_pack(pbe, &PBEPARAM_it, &pbe_str)) {
                 ASN1error(ERR_R_MALLOC_FAILURE);
@@ -165,9 +171,9 @@ PKCS5_pbe_set0_algor(X509_ALGOR *algor, int alg, int iter,
                 return 1;
 
  err:
-        if (pbe != NULL)
-                PBEPARAM_free(pbe);
+        PBEPARAM_free(pbe);
         ASN1_STRING_free(pbe_str);
+
         return 0;
 }