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

FFI: Loading of raw FrodoKEM keys & FIX: "insufficient buffer handling" in FFI's decapsulate #4373

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Oct 14, 2024

Drive-by fixes

  1. While adding and testing raw FrodoKEM key loading to the FFI, I found a bug in botan_pk_op_kem_decrypt_shared_key(). When provided with an insufficiently large buffer to output the shared key, this function would fail to return BOTAN_FFI_ERROR_INSUFFICIENT_BUFFER_SPACE and instead claim success. This gets fixed in the first commit of this pull request.
  2. When Botan was configured without (say) FrodoKEM-AES, but a user would still try to load a key of that mode, an assertion was triggered. The second commit in this pull request replaces this assertion by throwing Not_Implemented.

@randombit Should we create an independent PRs for these fixes? I dropped it in here, as I don't expect that many people actually used the KEM interface prior to PQC.

Description

This introduces botan_privkey_load_frodokem(), and botan_pubkey_load_frodokem() to conveniently decode raw FrodoKEM keys via the FFI (see discussion in #4366). Note that raw encoding is implemented generically, see #4368.
I opted to not implement the loading generically, for consistency with the existing low-level "raw" decodings of RSA, ECC and friends. But technically (for the PQC-algos), we could also go for a generic approach along those lines: load_generic_*(&key, encoded_key, encoded_key_len, algo_name, algo_mode_descriptor). @randombit What's your view on this?

Also, this adds a fairly extensive and generic test for the KEM support in FFI that I'm planning to re-use for ML-KEM (#3893) and Classic McEliece (#3883).

When passing an insufficiently sized buffer into
botan_pk_op_kem_decrypt_shared_key() it returned BOTAN_FFI_SUCCESS
instead of BOTAN_FFI_ERROR_INSUFFICIENT_BUFFER_SPACE.
@reneme reneme added bug enhancement Enhancement or new feature labels Oct 14, 2024
@reneme reneme added this to the Botan 3.6.0 milestone Oct 14, 2024
@reneme reneme requested a review from randombit October 14, 2024 10:12
@reneme reneme self-assigned this Oct 14, 2024
... the assertion replaced by this was easily triggerable via the
public API, simply by trying to create a FrodoKEM key pair of a
mode that is not available in the particular build of Botan.
... this will be used as a base class to test other (PQ)-KEMs
@coveralls
Copy link

coveralls commented Oct 14, 2024

Coverage Status

coverage: 91.118% (-0.002%) from 91.12%
when pulling cb9b92b on Rohde-Schwarz:ffi/frodo_key_loading
into ed74c95 on randombit:master.

This was referenced Oct 14, 2024
@tobiasbrunner
Copy link
Contributor

Thanks for adding these missing functions. I've successfully tested public key de-/encoding for FrodoKEM via the feature/mlkem_ipd branch from your repository (after you rebased it on this branch).

I've not tested private key loading because our KAT vectors are currently based on a DRBG seed and the FrodoKEM private key format is not just the seeds (s || seedSE || z), like with ML-KEM, but the more complex structure from the specs that contains mostly derived/computed data (s || seedA || b || ST || pkh). Do you know if there was/is any discussion about changing the format to that used by ML-KEM/DSA? The seeds would be a lot more compact than the resulting expanded private key (which also includes the public key, seedA || b, and a hash thereof, pkh ).

return BOTAN_FFI_SUCCESS;
});
#else
BOTAN_UNUSED(key, privkey, key_len, frodo_mode);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy & paste issue:

Suggested change
BOTAN_UNUSED(key, privkey, key_len, frodo_mode);
BOTAN_UNUSED(key, pubkey, key_len, frodo_mode);

@randombit
Copy link
Owner

@reneme Fine to include the fix in this PR, don't see a need to split it out. Also fine to make the loader function algorithm specific; we may well want a generic approach in the long run but that has been the status quo for FFI up until now, and designing something at the last minute (wrt 3.6.0) doesn't seem wise.

@reneme reneme merged commit 4996790 into randombit:master Oct 15, 2024
38 checks passed
@reneme reneme deleted the ffi/frodo_key_loading branch October 15, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement Enhancement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants