Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement KAT for KBKDF with KMAC128 #23745

Closed
wants to merge 1 commit into from
Closed

Conversation

jvdsn
Copy link
Contributor

@jvdsn jvdsn commented Mar 5, 2024

FIPS 140-3 IG 10.3.A lists the following requirements regarding self-tests:

At the minimum, the cryptographic module shall perform a CAST for one of the functions defined in FIPS 202: SHA3-224, SHA3-256, SHA3-384, SHA3-512, SHAKE128 and SHAKE256, no matter how many of these functions the module may be designed to use.

Note2: The reason for requiring a self-test for only one of the FIPS 202-compliant hash functions is that all of these functions, including SHAKE128 and SHAKE256, rely on the same underlying Keccak-p permutation. Note that this is different from the SHA-1 and SHA-2 self-test requirements where separate self-tests are needed for SHA-1, SHA-256 and SHA-512, if the module is designed to use these hash functions.

These make it clear that, if a cryptographic module implements SHA-3 and SHAKE (from FIPS 202), it is sufficient to test either one of those functions to cover all of them.

However, for SP 800-185 (which defines additional XOFs based on Keccak), the IG says the following:

One CAST for one of the functions defined in SP 800-185: cSHAKE-128, cSHAKE-256, KMAC128, KMAC256, TupleHash and ParallelHash, is sufficient to meet the self-test requirements for all SP 800-185 functions implemented in the module, as long as all use the same underlying implementation of the Keccak-p permutation.

If FIPS 202 SHA-3 functions are in the same module and they share the same SHAKE implementation, then one CAST for either one SP 800-185 function or one FIPS 202 function is sufficient to meet the CAST requirement for SP 800-185 functions.

If a module supports both FIPS 202 and SP 800-185 functions and they share the same SHAKE implementation (either SHAKE128 or SHAKE256), then one CAST on a function defined in either SP 800-185 or FIPS 202 is sufficient to meet the CAST requirement for SP 800-185 functions.

Note4: The reason for allowing a CAST for either the SP 800-185 or SHAKE-based FIPS 202 hash functions is because all of these functions rely on the same SHAKE128 or SHAKE256 function, and the SP 800-185 functions do not add significant complexity to this underlying function.

In its entirety, this text is quite ambiguous. The intent seems to be that, to cover the SP 800-185 functions, one needs to implement a self-test for a function based on SHAKE (i.e. SHAKE128, SHAKE256, cSHAKE, KMAC...). SHA-3 is not defined in terms of SHAKE, but in terms of Keccak, and thus a SHA-3 KAT would not cover cSHAKE/KMAC/... On the other hand, SHA-3 and SHAKE seem to be considered equivalent for the purpose of self-testing FIPS 202 functions.

Because OpenSSL implements KMAC (an SP 800-185 function), I would like to propose that another KAT is added to test a SHAKE-based function. I'm aware that OpenSSL 3 already has FIPS 140-2 validation and is in the queue for FIPS 140-3 validation. Perhaps you have already discussed this with your lab and decided this would be a non-issue. I simply propose this KAT to err on the safe side.

There's a few options:

  • Test SHAKE128, which would cover both SHA-3, SHAKE, and KMAC, and remove the existing SHA3-256 self-test. self_test_kats.c currently does not support XOFs.
  • Test KMAC128, which would cover SHAKE and KMAC, but maybe not SHA-3. Even though it would make logical sense, I currently can't find in the IG where the FIPS 202 self-test requirements can be met by testing an SP 800-185 function. Regardless, self_test_kats.c currently does not support MACs.
  • Test KBKDF with KMAC128, which would cover SHAKE, KMAC, and KBKDF, though more complex. Currently, self_test_kats.c already supports KBKDF, so adding this KAT only requires small changes to self_test_data.inc.

The test data for this KAT was collected using the NIST ACVTS.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 5, 2024
@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Mar 6, 2024
@slontis
Copy link
Member

slontis commented Mar 11, 2024

I dont think this is necessary since the underlying construct is KECCAK.
I would prefer we dont add tests unless absolutely required, especially since the ability to run the self tests just once on install disappeared in FIPS 140-3.

@jvdsn
Copy link
Contributor Author

jvdsn commented Mar 11, 2024

I dont think this is necessary since the underlying construct is KECCAK. I would prefer we dont add tests unless absolutely required, especially since the ability to run the self tests just once on install disappeared in FIPS 140-3.

Right, and the ambiguity of the IG is the main problem here, ideally this would be clarified by the CMVP.

What about replacing the existing SHA3-256 test with a SHAKE128 test? Would that be considered a regression by OpenSSL?

@t8m t8m added triaged: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing labels Mar 12, 2024
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When comparing with other self tests, this one takes negligible amount of time. So IMO OK to add it even if it might be redundant by some interpretations of IGs.

@t8m t8m 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 Mar 12, 2024
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Mar 13, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Mar 13, 2024
@t8m
Copy link
Member

t8m commented Mar 13, 2024

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Mar 13, 2024
openssl-machine pushed a commit that referenced this pull request Mar 13, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23745)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants