Fix API to allow zero-length secret for EVP_KDF #12826

jon-oracle · 2020-09-08T22:44:24Z

The behaviour of EVP_KDF_CTX_set_params changed between alpha4 and current, so that one of our tests started to fail. The test results in this API call:

params[0] = OSSL_PARAM_construct_utf8_string(OSSL_KDF_PARAM_DIGEST, (char *) mdname, 0);
params[1] = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_SECRET, secret, secretlen);
params[2] = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_SEED, seed, seedlen);
params[3] = OSSL_PARAM_construct_end();

ret = EVP_KDF_CTX_set_params(ctx, params);

Where:
secret = pointer to blank buffer
secretlen = 0

The KDF is OSSL_KDF_NAME_TLS1_PRF.

This started to fail recently with a change to the HMAC code (which is used internally in the KDF):

3fddbb2

The code added to allocate a copy of the HMAC key is not zero-length aware so the OPENSSL_secure_malloc fails. The KDF code to store the secret is already zero-length aware.

This PR also goes some way to fixing #8531 although the secret buffer is still checked for NULL so the user would have to provide a non-null buffer even with a length of zero.

kaduk · 2020-09-08T22:50:17Z

It's like #11920 all over again...

paulidale · 2020-09-08T23:15:15Z

I think that there might be a better fix for this.

There is an issue where OPENSSL_secure_malloc(0) returns a pointer to memory if it's been set up and NULL if not. OPENSSL_malloc(0) always returns NULL. POSIX specifies that malloc(0) can either return NULL or a unique pointer that can be later freed.

I suspect changing OPENSSL_malloc(0) to return a pointer to a single byte is a better fix because it addresses the underlying difference between out APIs.

paulidale · 2020-09-08T23:26:48Z

Scratch that, fixing malloc causes test failures...

in kdf code

slontis · 2020-09-09T05:27:39Z

Is this behavior what we really want - or should it have caused an error?

jon-oracle · 2020-09-09T05:52:17Z

Is this behavior what we really want - or should it have caused an error?

I'm not going to make a call either way. If it should have caused an error then this should have been generated when the API was called, not left to a malloc failure later. Also if you prefer to fail on a zero-length secret then maybe you should also fail on any insecure secret length? I doubt a one-byte secret is providing much authentication.

slontis · 2020-09-09T05:57:29Z

I'm not going to make a call either way. If it should have caused

Are you just doing negative testing or is this a use case you actually need?

jon-oracle · 2020-09-09T06:01:21Z

I'm not going to make a call either way. If it should have caused

Are you just doing negative testing or is this a use case you actually need?

This is an edge-case test. I can't see a specific use case.

kaduk · 2020-09-09T07:01:31Z

I think we do see empty-keyed HMAC pop up in some protocol edge cases, though most of the examples that are coming to mind are using a block of zeros rather than a true empty key. The original report to me that prompted #11920 was definitely for HMAC; something QUIC-related IIRC.

paulidale · 2020-09-09T08:35:11Z

Would a zero length secret be permitted by NIST for a module integrity check?

I'm sure there are plenty of examples of this in the wild 😞

openssl-machine · 2020-09-10T00:00:13Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

slontis

If we are going to allow this then there at least needs to be a test for it.

jon-oracle · 2020-09-14T07:06:20Z

Some tests added. It seems that the initial bug reported in #12409 is no longer a problem.

test/evp_kdf_test.c

slontis

Thanks jon

test/evp_kdf_test.c

paulidale

Great 😁

kaduk · 2020-09-16T20:37:01Z

providers/implementations/kdfs/kbkdf.c

@@ -205,6 +205,12 @@ static int kbkdf_derive(void *vctx, unsigned char *key, size_t keylen)
        return 0;
    }

+    /* Fail if the output length is zero */


Interesting that KBKDF gets this comment but not HKDF or tls1_prf.

kaduk · 2020-09-16T20:40:05Z

test/evp_kdf_test.c

 static int test_kdf_tls1_prf(void)
 {
    int ret;
    EVP_KDF_CTX *kctx = NULL;
    unsigned char out[16];
-    OSSL_PARAM params[4], *p = params;
+    OSSL_PARAM *params;


Basically every other test in this file uses stack-allocated OSSL_PARAM arrays; I didn't see discussion of why this one should allocate on the heap. (It's not wrong per se, just surprising.)
You can keep the helper function and just pass in a pointer to the start of the (stack-allocated) array, if you want.

