hkdf: allow salt to be reset to null #27305

paulidale · 2025-04-09T02:48:48Z

The fix in #24141 wasn't quite correct. The test case just happened to work on a freshly initialised context.

documentation is added or updated
tests are added or updated

slontis · 2025-04-09T05:49:44Z

test/evp_extra_test.c

    if (!TEST_int_gt(EVP_PKEY_derive_init(pctx), 0)
            || !TEST_int_gt(EVP_PKEY_CTX_set_hkdf_md(pctx, EVP_sha256()), 0)
+            || !TEST_int_gt(EVP_PKEY_CTX_set1_hkdf_salt(pctx, fake,
+                                                        sizeof(fake) - 1), 0)


not really overjoyed with how this does sizeof(salt) - 1 and sizeof(info)-1..
These are just 0 values.

Where is the test for setting salt to NULL?

The line below sets a zero length salt (this was done by the existing test). This is needed because the default is a zero length salt and even though the set after this fails, it doesn't return an error so this test worked by fluke rather than design. Adding this set breaks the default and the next line has to work for the test to pass.

t8m · 2025-04-09T11:46:26Z

providers/implementations/kdfs/hkdf.c

+        ctx->salt = NULL;
+        if (!OSSL_PARAM_get_octet_string(p, (void **)&ctx->salt, 0,
+                                         &ctx->salt_len))


Would this be more correct allowing for using NULL, 0 for the param?

Suggested change

ctx->salt = NULL;

if (!OSSL_PARAM_get_octet_string(p, (void **)&ctx->salt, 0,

&ctx->salt_len))

ctx->salt = NULL;

ctx->salt_len = 0;

if (p->data_size != 0 && p->data != NULL

&& !OSSL_PARAM_get_octet_string(p, (void **)&ctx->salt, 0,

&ctx->salt_len))

The length and NULL check were introduced by the earlier fix and they are broken in a subtle way. Subtle enough that both you and I missed the problem when reviewing the change 😢

I thought about leaving these checks in but the big issue for me is that we don't do it this way anywhere else. Consistency in our APIs is something we should be striving for.

We do not generally:

check for ->data_size != 0

check for ->data == NULL

If the param exists we just assume it has a valid pointer associated with it and that the length is trustworthy. It seemed reasonable to do the same here.

I think @t8m is correct. Such a check is necessary because OSSL_PARAM_get_octet_string() fails if the param's data is NULL. This currently actually prevents setting the salt to NULL.

I think @t8m is correct. Such a check is necessary because OSSL_PARAM_get_octet_string() fails if the param's data is NULL. This currently actually prevents setting the salt to NULL.

Not sure if this was an intended change, but we found this statement to be true. We can no longer set the salt to NULL in openssl 3.5.1. We get the following error:

80476A18AA750000:error:078C0102:common libcrypto routines:get_string_internal:passed a null parameter

You could never set the salt to NULL before 3.5.1. If you tried the salt remained unchanged and the call apparently suceeded. So if you ever set the salt to a non–empty value, you could not set it back to empty^†. What changed in 3.5.1 is that trying to set the salt to NULL raises an error. I think this is better than silently doing nothing and it's consistent with other parameter usage.

I see a few ways forward:

if, as is likely, you never set the salt explicitly, there is no need to reset it;

if you do set the salt explicitly, you're in a world of hurt because the previous salt will have been used without you knowing or

set the salt to empty by passing "" and a length of zero instead of a NULL pointer.

In 3.4.0, line 310 means your attempt to set the parameter is silently ignored:

openssl/providers/implementations/kdfs/hkdf.c

Lines 309 to 317 in 636dfad

if ((p = OSSL_PARAM_locate_const(params, OSSL_KDF_PARAM_SALT)) != NULL) {

if (p->data_size != 0 && p->data != NULL) {

OPENSSL_free(ctx->salt);

ctx->salt = NULL;

if (!OSSL_PARAM_get_octet_string(p, (void **)&ctx->salt, 0,

&ctx->salt_len))

return 0;

}

}

In 3.5.1 this changed to:

openssl/providers/implementations/kdfs/hkdf.c

Lines 309 to 315 in f4fb4a6

if ((p = OSSL_PARAM_locate_const(params, OSSL_KDF_PARAM_SALT)) != NULL) {

OPENSSL_free(ctx->salt);

ctx->salt = NULL;

if (!OSSL_PARAM_get_octet_string(p, (void **)&ctx->salt, 0,

&ctx->salt_len))

return 0;

}

The call on line 313 raises an error if the parameter's data pointer is NULL.

^†: the reset call sets the salt to empty but also wipes everything else.

If you are having to set it and then reset it to NULL then something is also wrong. The RFC for HKDF says its optional, but it also says you should normally set it if possible because its more secure. The case where you dont set it should be if you just cant due to platform related restrictions. So you should probably always be setting it (if you can), or not setting it at all. In either of these cases setting it to NULL is not required.

paulidale · 2025-04-15T04:42:24Z

Ping for reviews?

openssl-machine · 2025-04-18T06:00:20Z

This pull request is ready to merge

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305)

Fixes #27302 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305)

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 12eb6c5)

Fixes #27302 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 7271179)

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 12eb6c5)

Fixes #27302 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 7271179)

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 12eb6c5)

Fixes #27302 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 7271179)

mattcaswell · 2025-04-18T14:36:49Z

Pushed to master, 3.5, 3.4, 3.3, 3.2 and 3.0.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 12eb6c5)

Fixes #27302 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #27305) (cherry picked from commit 7271179)

Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#27305)