search

randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.59k stars 570 forks source link

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

Closed reneme closed 1 month ago

reneme commented 1 month ago

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).

coveralls commented 1 month ago

Coverage Status

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

tobiasbrunner commented 1 month ago

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 ).

randombit commented 1 month ago

@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.