PQC: Classic McEliece #3883

FAlbertDev · 2024-01-11T12:04:01Z

This PR relates to the Classic McEliece KEM as specified in this ISO draft. It also contains the instances defined in the NIST Round 4 submission. The test cases were generated using the NIST submissions reference implementation. Note that Classic McEliece (module cmce) is not the original McEliece Algorithm that is implemented in Botan's mce module. See the Classic McEliece homepage, for a brief comparison.

TODO Tracker

coveralls · 2024-01-11T12:37:18Z

coverage: 91.85% (+0.1%) from 91.713%
when pulling 9a6f4df on Rohde-Schwarz:pqc/classic_mceliece
into 6945e90 on randombit:master.

reneme

This mostly looks at cmce_decaps.cpp, cmce_encaps.cpp and cmce.cpp, noting many minor code style things and a few suggestions for alternative code structuring. Not looking into the Classic McEliece specifics at all here.

src/build-data/oids.txt

src/lib/pubkey/classic_mceliece/cmce_types.h

src/lib/pubkey/classic_mceliece/cmce_matrix.cpp

src/lib/pubkey/classic_mceliece/cmce.cpp

src/lib/pubkey/classic_mceliece/cmce_encaps.cpp

src/lib/utils/strong_type.h

reneme

First pass on cmce_field_orderings.cpp. I'm somewhat concerned that this isn't very efficient. But it may well be "good enough". Should we profile that?

src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp

reneme

More comments. I didn't look at cmce_parameter_set.*, cmce_parameters_* and cmce_poly.*, yet.

src/lib/pubkey/classic_mceliece/cmce_keys_internal.cpp

src/lib/utils/strong_type.h

src/lib/pubkey/classic_mceliece/cmce_matrix.cpp

src/lib/pubkey/classic_mceliece/cmce.cpp

reneme · 2024-01-26T10:16:03Z

src/lib/pubkey/classic_mceliece/cmce_gf.cpp

+      // TODO: Only for test instances. Remove on final PR
+      size_t m = Classic_McEliece_GF::log_q_from_mod(mod);
+
+      for(int i = static_cast<int>(m) - 2; i >= 0; --i) {
+         x ^= CT::Mask<uint32_t>::expand((uint32_t(1) << (i + m)) & x)
+                 .if_set_return(static_cast<uint32_t>(mod.get()) << i);
+      }
+
+      return GF_Elem(static_cast<uint16_t>(x));


Don't forget to remove (and probably replace by some exception?)

src/lib/pubkey/classic_mceliece/cmce_matrix.cpp

reneme · 2024-01-29T16:11:55Z

src/lib/pubkey/classic_mceliece/cmce_matrix.cpp

+Code_Word Classic_McEliece_Matrix::mul(const Classic_McEliece_Parameters& params, const Error_Vector& e) const {
+   auto s = e.subvector(0, params.pk_no_rows());
+   auto e_T = e.subvector(params.pk_no_rows());
+   auto pk_slicer = BufferSlicer(m_mat_bytes);
+
+   for(size_t i = 0; i < params.pk_no_rows(); ++i) {
+      auto pk_current_bytes = pk_slicer.take(params.pk_row_size_bytes());
+      auto row = secure_bitvector(pk_current_bytes, params.n() - params.pk_no_rows());
+      row &= e_T;
+      s.at(i) = s.at(i) ^ row.has_odd_hamming_weight();
+   }
+
+   BOTAN_ASSERT_NOMSG(pk_slicer.empty());
+   return s.as<Code_Word>();
+}


This produces quite a few copies and allocations. If that's in the implementation's hot path, we should probably want to have another look. Here's which (I believe cause allocations/copies):

.subvector() always creates a new bitvector and copies the content into a newly allocated buffer,

the c'tor of bitvector copies the passed-in buffer (particularly pk_current_bytes

.as<> produces a copy of the bitvector with a new type

For (3): This could already be the right type using e.subvector<Code_Word>(), no?

Some context: This function is called once per encapsulation. Also, e and s are pretty small, while m_mat_bytes is gigantic. Therefore, 1) and 3) are not critical; I'll apply your suggestion for 3) anyway. I don't know how we can prevent 2) without having something like a bitvector view or dropping the bitvector altogether.

I think I'll have a timeboxed look into a bitvector that doesn't own its underlying storage. Perhaps that isn't too hard to achieve, especially when we can limit it to byte-aligned subvectors.

