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: 92.281% (+0.5%) from 91.83%
when pulling 0dcbeac on Rohde-Schwarz:pqc/classic_mceliece
into 00e234d 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.

randombit

Have not done a full review yet, leaving some initial comments

randombit · 2024-03-25T09:27:00Z

src/lib/utils/bit_ops.h

+      x = (x + (x >> 4)) & 0xF0F0F0F0F0F0F0F;
+      return (x * 0x101010101010101) >> 56;
+   } else {
+      static_assert(!std::unsigned_integral<T>, "T is not a suitable unsigned integer value");


I don’t understand this static_assert - shouldn’t the concept check on T already prevent this? It feels like this is intended to catch the case where T > 64 bits but that doesn’t necesarily apply if the compiler considers a 128 bit integer type to be unsigned_integral (maybe C++20 prohibits this, idk)

Perhaps instead starting the function with

static_assert(sizeof(T) <= 8, “T is not …”)

which anyway seems like a more clear statement of requirement?

I think you are right. @reneme?

src/tests/runner/test_runner.cpp

randombit · 2024-03-25T09:40:40Z

src/lib/pubkey/classic_mceliece/cmce.cpp

+}
+
+size_t Classic_McEliece_PublicKey::key_length() const {
+   return m_public->matrix().bytes().size();


a) Is it really necessary to encode the matrix (with memory allocation etc) just to determine the key length

b) Is the byte size of the matrix really the best value to return here? Returning the integer encoding of {n}{t} would be just as meaningul (ie, an arbitrary integer that goes upwards as the key gets stronger) while allowing some plausible method of mapping it back to a parameter set.

a) The matrix object stores the matrix bytes as a member. The method bytes only returns a const reference to these bytes. No new memory is allocated here.
b) Yeah, I got confused by the meaning of the method key length. I assumed it was the size of the public key in bytes without looking into the description. Thanks for mentioning this.
I think the most sensible value is $k = n - mt$ which is the dimension of the goppa code (i.e., there are $2^k$ code words).

src/lib/pubkey/classic_mceliece/cmce_parameter_set.h

src/lib/pubkey/classic_mceliece/cmce.cpp

src/lib/pubkey/classic_mceliece/cmce.h

src/lib/pubkey/classic_mceliece/cmce_field_ordering.cpp

FAlbertDev · 2024-03-26T08:03:10Z

Thanks very much for your initial review 🚀 I'll look into it.

FAlbertDev · 2024-03-26T15:39:12Z

I rebased to master (which removes the --no-stdout flag commit).

reneme · 2024-03-28T13:48:24Z

Lets have another look at the tests. Especially the coverage build time suffers quite a bit from the new tests. Presumably, because it runs an extended test set. This is summing up to a few minutes, though.

FAlbertDev · 2024-04-02T07:34:45Z

Especially, the coverage build time suffers quite a bit from the new tests.

Yeah. --run-long-tests tests all tests with all instances. If you think this is problematic, here are some suggestions to avoid this problem:

We may only want to test some instances in the keygen tests. The correctness of the keys is already covered in the KAT tests; only the Botan interface is tested, which does not differ for various instances. Also, the Keygen test creates two keys instead of one (for KAT tests). This should reduce the total time by around 66%.
Since the private and public keys are the same for pc and non-pc instances, we can try to "reuse" the key and save a keygen operation. This, however, is messy since we do not provide an interface for this reinterpretation, and the generic tests do not support something like this.
We can only test pc instances and a few non-pc instances.

IMHO, running --run-long-tests tests that even take minutes should be allowed. I'm totally fine applying suggestion 1 since this is a low-hanging fruit without losing KATs. I prefer to keep all KAT instances and do not want to mess around as in 2.

FAlbertDev · 2024-04-02T10:51:23Z

Rebased to master + commit with some left-over suggestions.

FAlbertDev · 2024-04-02T13:35:14Z

We may only want to test some instances in the keygen tests. The correctness of the keys is already covered in the KAT tests; only the Botan interface is tested, which does not differ for various instances. Also, the Keygen test creates two keys instead of one (for KAT tests). This should reduce the total time by around 66%.

I applied this and decreased the total time for CMCE long tests from 81 sec to 17 sec.

reneme · 2024-04-02T18:18:38Z

I applied this and decreased the total time for CMCE long tests from 81 sec to 17 sec.