I'm ambivalent either way. A comment might be beneficial to explain the change of style. However, there are plenty of cases where two different styles are used (param construct vs param_bld e.g.). One minor point in favour of the status quo is that construct_tls1_prf_params is returning a filled out param array, so doing it dynamically like this provides some future proofing against needing more params at some point (it's pretty weak).

kaduk · 2020-09-16T20:47:12Z

test/evp_kdf_test.c

+    EVP_KDF_CTX_free(kctx);
+    OPENSSL_free(params);
+    return ret;
+}


It's vaguely plausible that some of these tls-prf tests could be refactored consolidated into an ADD_ALL_TESTS (vs ADD_TEST) invocation, but I didn't do the work to check and won't insist that anyone else do the work.

kaduk · 2020-09-16T20:50:14Z

test/evp_kdf_test.c

+    *p++ = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_SALT,
+                                             salt, strlen(salt));
+    *p++ = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_KEY,
+                                             (unsigned char*)key, keylen);


If we have to make any other changes I'd suggest reordering these to match the order of the function parameters. It's purely cosmetic, though, so not critical.

kaduk · 2020-09-16T20:53:21Z

test/evp_kdf_test.c

+    EVP_KDF_CTX_free(kctx);
+    OPENSSL_free(params);
+    return ret;
+}


[same coment about consolidating/refactoring]

kaduk · 2020-09-16T20:57:46Z

test/evp_kdf_test.c

        || !TEST_true(EVP_KDF_CTX_set_params(kctx, params))
        /* A key length that is too small should fail */
-        || !TEST_int_eq(EVP_KDF_derive(kctx, out, 112 / 8 - 1), 0)
+        || !TEST_int_eq(EVP_KDF_derive(kctx, out, 112 / 8 - 1), 0))


I assume there's not an appropriate #define for 112 here.

kaduk · 2020-09-16T20:58:54Z

test/evp_kdf_test.c

+    OSSL_PARAM *params;
+
+    if (sizeof(len) > 32)
+        len = SIZE_MAX;


This seems incorrect/bizzare (but is unchanged from the old code). len is a size_t, which is not really of variable size (and 256-bit integers are not really a thing, let alone bigger ones!).

kaduk · 2020-09-16T21:07:12Z

test/evp_kdf_test.c

+
+    /* If the "pkcs5" mode is disabled then the derive will now fail */
+    if (!TEST_true(EVP_KDF_CTX_set_params(kctx, mode_params))
+        || !TEST_int_eq(EVP_KDF_derive(kctx, out, sizeof(out)), 0))


This actually raises a potentially interesting question: should EVP_KDF_derive() be a one-shot operation on a given EVP_KDF_CTX?

kaduk · 2020-09-16T21:20:58Z

