Add library for compile time instantiation of elliptic curves #3979

randombit · 2024-04-06T10:24:57Z

This new approach has a lot of benefits. It avoids the side channels that plague anything based on BigInt (or any general purpose arbitrary length integer type), it also avoids almost all heap allocations and inlines nicely.

Performance advantage depends on curve and compiler. For smaller curves (eg secp256r1) ECDSA seems be almost twice as fast. For larger such as secp521r1 the advantage is more like 40%. Clang seems to be able to optimize everything much better than GCC here, for reasons I haven't been able to nail down.

Performance can be improved significantly from here; besides P-521 we're not taking advantage of specialized reductions possible with many primes, nor is there a system to express the precomputed addition chains used for inversions. The BigInt based code makes use of both of these already.

One disadvantage is that the code size is quite significant. The worst overhead is actually the symbol names. I've applied various tricks to reduce these but still over half the size of math_pcurves.o is (per Google's bloaty tool) symbols. I'd like to reduce these in the future, but I don't think it's a blocker to merge.

reneme · 2024-04-06T10:58:33Z

Awesome! I'll certainly have a look. Not before tomorrow, though.

randombit · 2024-04-06T11:08:31Z

No rush! This is certainly not going into 3.4 so there is plenty of time.

Most interesting thing about the sea of red in CI - GCC 11.4 on x86 fails with constexpr timeout, while same version of GCC on say Aarch64 or S390 accepts. So constexpr limits are architecture dependent [*] :( and thus intrinsically flaky since you can be near some limit without knowing it

[*] And also version dependent since GCC 13 on my machine is fine with the code without increasing the constexpr limit.

randombit · 2024-04-06T14:02:22Z

Clang seems to have a nasty bug where std::is_constant_evaluated returns false incorrectly in constexpr context, then it rejects the code because we used asm or volatile in constexpr context in the !std::is_constant_evaluated branch 😡

It works fine for me in Clang 17 on my machines so I'm assuming this was a Clang bug that was fixed subsequently. I'd vote for just bumping the Clang minimum version - that's an absolutely insane bug and nearly impossible to work around - but unfortunately the version in Android NDK also has the bug and we are stuck with that version at least until the next NDK release.

coveralls · 2024-04-06T14:15:06Z

coverage: 91.769% (-0.1%) from 91.864%
when pulling c2f91bd on jack/pcurves
into 7be1498 on master.

randombit · 2024-04-06T14:29:03Z

Downside of this approach is that (due to name mangling being somewhat horribly thought out for this case) object sizes are really big. Even with just 3 curves, on my machine the object file is one of the largest from the whole library, that will get a lot worse with 27. And compile times may prove untenable.

I'm sure 90% of this can be salvaged but I suspect in the end the string based instantiation won't work in practice 😭 which is a shame since it's quite pretty imo

randombit · 2024-04-06T14:32:39Z

Clang bug might be llvm/llvm-project#55638

randombit · 2024-04-06T14:36:27Z

This error

 call to consteval function 'Botan::p_minus<(unsigned char)'\x01', unsigned long, 4UL>' is not a constant expression

Looks like llvm/llvm-project#51182

randombit · 2024-04-06T15:27:56Z

I can repro the std::if_constant_evaluated problem using Clang 16 on my machine so it looks to be a bug fixed in Clang 17, though I don't see anything that looks relevant in the release notes.

reneme

Interesting! 😄

I merely skimmed through the changes and left a few suggestions and comments here and there. No thorough review whatsoever.

reneme · 2024-04-08T06:56:21Z

src/lib/pubkey/pcurves/pcurves_util.h

+*/
+
+#ifndef BOTAN_PCURVES_UTIL_H_
+#define BOTAN_PCURVES_UTIL_H_


General comment for this file (and perhaps other places):

I'd suggest to use std::span<const W, N> instead of std::array<>& for the parameters of those utilities. No need to restrict those functions to arrays, IMO. And a static-length span should provide the same guarantees and optimization opportunities as an array, no?

Main problem is for some technical reason I'm not bothering to learn the details of, C++ can't deduce the conversion from a statically sized array to a statically sized span. https://stackoverflow.com/questions/70983595

This can be worked around with some additional noise but the additional flexibility doesn't buy us anything in this context so I'd rather keep things as simple as possible.

src/lib/pubkey/pcurves/pcurves_util.h

src/lib/pubkey/pcurves/pcurves_impl.h

reneme · 2024-04-08T07:28:51Z

src/lib/pubkey/pcurves/pcurves_impl.h

+         auto v = bigint_monty_redc(m_val, Self::P, Self::P_dash);
+         std::reverse(v.begin(), v.end());
+         auto bytes = store_be(v);
+
+         if constexpr(Self::BYTES == Self::N * WordInfo<W>::bytes) {
+            return bytes;
+         } else {
+            // Remove leading zero bytes
+            const size_t extra = Self::N * WordInfo<W>::bytes - Self::BYTES;
+            std::array<uint8_t, Self::BYTES> out;
+            copy_mem(out.data(), &bytes[extra], Self::BYTES);
+            return out;
+         }


Perhaps we could get away without the if constexpr and the potential memory copy entirely?

Suggested change

auto v = bigint_monty_redc(m_val, Self::P, Self::P_dash);

std::reverse(v.begin(), v.end());

auto bytes = store_be(v);

if constexpr(Self::BYTES == Self::N * WordInfo<W>::bytes) {

return bytes;

} else {

// Remove leading zero bytes

const size_t extra = Self::N * WordInfo<W>::bytes - Self::BYTES;

std::array<uint8_t, Self::BYTES> out;

copy_mem(out.data(), &bytes[extra], Self::BYTES);

return out;

}

auto v = bigint_monty_redc(m_val, Self::P, Self::P_dash);

std::reverse(v.begin(), v.end());

std::array<uint8_t, Self::BYTES> out = {0};

static_assert(Self::N * WordInfo<W>::bytes >= Self::BYTES); // is this guranteed somehow anyway?

const size_t zero_padding_offset = Self::N * WordInfo<W>::bytes - Self::BYTES;

store_be(std::span{out}.template subspan<zero_padding_offset>(), v);

return out;

... this is untested. Just a sketch of the idea. store_be with a statically-sized out-param will static_assert that the byte buffer matches the input range exactly.

src/lib/pubkey/pcurves/pcurves_impl.h

reneme · 2024-04-08T07:45:49Z

src/lib/pubkey/pcurves/pcurves_impl.h

+      void conditional_assign(bool cond, const Self& pt) {
+         m_x.conditional_assign(cond, pt.x());
+         m_y.conditional_assign(cond, pt.y());
+         m_z.conditional_assign(cond, pt.z());
+      }


I'm assuming this is needed for a constant-time implementation? So far, I saw it as an unspoken rule that all methods that are "supposed to be constant-time" are prefixed with ct_.... I found this a helpful convention, that might be applicable here as well.

In this code I'm going for the opposite convention namely that everything is constant time by default and anything that is intentionally variable time has a _vartime suffix.

(There are some current exceptions eg in the point multiplication that will be fixed before merging.)

randombit · 2024-04-08T15:43:09Z

GCC 11 miscompilation, excellent

reneme · 2024-06-04T15:23:23Z

While reviewing the multiplication table helpers, we introduced a few refactorings and added some comments for better comprehension (in #4095): 030c919

reneme

I like this a lot! Thanks for bearing with the load of comments we've had about that. There are certainly some more nits to be found, but I feel its more efficient if we address them in follow-ups.

randombit · 2024-06-05T11:31:47Z

👍 Thanks for the reviews! Some of your review comments still seem worth fixing here, also I need to clean up this dumpster fire of a commit history before merge. I'd also like #4096 to merge prior to this landing so we have some assurance wrt constant time, just in case more substantial reworking is required.

reneme

Thanks for the added documentation and the co-authorship! 🤩

reneme · 2024-06-07T07:16:28Z

src/lib/math/pcurves/pcurves_impl.h

+* are set then add G or H resp. If both bits are set, add GH.
+*/
+template <typename C, size_t W>
+class WindowedMul2Table final {


Fabian and I had done a refactoring on this, here: 030c919

I don't remember receiving feedback about this commit from you. Just making sure it doesn't fall through the cracks.
At this point it might not apply neatly anymore, though. I'll rebase later.

Rebased: b6e847e

Thanks for the rebase. I did see the commit but still need to think that one over.

src/lib/math/pcurves/pcurves_impl.h

reneme · 2024-06-09T14:20:26Z

src/lib/math/pcurves/pcurves_brainpool256r1/info.txt

+<module_info>
+name -> "PCurve brainpool256r1"
+brief -> "brainpool256r1"
+type -> "Internal"


Not 100% sure, but type -> "Internal" hides the module from the application user. I.e. they would be barred from enabling/disabling it via ./configure.py. 😞

Seems to work

$ ./configure.py --disable-modules=pcurves_secp256k1 ... INFO: Skipping (disabled by user): pcurves_secp256k1 ...

What doesn't work, however:

$ ./configure.py --minimized-build --enable-modules=pcurves_secp256k1 ... ERROR: Module 'pcurves_secp256r1' is meant for internal use only ...

... that's inconsistent with the --disable-modules switch. 😓 Let's tackle that independently.

I remember that I deliberately set up the internal modules that way:

Internal modules behave essentially like Public modules, but the library user cannot actively enable/disable them. They are supposed to contain code that shouldn't be interacted with directly by the library user. Frankly, I don't know of any such module apart from the recently introduced kyber_common.

... this was apparently changed later: #3489 (for --disable-modules).

I don't have a hard objection for allowing the user to interact with internal modules, though, my initial model was that they aren't visible to the user altogether.

initial model was that they aren't visible to the user altogether.

The problem here is there are two different notions of internal we could use

Something internal, which if enabled the user can't see (eg in a header) but which affects behavior (pcurves_secp256r1 being enabled or not changes, for example, if hash2curve works for P-256)

Something truly internal which only exists to satisfy some other dependency and otherwise does not have a visible change in behavior (if KMAC is not included, then the existence or absence of cshake_xof changes nothing, so there is no reason to allow the user to enable it)

Maybe the answer here is that none of these modules should be "Internal" since their absence does introduce user visible changes?

reneme · 2024-06-09T14:21:04Z

src/lib/math/pcurves/pcurves_brainpool256r1/info.txt

+
+<module_info>
+name -> "PCurve brainpool256r1"
+brief -> "brainpool256r1"


Just in case: the brief statement can be omitted entirely.

reneme · 2024-06-09T14:22:13Z

src/lib/math/pcurves/pcurves_secp521r1/pcurves_secp521r1.cpp

+template <typename Params>
+class P521Rep final {


Nice! I like how this moved closer to the actual usage.

reneme · 2024-06-09T14:24:45Z

src/lib/math/pcurves/pcurves.cpp

+std::vector<PrimeOrderCurveId> PrimeOrderCurveId::all() {
+   return {
+      PrimeOrderCurveId::secp256r1,
+      PrimeOrderCurveId::secp384r1,
+      PrimeOrderCurveId::secp521r1,
+      PrimeOrderCurveId::secp256k1,
+      PrimeOrderCurveId::brainpool256r1,
+      PrimeOrderCurveId::brainpool384r1,
+      PrimeOrderCurveId::brainpool512r1,
+      PrimeOrderCurveId::frp256v1,
+      PrimeOrderCurveId::sm2p256v1,
+   };
+}


Perhaps this should only report the curves that are actually available in this build? I'm a bit on the fence about it, given that the curve-specific factories do exist regardless and just return a default value.

Yeah, I'm not sure what's more helpful. The main usage of this is for the tests though; I don't really see why we would ever call this within the library. Maybe it's best to just move this list to the tests.

Also the tests still assume that all the curve are always available

auto curve = Botan::PCurve::PrimeOrderCurve::from_id(id); if(!curve) { result.test_failure("ID exists but curve does not"); continue; }

so I'll have to fix that regardless.

Mhh, I think there's value in having this closer to the implementation, rather than moving it to the tests where it might get missed when adding curves. Perhaps just leave a docstring of warning?

src/lib/math/pcurves/pcurves.cpp

This is designed from the start to avoid side channels and for good performance. On most benchmarks it is approximately 2x faster. Co-Authored-By: René Meusel <[email protected]> Co-Authored-By: Fabian Albert <[email protected]>

randombit · 2024-06-11T00:57:18Z

OK there is probably a lot more that could/should be done here but I'm going to go ahead and merge

Late in the development of #3979 the pcurves were split one per file. This change accidentally made it so that it was possible for two distinct PrimeOrderCurve instances were created for a particular curve. In particular this caused the basepoint multiplication table to be computed twice for each curve.

randombit added this to the Botan 3.5.0 milestone Apr 6, 2024

randombit requested a review from reneme April 6, 2024 10:24

randombit force-pushed the jack/pcurves branch from e7b71fc to 2f7186e Compare April 6, 2024 13:47

randombit force-pushed the jack/pcurves branch from bcf81fe to 344cdd6 Compare April 6, 2024 14:43

randombit force-pushed the jack/pcurves branch 4 times, most recently from 3496464 to cd40575 Compare April 7, 2024 13:17

reneme reviewed Apr 8, 2024

View reviewed changes

randombit force-pushed the jack/pcurves branch 12 times, most recently from 2feb181 to 3bde391 Compare April 14, 2024 09:51

reneme approved these changes Jun 5, 2024

View reviewed changes

randombit mentioned this pull request Jun 6, 2024

Add CT::value_barrier #4096

Merged

randombit force-pushed the jack/pcurves branch 3 times, most recently from e98e9d0 to 0ef936b Compare June 7, 2024 02:24

reneme approved these changes Jun 7, 2024

View reviewed changes

randombit added 2 commits June 7, 2024 18:33

Add StringLiteral helper

a4ebc0d

Various mp library improvements

9b1b666

randombit force-pushed the jack/pcurves branch 2 times, most recently from 45e42fa to 256adc3 Compare June 7, 2024 23:50

reneme reviewed Jun 9, 2024

View reviewed changes

randombit force-pushed the jack/pcurves branch from 2ea78a4 to 26c7c04 Compare June 10, 2024 13:10

randombit and others added 6 commits June 10, 2024 09:11

Add new library for elliptic curve arithmetic

df42f6b

This is designed from the start to avoid side channels and for good performance. On most benchmarks it is approximately 2x faster. Co-Authored-By: René Meusel <[email protected]> Co-Authored-By: Fabian Albert <[email protected]>

Add test for pcurves library

f15e324

Use pcurves for hash to curve

97056ed

Add pcurves benchmarks

1eeede5

Track valgrind origins

2988574

Split up the curves one per module

c2f91bd

randombit force-pushed the jack/pcurves branch from 26c7c04 to c2f91bd Compare June 10, 2024 13:11

randombit merged commit 856174e into master Jun 11, 2024
42 checks passed

randombit deleted the jack/pcurves branch June 11, 2024 00:57

reneme mentioned this pull request Jun 13, 2024

Double-check crypto documentation of ECC sehlen-bsi/botan-docs#219

Merged

randombit mentioned this pull request Jun 17, 2024

Avoid duplicating basepoint multiplication tables #4136

Merged

reneme mentioned this pull request Jun 25, 2024

Bridge pcurves into the main elliptic curve arithmetic #4143

Merged

This was referenced Jul 31, 2024

Remove internal annotation from pcurves modules #4279

Merged

Doc: clarify definition of 'Internal' modules #4281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add library for compile time instantiation of elliptic curves #3979

Add library for compile time instantiation of elliptic curves #3979

randombit commented Apr 6, 2024 •

edited

Loading

reneme commented Apr 6, 2024

randombit commented Apr 6, 2024 •

edited

Loading

randombit commented Apr 6, 2024

coveralls commented Apr 6, 2024 •

edited

Loading

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

reneme left a comment

reneme Apr 8, 2024

randombit Apr 14, 2024

reneme Apr 8, 2024

reneme Apr 8, 2024

randombit Apr 8, 2024

randombit commented Apr 8, 2024

reneme commented Jun 4, 2024

reneme left a comment

randombit commented Jun 5, 2024

reneme left a comment

reneme Jun 7, 2024

reneme Jun 7, 2024

randombit Jun 7, 2024

reneme Jun 9, 2024

randombit Jun 9, 2024

reneme Jun 10, 2024

randombit Jun 10, 2024

reneme Jun 9, 2024

reneme Jun 9, 2024

reneme Jun 9, 2024

randombit Jun 9, 2024

reneme Jun 10, 2024

randombit commented Jun 11, 2024

Add library for compile time instantiation of elliptic curves #3979

Add library for compile time instantiation of elliptic curves #3979

Conversation

randombit commented Apr 6, 2024 • edited Loading

reneme commented Apr 6, 2024

randombit commented Apr 6, 2024 • edited Loading

randombit commented Apr 6, 2024

coveralls commented Apr 6, 2024 • edited Loading

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

randombit commented Apr 6, 2024

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

Choose a reason for hiding this comment

randombit commented Apr 8, 2024

reneme commented Jun 4, 2024

reneme left a comment

Choose a reason for hiding this comment

randombit commented Jun 5, 2024

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

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

randombit commented Jun 11, 2024

randombit commented Apr 6, 2024 •

edited

Loading

randombit commented Apr 6, 2024 •

edited

Loading

coveralls commented Apr 6, 2024 •

edited

Loading