Thanks!

IMHO, running --run-long-tests tests that even take minutes should be allowed.

I do agree, but then we should move away from running such tests for every push (like we actually do in the "sanitizer" and "coverage" builds, currently). If we wanted to have such long-running tests, we should outsource them to a nightly, to keep CI times manageable for interactive use cases.

reneme · 2024-05-24T07:15:29Z

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

- constant time conditional swap with mask - floor_log2 Co-Authored-By: Amos Treiber <amos.treiber@rohde-schwarz.com>

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 <amos.treiber@rohde-schwarz.com>

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.

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

randombit reviewed Mar 25, 2024

View reviewed changes

FAlbertDev mentioned this pull request Mar 26, 2024

Add --no-stdout flag to botan-test #3945

Merged

FAlbertDev force-pushed the pqc/classic_mceliece branch from e431972 to ddd8094 Compare March 26, 2024 15:38

FAlbertDev force-pushed the pqc/classic_mceliece branch 2 times, most recently from 9a5ad0b to f6333fd Compare March 27, 2024 13:10

FAlbertDev force-pushed the pqc/classic_mceliece branch from f6333fd to 3df2804 Compare April 2, 2024 10:50

randombit modified the milestones: Botan 3.4.0, Botan 3.5.0 Apr 8, 2024

reneme assigned FAlbertDev May 24, 2024

FAlbertDev and others added 6 commits May 27, 2024 11:23

Utility functions for Classic McEliece

feabae9

- constant time conditional swap with mask - floor_log2 Co-Authored-By: Amos Treiber <amos.treiber@rohde-schwarz.com>

bitvector<> with basic functionality

39c6c37

Classic McEliece implementation

78f9c2c

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 <amos.treiber@rohde-schwarz.com>

Apply review suggestions

195ac1c

Apply left-over suggestions

8594f14

Faster long keygen tests

7c0b5d8

FAlbertDev force-pushed the pqc/classic_mceliece branch from 8271956 to 393f698 Compare May 27, 2024 09:54

Post-rebase fixes

8804b64

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

FAlbertDev added 2 commits May 27, 2024 15:50

Apply ceil_division of bitops

75af270

Fix PK_Key_Generation_Test for CMCE

0dcbeac

reneme mentioned this pull request May 29, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PQC: Classic McEliece #3883

PQC: Classic McEliece #3883

FAlbertDev commented Jan 11, 2024 •

edited

coveralls commented Jan 11, 2024 •

edited

This comment was marked as resolved.

This comment was marked as resolved.

reneme left a comment

reneme left a comment

reneme left a comment

reneme Jan 26, 2024

reneme Jan 29, 2024

FAlbertDev Feb 2, 2024

reneme Feb 6, 2024

reneme left a comment

reneme Jan 30, 2024

reneme Jan 30, 2024

FAlbertDev commented Feb 5, 2024

aewag Feb 10, 2024

FAlbertDev Feb 12, 2024

atreiber94 Feb 29, 2024

randombit left a comment

randombit Mar 25, 2024

FAlbertDev Mar 26, 2024

randombit Mar 25, 2024

FAlbertDev Mar 26, 2024

FAlbertDev commented Mar 26, 2024

FAlbertDev commented Mar 26, 2024

reneme commented Mar 28, 2024

FAlbertDev commented Apr 2, 2024 •

edited

FAlbertDev commented Apr 2, 2024

FAlbertDev commented Apr 2, 2024 •

edited

reneme commented Apr 2, 2024

reneme commented May 24, 2024

FAlbertDev commented May 27, 2024

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

TODO Tracker

coveralls commented Jan 11, 2024 • edited

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

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

FAlbertDev commented Mar 26, 2024

FAlbertDev commented Mar 26, 2024

reneme commented Mar 28, 2024

FAlbertDev commented Apr 2, 2024 • edited

FAlbertDev commented Apr 2, 2024

FAlbertDev commented Apr 2, 2024 • edited

reneme commented Apr 2, 2024

reneme commented May 24, 2024

FAlbertDev commented May 27, 2024

FAlbertDev commented Jan 11, 2024 •

edited

coveralls commented Jan 11, 2024 •

edited

FAlbertDev commented Apr 2, 2024 •

edited

FAlbertDev commented Apr 2, 2024 •

edited