Perform key size check for HMAC, HKDF, SSHKDF and TLS KDF #23900

0140454 · 2024-03-20T10:22:48Z

According to SP 800-131Ar2, the length of the key-derivation key shall be at least 112 bits.

This PR introduces key size checks in HMAC, HKDF, SSHKDF and TLS KDF.

Since the key shorter than 112 bits can be used for legacy use in HMAC generation, a pedantic flag is introduced to control whether to perform the check in HMAC.

Similarly, a pedantic flag is also introduced in KDFs to turn off the check for some scenario, such as ignoring salt length check.

With this changes, we can use API return value to indicate whether the operation is approved.

This PR depends on #23833.

Checklist

documentation is added or updated
tests are added or updated

paulidale

Good effort.

I'm not convinced that pedantic should be exposed outside the FIPS provider. It's not used by the default provider at all and thus would be better omitted.

Test failures seem relevant.

providers/implementations/kdfs/hkdf.c

paulidale · 2024-03-20T22:54:49Z

providers/implementations/kdfs/hkdf.c

+     * Set pedantic to zero to allow a salt with arbitrary length.
+     */
+    int pedantic = 0;
+    OSSL_PARAM mac_params[] = {


Unfortunately, this won't build for some platforms which consider &pedantic to not be constant.

Does this impact FIPS compliance?

Fixed.

Our lab think it only need to ensure that HMAC function and key are strong enough. Therefore, I think the length of salt should not impact the compliance.

You misunderstood. Taking the address of pedantic is not considered a constant initialiser on some platforms. I.e. this code will not compile.

This is why we use other methods to construct OSSL_PARAM arrays at runtime.

The method to construct OSSL_PARAM arrays changed.

slontis

I would suggest that this PR is somewhat dependent on fips indicators being resolved. See #23609. We definitely want a consistent way of addressing all of the issues (not just the keysize checks).

slontis · 2024-03-20T23:21:41Z

For the MAC, I think there are multiple things to consider

There is currently no way to distinguish the difference between a MAC generate and a MAC verify operation... I would consider the MAC verify to be the 'legacy' operation that may allow a smaller key size.
This depends on what we decide to do with fips indicators - but if we use the a 'strict' mode- inside the FIPS provider it could have the pedantic flag on by default - at least for HMAC generation.

paulidale

Does this mandate changes to the build.cfg files?

paulidale · 2024-03-22T05:11:38Z

providers/implementations/kdfs/hkdf.c

+     * Set pedantic to zero to allow a salt with arbitrary length.
+     */
+    int pedantic = 0;
+    OSSL_PARAM mac_params[] = {


You misunderstood. Taking the address of pedantic is not considered a constant initialiser on some platforms. I.e. this code will not compile.

This is why we use other methods to construct OSSL_PARAM arrays at runtime.

0140454 · 2024-03-22T05:42:55Z

Does this mandate changes to the build.cfg files?

@paulidale Sorry that I don't understand your meaning. Do you want to known whether this changes also need to modify build.info files?

slontis · 2024-03-22T05:53:15Z

test/recipes/30-test_evp_data/evpkdf_hkdf.txt

+Title = HKDF bad key size test
+
+Availablein = fips
+KDF = HKDF


These are going to need a line specifying which provider version this test is supported by....
(Because we test against old providers with the current source).

i.e. (Assuming OpenSSL 3.3 has been released)..
FIPSVersion = >=3.4.0

Until 3.3 is release you will have to use >= 3.3.0, and then change it later

Changed to FIPSversion = >=3.3.0 because v3.3.0 is still in alpha stage.

slontis · 2024-03-22T05:58:21Z

ssl/quic/quic_record_util.c

+    /*
+     * According to RFC 9000, the length of destination connection ID must be
+     * at least 8 bytes. It means that the length of destination connection ID
+     * may be less than the minimum length for HKDF required by FIPS provider.


Not sure this should be done here.. It should probably break in FIPS mode.

The operation may fail if FIPS provider is used with old libssl.so.

I think it cannot be avoided unless

move the ossl_quic_hkdf_extract function into FIPS provider

or has a global pedantic flag, which is turned off in default for back-compatibility, in fipsmodule.cnf

If I am wrong, please correct me. Thank you.

@t8m or @paulidale Do you have an opinion on this, technically it should fall over here..

Yeah #$%@! FIPS rules bite again. The initial secret where this is used is not really security relevant so IMO OK to set pedantic to 0.

slontis · 2024-03-22T05:59:49Z

test/recipes/30-test_evp_data/evpkdf_tls12_prf.txt

@@ -43,7 +43,16 @@ Output = d06139889fffac1e3a71865f504aa5d0d2a2e89506c6f2279b670c3e1b74f531016a253

 # Missing digest.
 KDF = TLS1-PRF
+Ctrl.Pedantic = pedantic:0


Is this needed here and in other places? It is FIPS specific..

Current implementation performs key size check in set_ctx_params function.

The secret to set in this test case is only 8 bits.
Therefore, it is needed to set pedantic flag to zero to make it pass through the logic for checking digest.

Update:

I will make this test case only available in default provider and remove Ctrl.Pedantic in next commit.

Make this test case only available in default provider.

slontis · 2024-03-22T06:03:59Z

Pauli is asking whether you checked the build.info files for any files that you have added #ifdef FIPS_MODULE to...

The related .o file needs to go into both libdefault and libfips.

slontis · 2024-03-22T06:05:00Z

For fips indicator purposes there should also be a getter

0140454 · 2024-03-25T02:43:29Z

Pauli is asking whether you checked the build.info files for any files that you have added #ifdef FIPS_MODULE to...

The related .o file needs to go into both libdefault and libfips.

@paulidale All files that I have added #ifdef FIPS_MODULE have been included in libdefault.a and libfips.a before this PR.

For fips indicator purposes there should also be a getter

@slontis A getter has been added.

slontis · 2024-03-25T03:12:52Z

test failures are relevant.
Hopefully soon OTC will agree on the way forward with fips indicators. I would prefer to wait till that point before approving.

0140454 · 2024-03-28T15:09:05Z

Rebased dev branch onto current master branch.

To make test pass, the commit from #23833 picked and it will be removed in the future.

0140454 · 2024-04-11T08:07:03Z

Rebase dev branch onto current master branch, and change FIPSversion = >=3.3.0 to FIPSversion = >=3.4.0 because v3.3.0 released.

openssl-machine · 2024-05-21T00:09:20Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 20, 2024

paulidale reviewed Mar 20, 2024

View reviewed changes

slontis reviewed Mar 20, 2024

View reviewed changes

paulidale reviewed Mar 22, 2024

View reviewed changes

slontis reviewed Mar 22, 2024

View reviewed changes

t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Mar 25, 2024

0140454 force-pushed the key-size-check branch from 554fd25 to 14bf1f5 Compare March 28, 2024 15:08

0140454 force-pushed the key-size-check branch 2 times, most recently from de6e90e to cb64d51 Compare March 30, 2024 17:27

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: present The PR has suitable tests present labels Apr 3, 2024

0140454 added 4 commits April 11, 2024 14:45

Perform key size check for HMAC

8df9a86

Perform key size check for HKDF, SSHKDF and TLS KDF

884f4ed

Make pedantic flag be conditioned on FIPS_MODULE

c3df371

Add getter for pedantic flag in MAC, HKDF, SSHKDF and TLS KDF

ff6f39e

0140454 force-pushed the key-size-check branch from cb64d51 to ff6f39e Compare April 11, 2024 06:54

fixup! Add getter for pedantic flag in MAC, HKDF, SSHKDF and TLS KDF

9073840

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform key size check for HMAC, HKDF, SSHKDF and TLS KDF #23900

Perform key size check for HMAC, HKDF, SSHKDF and TLS KDF #23900

0140454 commented Mar 20, 2024 •

edited

paulidale left a comment

paulidale Mar 20, 2024

0140454 Mar 21, 2024

paulidale Mar 22, 2024

0140454 Mar 22, 2024

slontis left a comment •

edited

slontis commented Mar 20, 2024

paulidale left a comment

paulidale Mar 22, 2024

0140454 commented Mar 22, 2024

slontis Mar 22, 2024

slontis Mar 22, 2024

0140454 Mar 25, 2024

slontis Mar 22, 2024

0140454 Mar 25, 2024

slontis Apr 5, 2024

t8m Apr 5, 2024

slontis Mar 22, 2024

0140454 Mar 25, 2024 •

edited

0140454 Mar 30, 2024

slontis commented Mar 22, 2024

slontis commented Mar 22, 2024

0140454 commented Mar 25, 2024

slontis commented Mar 25, 2024

0140454 commented Mar 28, 2024

0140454 commented Apr 11, 2024

openssl-machine commented May 21, 2024

Perform key size check for HMAC, HKDF, SSHKDF and TLS KDF #23900

Are you sure you want to change the base?

Perform key size check for HMAC, HKDF, SSHKDF and TLS KDF #23900

Conversation

0140454 commented Mar 20, 2024 • edited

Checklist

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

slontis left a comment • edited

Choose a reason for hiding this comment

slontis commented Mar 20, 2024

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0140454 commented Mar 22, 2024

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

0140454 Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis commented Mar 22, 2024

slontis commented Mar 22, 2024

0140454 commented Mar 25, 2024

slontis commented Mar 25, 2024

0140454 commented Mar 28, 2024

0140454 commented Apr 11, 2024

openssl-machine commented May 21, 2024

0140454 commented Mar 20, 2024 •

edited

slontis left a comment •

edited

0140454 Mar 25, 2024 •

edited