PQC: FrodoKEM #3679

atreiber94 · 2023-08-23T13:00:38Z

Closes #3106 by introducing FrodoKEM as specified in the ISO proposal.

Includes all parameter sets:
- FrodoKEM and ephemeral FrodoKEM ("eFrodoKEM").
- AES and SHAKE modes for generating the public matrix.
- All supported matrix sizes (640, 976, 1344).
Includes optimizations from the reference implementation
Includes integration into TLS 1.3, X.509, and CLI speed. Compatibility was tested with a liboqs TLS server. If desired, we can also split this integration into separate PRs.
We did not integrate FrodoKEM into FFI yet.

Open Questions:

Since FrodoKEM is still in the proposal stage of standardization, should we use separate identifiers (similar to "R3" for Kyber)?
How to generalize FFI (see Future Work)?

Future Work:

FFI: The current issue is that a solution similar to Kyber's does not work, since we can't determine the parameter set uniquely based on the given key_len. It would be possible to load FrodoKEM keys with an additional parameter specifying the parameter set. However, it may be more worthwhile to provide a general botan_privkey_load interface, similar to botan_privkey_create.
Further optimizations may still be possible (SSE, ...).

src/lib/pubkey/frodokem/frodo_matrix.cpp

coveralls · 2023-09-28T12:47:26Z

coverage: 92.008% (-0.06%) from 92.066%
when pulling c256e53 on Rohde-Schwarz:pqc/frodokem
into ceace35 on randombit:master.

lieser

I did just a very quick look at the code, no detailed code review or trying to follow the Frodo algorithm.

src/lib/pubkey/frodokem/frodo_constants.cpp

src/lib/pubkey/frodokem/frodo_constants.h

src/lib/pubkey/frodokem/frodo_matrix.h

lieser · 2023-10-11T16:51:48Z

src/lib/pubkey/frodokem/frodo_matrix.cpp

+   // Multiply by s on the left
+   // Inputs: b (N x N_BAR), s (N_BAR x N), e (N_BAR x N_BAR)
+   // Output: out = s*b + e (N_BAR x N_BAR). The existing elements are overwritten and a self reference is returned.


Do you think this descriptions are more important for understanding the implementation than for a there the function is used, or why are they here?
Not completely against keeping this style here, but to me looks a lot like a description that belongs in a docstring.

Same for all the other functions in this file.

Tbh we relied on it for the implementation of the function itself but it would also be useful when calling the function (since the necessary dimensions are shown). I put them into docstrings accordingly.

You just moved them in the header, but they are not yet proper docstrings.

src/lib/pubkey/frodokem/frodo_matrix.cpp

src/lib/pubkey/frodokem/frodo_mode.h

FAlbertDev

I didn't compare the code against the specification. I only left some remarks. All in all, LGTM!

doc/dev_ref/oids.rst

src/lib/pubkey/frodokem/frodo_constants.cpp

src/lib/pubkey/frodokem/frodo_matrix.cpp

src/lib/pubkey/frodokem/frodo_matrix.h

src/lib/pubkey/frodokem/frodo_matrix.cpp

src/lib/pubkey/frodokem/frodo_types.h

src/lib/pubkey/frodokem/frodokem.h

src/lib/tls/tls13_pqc/hybrid_public_key.cpp

FAlbertDev · 2023-10-13T08:20:15Z

src/scripts/dev_tools/gen_frodo_kat.py

+    if args is None:
+        args = sys.argv
+
+    output = open('src/tests/data/pubkey/frodokem_kat.vec', 'w')


More modern Python would be: with open(...) as output:.

Super modern python would also be to add encoding='utf8'. 😨 This script is more documentation than actual functionality, though. So not super important IMO.

src/tests/test_frodokem.cpp

randombit

This is a first pass review of everything except the frodo_matrix.cpp file

doc/dev_ref/oids.rst

randombit · 2023-10-31T10:11:36Z

src/lib/pubkey/frodokem/frodo_constants.cpp