(That's a +0 from me; I read the whole thing and there are no critical flaws, and I'm okay with it proceeding as-is with the other reviews if that's desired.)

paulidale · 2020-09-17T02:03:08Z

@kaduk, does this mean another review is required or does your earlier stand as enough of a positive for this to go in?

kaduk · 2020-09-17T02:54:17Z

@paulidale oops, I seem to have misread the preexisting approval count. Mine would have been enough for it to go in, I think, but Tim has short-circuited the issue in the time it took me to check my email.

paulidale · 2020-09-17T03:04:43Z

@kaduk, it was your final +0 comment that threw me. I didn't want to assume that your approval stood as a +1.
All good now.

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #12826)

Also add more test cases Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #12826)

paulidale · 2020-09-17T08:31:59Z

Merged to master after removing some end of line spaces. Thanks.

Allow zero-length secret for EVP_KDF API

45255cc

kaduk approved these changes Sep 8, 2020

View reviewed changes

kaduk added approval: otc review pending branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Sep 8, 2020

kaduk added this to the 3.0.0 milestone Sep 8, 2020

paulidale approved these changes Sep 8, 2020

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals hold: need otc decision The OTC needs to make a decision and removed approval: otc review pending labels Sep 8, 2020

paulidale removed the hold: need otc decision The OTC needs to make a decision label Sep 8, 2020

slontis requested changes Sep 10, 2020

View reviewed changes

Make KDFs fail if requesting a zero-length key. Also add more test cases

6c94427

slontis reviewed Sep 14, 2020

View reviewed changes

test/evp_kdf_test.c Outdated Show resolved Hide resolved

slontis reviewed Sep 14, 2020

View reviewed changes

test/evp_kdf_test.c Outdated Show resolved Hide resolved

Review updates

84ce432

t8m requested a review from kaduk September 15, 2020 17:04

paulidale added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Sep 15, 2020

Remove strlen calls when constructing utf8 string params

3d3c9de

paulidale approved these changes Sep 16, 2020

View reviewed changes

kroeckx modified the milestones: 3.0.0, 3.0.0 beta1 Sep 16, 2020

kaduk reviewed Sep 16, 2020

View reviewed changes

t-j-h approved these changes Sep 17, 2020

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 17, 2020

openssl-machine pushed a commit that referenced this pull request Sep 17, 2020

Allow zero-length secret for EVP_KDF API

0010870

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #12826)

paulidale closed this Sep 17, 2020

paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 17, 2020

kaduk mentioned this pull request Sep 30, 2020

KBKDF (SP 800 108 KDF) wrong result for non-empty salt in feedback mode #12409

Closed

kaduk mentioned this pull request Oct 7, 2020

master branch: EVP_PKEY_new_mac_key fails with 0-length key #13089

Open

jon-oracle deleted the evp-kdf-zlen-secret branch February 12, 2021 01:47

panva mentioned this pull request Jul 23, 2022

crypto: pass all WebCryptoAPI WPTs nodejs/node#43656

Closed

This was referenced Jan 10, 2023

Add basic support for caBLE authenticators / initiators kanidm/webauthn-rs#232

Merged

Fail builds when running on old/broken versions of OpenSSL kanidm/webauthn-rs#256

Merged

micolous mentioned this pull request Jun 14, 2023

Update openssl-src to 300? sfackler/rust-openssl#1645

Closed

bleichenbacher-daniel mentioned this pull request Apr 10, 2024

Can HKDF accept empty IKM? C2SP/wycheproof#114

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix API to allow zero-length secret for EVP_KDF #12826

Fix API to allow zero-length secret for EVP_KDF #12826

jon-oracle commented Sep 8, 2020

kaduk commented Sep 8, 2020

paulidale commented Sep 8, 2020

paulidale commented Sep 8, 2020 •

edited

Loading

slontis commented Sep 9, 2020

jon-oracle commented Sep 9, 2020

slontis commented Sep 9, 2020

jon-oracle commented Sep 9, 2020

kaduk commented Sep 9, 2020

paulidale commented Sep 9, 2020

openssl-machine commented Sep 10, 2020

slontis left a comment

jon-oracle commented Sep 14, 2020

slontis left a comment

paulidale left a comment

kaduk Sep 16, 2020

kaduk Sep 16, 2020

paulidale Sep 16, 2020

kaduk Sep 16, 2020

kaduk Sep 16, 2020

kaduk Sep 16, 2020

kaduk Sep 16, 2020

kaduk Sep 16, 2020

kaduk Sep 16, 2020

kaduk commented Sep 16, 2020

paulidale commented Sep 17, 2020

kaduk commented Sep 17, 2020

paulidale commented Sep 17, 2020

paulidale commented Sep 17, 2020

Fix API to allow zero-length secret for EVP_KDF #12826

Fix API to allow zero-length secret for EVP_KDF #12826

Conversation

jon-oracle commented Sep 8, 2020

kaduk commented Sep 8, 2020

paulidale commented Sep 8, 2020

paulidale commented Sep 8, 2020 • edited Loading

slontis commented Sep 9, 2020

jon-oracle commented Sep 9, 2020

slontis commented Sep 9, 2020

jon-oracle commented Sep 9, 2020

kaduk commented Sep 9, 2020

paulidale commented Sep 9, 2020

openssl-machine commented Sep 10, 2020

slontis left a comment

Choose a reason for hiding this comment

jon-oracle commented Sep 14, 2020

slontis left a comment

Choose a reason for hiding this comment

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaduk commented Sep 16, 2020

paulidale commented Sep 17, 2020

kaduk commented Sep 17, 2020

paulidale commented Sep 17, 2020

paulidale commented Sep 17, 2020

paulidale commented Sep 8, 2020 •

edited

Loading