reneme

Done with a first pass on the implementation. Don't get put off by the number of comments. Most are just C++ style nits and smaller programming suggestions.

That's really good work! 😃

reneme · 2024-01-30T07:34:29Z

src/lib/pubkey/classic_mceliece/cmce_parameter_set.h

+   /// Reduced instances for side channel analysis (Self-created test instance with
+   /// m=8, n=128, t=8, f(z)=z^8+z^7+z^2+z+1, F(y)=y^8+y^4+y^3+y^2+1)
+   /// Minimal instance without semi-systematic matrix creation and no plaintext confirmation
+   test,
+   /// Minimal instance with semi-systematic matrix creation and no plaintext confirmation
+   testf,
+   /// Minimal instance without semi-systematic matrix creation and with plaintext confirmation
+   testpc,
+   /// Minimal instance with semi-systematic matrix creation and with plaintext confirmation
+   testpcf


TODO: remove before merging

src/lib/pubkey/classic_mceliece/cmce_parameter_set.h

reneme · 2024-01-30T07:39:16Z

src/lib/pubkey/classic_mceliece/cmce_parameters.h

+/**
+ * @returns ceil(n/d)
+ * TODO: Remove once LMS is merged
+ */
+constexpr size_t ceil_div(size_t n, size_t d) {
+   return (n + d - 1) / d;
+}


Re-evaluate before merging this.

src/lib/pubkey/classic_mceliece/cmce_parameters.h

src/lib/pubkey/classic_mceliece/cmce_poly.cpp

src/lib/pubkey/classic_mceliece/info.txt

src/lib/utils/bit_ops.h

src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp

FAlbertDev · 2024-02-05T14:48:10Z

Thanks a lot for your extensive review, @reneme! I addressed your suggestions and am optimistic that this PR is ready to drop its Draft status 🎉. Note that some suggestions that depend on other PRs are still open, which are not critical, though. Also, note that an extensive side-channel analysis is still in progress.

aewag · 2024-02-10T22:22:15Z

src/lib/utils/bitvector.h

+            constexpr bitref& operator^=(bool other) noexcept { return assign(this->is_set() ^ other); }
+
+         private:
+            constexpr bitref& assign(bool bit) noexcept { return (bit) ? set() : unset(); }


As discussed, DATA identified this line as a leakage during our SCA review.

Due to the ? operator, a control-flow difference is observed based on the boolean input bit variable.

The assign() routine is used within the push_back() routine, which in turn is used within the decode() routine. This may allow an adversary to observe the error vector e and, hence recover the shared secret.

We suggest to perform both the set() and unset() functions with the input as a mask, for example:

private: constexpr bitref& assign(bool bit) noexcept { const block_type assign_mask = 0 - static_cast<block_type>(bit); this->m_block |= (this->m_mask & assign_mask); this->m_block &= ~(this->m_mask & ~assign_mask); return *this; }

In our case, this results in the following instructions - without any conditional branch based on the input:

[ ... ] const block_type assign_mask = 0 - static_cast<block_type>(bit); 41d397: 41 f7 dc neg %r12d [ ... ] this->m_block |= (this->m_mask & assign_mask); 41d3ab: 44 89 e1 mov %r12d,%ecx 41d3ae: 21 c1 and %eax,%ecx this->m_block &= ~(this->m_mask & ~assign_mask); 41d3b0: f7 d0 not %eax this->m_block |= (this->m_mask & assign_mask); 41d3b2: 0a 0a or (%rdx),%cl this->m_block &= ~(this->m_mask & ~assign_mask); 41d3b4: 44 09 e0 or %r12d,%eax 41d3b7: 21 c8 and %ecx,%eax [ ... ]

Thanks for your analysis and your report! I applied your fix using Botan's constant-time helper class (7a0fe59)

atreiber94 · 2024-02-29T13:26:53Z

src/lib/utils/bitvector.h