+FrodoKEMConstants::FrodoKEMConstants(FrodoKEMMode mode) : m_mode(mode), m_len_a(128), m_n_bar(8) {
+#if !defined(BOTAN_HAS_AES)
+   BOTAN_ARG_CHECK(!mode.is_aes(), "cannot instantiate AES-based FrodoKEM: This build does not support AES");
+#endif


Instead of having to do a macro check along with mode.is_aes() let's add

mode.is_available() (or is_supported or ...)

and use it here, in speed.cpp, and anywhere else we're doing an explicit BOTAN_HAS_AES check.

This also generalizes if we decide to make SHAKE modes optional in the future.

I added this method and used it when possible, but in some instances (row generation, KATs, and PK_Key_Generation_Test) compile time checks were still needed.

randombit · 2023-10-31T10:13:43Z

src/tests/test_frodokem.cpp

+         Botan::FrodoKEMMode::eFrodoKEM1344_AES, Botan::FrodoKEMMode::eFrodoKEM976_AES,
+         Botan::FrodoKEMMode::eFrodoKEM640_AES, Botan::FrodoKEMMode::FrodoKEM1344_AES,
+         Botan::FrodoKEMMode::FrodoKEM976_AES, Botan::FrodoKEMMode::FrodoKEM640_AES
+   #endif


Instead of excluding the modes at compile time in this way lets just continue if !mode.is_supported()

src/tests/test_frodokem.cpp

randombit · 2023-10-31T10:31:19Z

src/lib/pubkey/frodokem/frodokem.cpp

+}
+
+OID FrodoKEM_PublicKey::object_identifier() const {
+   return OID::from_string(m_public->constants().mode().to_string());


Couldn't this use mode().object_identifier()?

randombit · 2023-10-31T10:37:35Z

src/lib/pubkey/frodokem/frodokem.cpp

+                                std::tuple(consts.n_bar(), consts.n()),
+                                shake.output<FrodoSampleR>(sizeof(uint16_t) * consts.n_bar() * consts.n()));
+
+         const auto e_p =


We have here three repetitions of

FrodoMatrix::sample(consts, std::tuple(consts.n_bar(), consts.XXX()), shake.output<FrodoSampleR>(sizeof(uint16_t) * consts.n_bar() * consts.XXX())

I'm not sure if this is the best approach but: at the minimum it seems like a lambda that captures shake as a mutable reference and takes XXX (either n_bar or n) as an argument would clean this up.

There are three similar invocations during decrypt - maybe a sample helper on FrodoMatrix or a function in an anon namespace in this file would be better than a lambda which would still have to be duplicated.

Added a helper function to FrodoMatrix. This nicely reduces complexity, thanks for the tip!

randombit · 2023-10-31T10:40:54Z

src/lib/pubkey/frodokem/frodokem.cpp

+         b_pp.reduce(consts);
+         c_p.reduce(consts);
+         const bool cmp_b_p_b_pp = b_p.constant_time_compare(b_pp);
+         const bool cmp_c_c_p = c.constant_time_compare(c_p);


These constant_time_compare calls should probably return CT::Mask instead of bools

Agreed, thanks for pointing this out!

conditional_copy_mem now also directly uses the mask instead of the value.

randombit · 2023-10-31T10:44:23Z

src/lib/pubkey/frodokem/frodo_matrix.cpp

+#include <botan/mem_ops.h>
+#if defined(BOTAN_HAS_AES)
+   #include <botan/internal/aes.h>
+#endif


Larger question: is there really a reason

a) To include the AES support at all? It seems like everyone is moving to SHAKE for pqc

b) If AES support is required/useful (for standards conformance or contract reasons, etc): can we just make the AES support also mandatory? 99% of the time you want AES anyway for other reasons, and while you maybe don't need Frodo-AES, in the current approach there isn't any way to disable that anyway; if you enable plain old AES, plus Frodo, you get Frodo-AES. This contrasts with how Kyber was done where Kyber-90s is a distinct module that can be toggled off.

To include the AES support at all?

In contrast to the NIST drafts, currently the ISO draft for (e)FrodoKEM explicitly specifies the AES variant. Otherwise we would have left it out.

This contrasts with how Kyber was done where Kyber-90s is a distinct module that can be toggled off.

Initially, we planned to build two sub-modules for FrodoKEM as well. However, in contrast to Kyber, AES in FrodoKEM is somewhat an "add-on", as both FrodoKEM-SHAKE and FrodoKEM-AES depend on SHAKE. 😞 So FrodoKEM without SHAKE doesn't make sense while FrodoKEM without AES still works partially.
You make a fair point, though, that we currently cannot switch off AES-support in FrodoKEM if both frodokem and aes are enabled in the build.

Let us try to extract the AES portions into a very thin module, see how much complexity that adds and then decide whether its worth it or not.

Let us try to extract the AES portions into a very thin module, see how much complexity that adds and then decide whether its worth it or not.

Done here: 0694f1d
In my opinion the AES sub module doesn't bring much added complexity for giving users the flexibility to switch it off.

I agree that it's nice to be able to switch of FrodoKEM-AES even if you have the AES module.

What do you think about a FrodoKEM-SHAKE module?

It does not save any compile complexity but imo a minimized build with --enable-modules=frodokem_shake for FrodoKEM-SHAKE, --enable-modules=frodokem_aes for FrodoKEM-AES, and --enable-modules=frodokem for both variants would be clearer than the current solution. It also covers the case where one would want to disallow FrodoKEM-SHAKE. I know we'd prefer always using FrodoKEM-SHAKE but at least according to the current ISO specification both variants are equal, so one would expect to get both with the "FrodoKEM" module and not with the "FrodoKEM-AES" module.

For the sake of consistency, we could certainly do that. Note that you will need to explicitly enable frodokem_shake and frodokem_aes. There's (AFAIK) no concept of a "virtual" module feature that would enable both variants. Similarly, kyber enables the Keccak-based Kyber and kyber_90s enables the AES variant. There's an (internal) kyber_common with the base implementation that both flavors depend on.

I applied this suggestion. Now there are three modules:

frodokem_common (internal module) with the implementation,

frodokem_aes the AES-based matrix generator (depends on aes and frodokem_common), and

frodokem the SHAKE-based matrix generator (depends on shake_xof and frodokem_common)

It is possible to enable either frodokem_aes or frodokem individually or both at the same time.

src/lib/pubkey/frodokem/frodo_matrix.cpp

atreiber94 · 2023-11-06T16:02:00Z

Thanks for the review! We now addressed all comments except the discussions on the modules and whether to skip KATs without AES support.

reneme · 2023-11-16T10:48:28Z

Rebased to latest master and squashed commit history.

reneme · 2023-12-22T08:55:27Z

Rebased again, incorporated (the now closed) #3808.

randombit

LGTM, left a few style comments on bits that are probably leftover from the C

randombit · 2023-12-30T17:53:25Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+   const size_t nwords = (constants.n_bar() * constants.n_bar()) / 8;
+   uint16_t temp;
+   uint16_t maskex = static_cast<uint16_t>(1 << constants.b()) - 1;
+   uint16_t maskq = static_cast<uint16_t>(1 << constants.d()) - 1;


I think these two can both be const