@@ -1,6 +1,8 @@
 /*
 * An abstraction for an arbitrarily large bitvector that can


@reneme the code still contains some artefacts for varying the underlying data type that we'd ideally want to get rid of.

FAlbertDev · 2024-05-27T14:57:43Z

@FAlbertDev This needs another rebase. Also, with #3985 merged, it'll need to implement the new method Public_Key::raw_public_key_bits() as well as the method PK_Key_Generation_Test::public_key_from_raw() in the CMCE_Generic_Keygen_Tests test case.

Done! @reneme Do you still want to reduce the test times further for the --run-long-test? Without a --nightly test CLI option or something similar, I see no elegant way to do so without cutting out KATs. I don't know if we want to introduce multiple layers of long tests, though.

randombit · 2024-06-12T23:03:23Z

Looks like another rebase is required, have not investigate the reason

randombit · 2024-06-12T23:04:48Z

Re the long tests we should probably just go ahead with a --nightly or some similar option. Doesn't have to happen in this PR.

FAlbertDev · 2024-06-13T06:49:42Z

Rebased to master.

FAlbertDev · 2024-06-13T06:54:44Z

Cleaned-up history.

randombit

Just a partial review I have yet to get into the meat of the implementation

randombit · 2024-06-26T01:09:08Z

src/lib/pubkey/classic_mceliece/cmce.h

+ * - Relatively slow key generation
+ * - Algorithm is complex and hard to implement side-channel resistant
+ */
+class BOTAN_PUBLIC_API(3, 4) Classic_McEliece_PublicKey : public virtual Public_Key {


Should bump this to match 3,5 (optimistically)

randombit · 2024-06-26T01:10:19Z

src/lib/pubkey/classic_mceliece/cmce.h

+
+   protected:
+      std::shared_ptr<Classic_McEliece_PublicKeyInternal>
+         m_public;  // NOLINT(misc-non-private-member-variables-in-classes)


Readability nit I think this would be better with // NOLINTNEXTLINE so the declaration itself isn't split

randombit · 2024-06-26T01:10:37Z

src/lib/pubkey/classic_mceliece/cmce.h

+BOTAN_DIAGNOSTIC_PUSH
+BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE
+
+class BOTAN_PUBLIC_API(3, 4) Classic_McEliece_PrivateKey final : public virtual Classic_McEliece_PublicKey,


randombit · 2024-06-26T01:17:02Z

src/lib/pubkey/classic_mceliece/cmce_gf.cpp

+
+namespace {
+// Only for moduli 0b0010000000011011 and 0b0001000000001001
+inline CmceGfElem internal_reduce(uint32_t x, CmceGfMod mod) {


Would it make sense for mod to be a compile-time template parameter instead?

Alternately, if CmceGfMod could only take on these specific values we could be assured that unexpected inputs could not occur.

randombit · 2024-06-26T01:18:09Z

src/lib/pubkey/classic_mceliece/cmce_gf.cpp

+}
+
+Classic_McEliece_GF Classic_McEliece_GF::inv() const {
+   // Compute the inverse using fermat's little theorem: a^(q-1) = 1 => a^(q-2) = a^-1


src/lib/utils/bitvector.h

randombit · 2024-06-26T01:33:34Z

src/lib/utils/bitvector.h

+      /**
+       * Renders this bitvector into a sequence of "0"s and "1"s.
+       */
+      std::string to_string() const {


When would this be useful? If just for tests/debugging maybe better to define it elsewhere

That's mostly meant for debugging, yes. Where would you prefer to have this implemented?
Why is that bad to have as a member? It's an internal class, after all.

I added a note to the docstring for now.

src/lib/utils/bitvector.h

randombit · 2024-06-26T01:42:07Z

src/lib/utils/bitvector.h

+      /**
+       * @returns true iff no bit is set
+       */
+      bool none() const {


This convention of foo plus ct_foo seems very easy to make a mistake and call foo in the wrong context and introduce a side channel.

In newer code (eg in pcurves) I've moved to foo is the constant time implementation, and if a variable time foo is required this is explicit as eg vartime_foo or foo_vartime. The explicit vartime jumps out at the reader and it's much harder to accidentally be variable time when not intended.

I now applied this convention to none(), all(), any() and equals() and removed the ct_ prefix from hamming_weight() and has_odd_hamming_weight() (without providing a ct-alternative). All other (non re-allocating) methods should be CT (review appreciated for this statement).

randombit · 2024-06-26T01:43:58Z

src/lib/utils/bitvector.h

+            template <std::unsigned_integral BlockT>
+               requires(is_byte_aligned())
+            size_t is_memory_aligned_to() const {
+               void* ptr = const_cast<void*>(static_cast<const void*>(m_source.as_byte_span().data() + read_bytepos()));


Why the const_cast here? Probably worth a comment

std::align is treating ptr as an out-param (taking it as void*& 😨). The data pointed to by ptr isn't changed, but it re-sets ptr to the next aligned address. I changed the order of operations a little and added a comment.

reneme · 2024-06-28T14:14:33Z

Rebased to master, after #4154 got merged.

randombit · 2024-06-29T11:25:56Z

src/tests/test_utils_bitvector.cpp

+
+std::vector<Test::Result> test_bitvector_bitwise_accessors() {
+   return {
+      Botan_Tests::CHECK("default constructed bitvector",


Botan_Tests:: prefix is unnecessary and causes more indentation than would otherwise be used

randombit · 2024-06-29T11:31:02Z

src/tests/test_utils_bitvector.cpp

+                            result.confirm("1", bv.at(1));
+                            result.confirm("2", bv.at(2));
+                            result.confirm("3", bv.at(3));
+                            result.confirm("4", !bv.at(4));


I feel like this test, and many of the others here, could usefully be randomized.

Choose a random bit vector length n

Choose a hamming weight h between 0 and n

Choose h of n indexes randomly

Iterate over n verify that all are either set or unset appropriately

I don't insist on it I guess but it just feels odd to me to test this one situation (set 1,2,3 otherwise unset) especially when it skips edge cases like the topmost bit. Currently (Lines of code) / (Signal of things working correctly if this test passes) feels quite high.

randombit · 2024-06-29T11:32:36Z

src/tests/test_utils.cpp

+      template <typename T>
+      auto random_pc(Test::Result& result) {
+         std::array<uint8_t, sizeof(T)> buf;
+         Test::rng().randomize(buf);


This can use random_array from #4152

- constant time conditional swap with mask - floor_log2 Co-Authored-By: Amos Treiber <[email protected]>

This is an implementation of the Classic McEliece KEM according to the NIST Round 4 submission and the ISO draft 20230419. Co-Authored-By: Amos Treiber <[email protected]>

Co-Authored-By: Jack Lloyd <[email protected]>

FAlbertDev · 2024-07-12T13:12:58Z

src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp

+/**
+* @brief Create permutation pi as in (Section 8.2, Step 3).
+*/
+CmcePermutation create_pi(std::span<const uint32_t> a) {
+   auto a_pi_zipped = enumerate(a);
+   CMCE_CT::bitonic_sort_pair(std::span(a_pi_zipped));
+
+   CmcePermutation pi_sorted;
+   std::tie(a, pi_sorted.get()) = unzip(std::span(a_pi_zipped));
+
+   return pi_sorted;
+}


Reminder to me: This method is messed up. We want to return the sorted a values too, otherwise the std::adjacent_find will not find repetitions. Also add a regression test for this.

Good catch! I just saw we used to have secure_vector<uint32_t>& a as input

FAlbertDev · 2024-07-12T15:07:06Z

src/lib/pubkey/classic_mceliece/cmce_matrix.cpp

+      pivot_indices.at(row_idx) = count_lsb_zeros(row_acc);
+
+      // Add subsequent rows to the current row, until the pivot
+      // bit is set.
+      for(size_t next_row = row_idx + 1; next_row < Classic_McEliece_Parameters::mu(); ++next_row) {
+         sub_mat.at(row_idx).ct_conditional_xor(!sub_mat.at(row_idx).at(pivot_indices.at(row_idx)).as_choice(),
+                                                sub_mat.at(next_row));
+      }


Note to future me: This seems to be a side-channel. (conditional memory access based on pivot_idices.at(row_idx))

Co-Authored-By: Fabian Albert <[email protected]>

This allows extracting subvectors of the bitvector as unsigned integral bit masks.

... introduced in randombit#4170

reneme added this to the Botan 3.4.0 milestone Jan 11, 2024

FAlbertDev force-pushed the pqc/classic_mceliece branch 4 times, most recently from f73829f to e505390 Compare January 19, 2024 14:53

atreiber94 force-pushed the pqc/classic_mceliece branch 2 times, most recently from 04b9314 to 190261d Compare January 19, 2024 15:59

reneme self-requested a review January 22, 2024 09:33

This comment was marked as resolved.

Sign in to view

reneme force-pushed the pqc/classic_mceliece branch 6 times, most recently from 7662592 to 2a3e60f Compare January 23, 2024 15:55

This comment was marked as resolved.

Sign in to view

reneme reviewed Jan 25, 2024

View reviewed changes

src/lib/utils/strong_type.h Outdated Show resolved Hide resolved

reneme reviewed Jan 25, 2024

View reviewed changes

reneme reviewed Jan 29, 2024

View reviewed changes

reneme requested changes Jan 30, 2024

View reviewed changes

github-advanced-security bot found potential problems Feb 2, 2024

View reviewed changes

src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp Fixed Show resolved Hide resolved

FAlbertDev marked this pull request as ready for review February 5, 2024 14:49

aewag reviewed Feb 10, 2024

View reviewed changes

This was referenced Feb 14, 2024

Classic McEliece Cryptodoc sehlen-bsi/botan-docs#186

Open

Classic McEliece Testspecification sehlen-bsi/botan-docs#187

Open

atreiber94 reviewed Feb 29, 2024

View reviewed changes

FAlbertDev force-pushed the pqc/classic_mceliece branch from 5f0efcc to 54395c2 Compare March 8, 2024 13:24

FAlbertDev force-pushed the pqc/classic_mceliece branch 2 times, most recently from 393f698 to 8804b64 Compare May 27, 2024 13:46

reneme mentioned this pull request May 29, 2024

Skip the Frodo KAT tests under valgrind and arm32-qemu #4081

Merged

FAlbertDev force-pushed the pqc/classic_mceliece branch from 0dcbeac to 50474fa Compare June 13, 2024 06:48

FAlbertDev force-pushed the pqc/classic_mceliece branch from 50474fa to 229b96f Compare June 13, 2024 06:54

FAlbertDev force-pushed the pqc/classic_mceliece branch from afcd07a to 51b4ec5 Compare June 13, 2024 09:42

FAlbertDev requested a review from randombit June 25, 2024 13:17

randombit reviewed Jun 26, 2024

View reviewed changes

reneme mentioned this pull request Jun 26, 2024

Add EC_Scalar and EC_AffinePoint types #4042

Merged

reneme force-pushed the pqc/classic_mceliece branch from bd801f2 to e0dc6cc Compare June 28, 2024 14:13

randombit modified the milestones: Botan 3.5.0, Botan 3.6.0 Jul 6, 2024

randombit reviewed Jul 6, 2024

View reviewed changes

FAlbertDev and others added 4 commits July 12, 2024 14:40

Utility functions for Classic McEliece

daf2ae6

- constant time conditional swap with mask - floor_log2 Co-Authored-By: Amos Treiber <[email protected]>

bitvector<> with basic functionality

eaf1c00

Classic McEliece implementation

f679c6a

This is an implementation of the Classic McEliece KEM according to the NIST Round 4 submission and the ISO draft 20230419. Co-Authored-By: Amos Treiber <[email protected]>

Review comments on bitvector<>

5171cc3

Co-Authored-By: Jack Lloyd <[email protected]>

FAlbertDev commented Jul 12, 2024

View reviewed changes

add ct_reverse_bits<>

60bd9fb

Co-Authored-By: Fabian Albert <[email protected]>

reneme force-pushed the pqc/classic_mceliece branch from e0dc6cc to 60bd9fb Compare July 12, 2024 15:19

reneme mentioned this pull request Jul 16, 2024

Botan: add version 3.5.0 conan-io/conan-center-index#24638

Open

3 tasks

reneme added 2 commits July 16, 2024 16:51

add bitvector<>::subvector<uintX_t>()

afcabfd

This allows extracting subvectors of the bitvector as unsigned integral bit masks.

Refactor: use nwe Strong<> unwrapping helpers

9a6f4df

... introduced in randombit#4170

		@@ -1,6 +1,8 @@
		/*
		* An abstraction for an arbitrarily large bitvector that can

PQC: Classic McEliece #3883

Are you sure you want to change the base?

PQC: Classic McEliece #3883

Conversation

FAlbertDev commented Jan 11, 2024 • edited Loading

TODO Tracker

coveralls commented Jan 11, 2024 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

reneme left a comment

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

reneme 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

reneme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FAlbertDev commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FAlbertDev commented May 27, 2024

randombit commented Jun 12, 2024

randombit commented Jun 12, 2024

FAlbertDev commented Jun 13, 2024

FAlbertDev commented Jun 13, 2024

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

reneme Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

reneme commented Jun 28, 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

FAlbertDev commented Jan 11, 2024 •

edited

Loading

coveralls commented Jan 11, 2024 •

edited

Loading

reneme Jun 27, 2024 •

edited

Loading