randombit · 2023-12-30T17:55:00Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+
+   size_t index = 0;
+   for(size_t i = 0; i < nwords; i++) {
+      templong = 0;


templong should be declared here rather than being function scope.

randombit · 2023-12-30T17:55:18Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+   for(size_t i = 0; i < nwords; i++) {
+      templong = 0;
+      for(size_t j = 0; j < 8; j++) {
+         temp = static_cast<uint16_t>(((m_elements.at(index) & maskq) + (1 << (constants.d() - constants.b() - 1))) >>


temp could be declared/initialized here and not be function scope

randombit · 2023-12-30T17:56:05Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+      uint8_t b = 0;  // bits in out[i] already filled in
+      while(b < lsb) {
+         const uint8_t nbits = std::min(static_cast<uint8_t>(lsb - b), bits);
+         uint16_t mask = static_cast<uint16_t>(1 << nbits) - 1;


mask can be const right?

randombit · 2023-12-30T18:03:03Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+         auto& current = elements.at(i * constants.n_bar() + j);
+         current = 0;
+         for(size_t k = 0; k < constants.n(); ++k) {
+            current += static_cast<uint16_t>(


This expression is very hard to read thanks to how the inner comments get formatted. Splitting it out to a couple of variables with leading comments would probably help a lot, eg

const uint32_t b_ink = b.elements_at(...); // Since ... const uint32_t s_jnk = s.elements_at(...); current += ...

randombit · 2023-12-30T18:04:10Z

src/lib/pubkey/frodokem/frodokem_common/frodo_matrix.cpp

+   for(size_t i = 0; i < n; ++i) {
+      uint32_t sample = 0;                   // Avoid integral promotion
+      uint16_t prnd = elements.at(i) >> 1;   // Drop the least significant bit
+      uint16_t sign = elements.at(i) & 0x1;  // Pick the least significant bit


These two uint16_t can be const right?

Integrated into CLI benchmarking tool and X.509 tests Co-Authored-By: René Meusel <rene.meusel@rohde-schwarz.com>

Co-Authored-By: René Meusel <rene.meusel@rohde-schwarz.com>

reneme · 2024-01-02T12:32:02Z

Thanks, @randombit! We addressed the review comments. Currently, we're looking deeper into the application of the newly added endian load/store mechanics in FrodoKEM. But maybe we should leave these for after the Botan 3.3.0 release. What do you think?

Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>

reneme · 2024-01-04T16:15:18Z

But maybe we should leave these for after the Botan 3.3.0 release.

I'm quite certain we want to keep those refactorings for after the release. They require parts of #3869 which we certainly don't want to merge just before the release.

Let's revisit this 3.3.0 is out.

github-advanced-security bot found potential problems Aug 24, 2023

View reviewed changes

src/lib/pubkey/frodokem/frodo_matrix.cpp Fixed Show fixed Hide fixed

src/lib/pubkey/frodokem/frodo_matrix.cpp Fixed Show fixed Hide fixed

atreiber94 mentioned this pull request Sep 8, 2023

FrodoKEM Cryptodoc sehlen-bsi/botan-docs#117

Merged

reneme force-pushed the pqc/frodokem branch from a49f970 to fe72599 Compare September 28, 2023 11:39

atreiber94 force-pushed the pqc/frodokem branch from fa74b71 to 1ae5b96 Compare October 5, 2023 09:38

reneme force-pushed the pqc/frodokem branch 2 times, most recently from 322b34e to e0c7fd8 Compare October 5, 2023 10:07

reneme mentioned this pull request Oct 5, 2023

[TLS 1.3] Hybrid PQ/T key establishment #3609

Merged

5 tasks

atreiber94 force-pushed the pqc/frodokem branch 2 times, most recently from ea6d352 to 3a043ff Compare October 9, 2023 10:06

reneme mentioned this pull request Oct 9, 2023

Add missing simd requires to aes_ni #3741

Merged

reneme force-pushed the pqc/frodokem branch from bb53614 to a3e531c Compare October 9, 2023 13:24

atreiber94 marked this pull request as ready for review October 10, 2023 14:04

reneme requested review from reneme and lieser October 10, 2023 14:04

reneme assigned atreiber94 Oct 10, 2023

reneme added this to the Botan 3.3.0 milestone Oct 10, 2023

reneme added the enhancement Enhancement or new feature label Oct 10, 2023

randombit self-requested a review October 11, 2023 11:41

lieser reviewed Oct 12, 2023

View reviewed changes

FAlbertDev reviewed Oct 13, 2023

View reviewed changes

atreiber94 requested review from FAlbertDev and lieser October 17, 2023 13:19

randombit reviewed Oct 31, 2023

View reviewed changes

reneme force-pushed the pqc/frodokem branch from b32703e to cdcaad5 Compare November 2, 2023 06:58

reneme force-pushed the pqc/frodokem branch from bf255a0 to 236627e Compare November 14, 2023 17:48

reneme mentioned this pull request Nov 16, 2023

Generalize PQC KEM KAT Runner + Minor Adaptions in Kyber #3807

Merged

reneme force-pushed the pqc/frodokem branch from 236627e to 0c780c6 Compare November 16, 2023 10:46

reneme mentioned this pull request Nov 16, 2023

Use generic PQC KEM KATs for FrodoKEM #3808

Closed

reneme force-pushed the pqc/frodokem branch from 0c780c6 to 92914b7 Compare November 16, 2023 11:05

reneme force-pushed the pqc/frodokem branch from 92914b7 to d3e8873 Compare December 22, 2023 08:52

reneme force-pushed the pqc/frodokem branch from d3e8873 to f3ffdbb Compare December 22, 2023 09:04

randombit approved these changes Dec 30, 2023

View reviewed changes

reneme and others added 4 commits January 2, 2024 10:55

FIX: how to regenerate oid_maps.cpp/x509_dn_ub.cpp

dddcd3b

Add subscript operator to strong type

fac04ab

Implement FrodoKEM and eFrodoKEM according to ISO 20230314

21b52d3

Integrated into CLI benchmarking tool and X.509 tests Co-Authored-By: René Meusel <rene.meusel@rohde-schwarz.com>

Support eFrodoKEM in TLS 1.3 (also as hybrid PQ/T)

57f46d9

Co-Authored-By: René Meusel <rene.meusel@rohde-schwarz.com>

reneme force-pushed the pqc/frodokem branch from f3ffdbb to 268a36b Compare January 2, 2024 12:30

address review comments

c256e53

Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>

reneme force-pushed the pqc/frodokem branch from 268a36b to c256e53 Compare January 2, 2024 16:51

reneme merged commit a2cb135 into randombit:master Jan 4, 2024
39 checks passed

reneme deleted the pqc/frodokem branch January 4, 2024 16:15

PQC: FrodoKEM #3679

PQC: FrodoKEM #3679

Conversation

atreiber94 commented Aug 23, 2023 • edited Loading

coveralls commented Sep 28, 2023 • edited Loading

lieser 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

FAlbertDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randombit 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

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

reneme Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

atreiber94 commented Nov 6, 2023

reneme commented Nov 16, 2023

reneme commented Dec 22, 2023

randombit 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

reneme commented Jan 2, 2024

reneme commented Jan 4, 2024

atreiber94 commented Aug 23, 2023 •

edited

Loading

coveralls commented Sep 28, 2023 •

edited

Loading

reneme Nov 14, 2023 •

edited

Loading