Make scalars always reduced #519

rozbb · 2023-03-20T08:58:12Z

This makes unreduced Scalar unrepresentable by removing Scalar::{from_bits, from_bytes_clamped}. As a consequence, we can remove the additional reduction steps in scalar addition and subtraction. Benchmarks on my M1 Macbook Air show runtime diffs of -52% and -67% on scalar addition and subtraction, respectively.

Since clamping is necessary elsewhere, this change exposes new methods MontgomeryPoint::{mul_clamped, mul_base_clamped}, EdwardsPoint::{mul_clamped, mul_base_clamped}, and BasepointTable::mul_base_clamped, which take byte sequences.

This partially addresses #507 (scalar performance) and #514 (scalar arithmetic well-definedness).

The first snippet of #514 points out that it is possible to construct a clamped scalar s and a point P such that s * P and s.reduce() * P are not equal. This change makes this test case impossible to represent, since unreduced scalars are now unrepresentable.

Upstream changes

dalek-cryptography/ed25519-dalek#293

dalek-cryptography/x25519-dalek#120

…alars are now reduced mod l

…nd legacy_compatibility

src/edwards.rs

src/scalar.rs

tarcieri · 2023-03-21T02:42:59Z

I like the API and the direction. Preventing the clamped scalars from leaking as returned values seems like an elegant approach, while covering the important use cases.

rozbb · 2023-03-26T15:41:08Z

@jrose-signal would you mind taking a look at this and verifying it wouldn't break your libraries?

…er supported

Cargo.toml

rozbb · 2023-03-27T09:24:04Z

src/montgomery.rs

@@ -342,6 +370,9 @@ impl<'a, 'b> Mul<&'b Scalar> for &'a MontgomeryPoint {
            W: FieldElement::ONE,
        };

+        // NOTE: The below swap-double-add routine skips the first iteration, i.e., it assumes the
+        // MSB of `scalar` is 0. This is allowed, since it follows from Scalar invariant #1.
+


I noticed that this was the case because the following test fails.

let a_bytes = [0xff; 32]; let s1 = Scalar::from_bytes_mod_order(a_bytes); let s2 = Scalar { bytes: a_bytes }; assert_eq!( s1 * constants::X25519_BASEPOINT, s2 * constants::X25519_BASEPOINT );

That's kinda surprising. There's no NAF or any fancy arithmetic necessary for the Montgomery ladder. Anyways, neither here nor there, because it works for all scalars < 2^255, which is all we care about.

I mean, that's expected for me. One scalar is reduced, the other is not, but Scalar { bytes: a_bytes } would not be valid under Scalar's (new) invariant and can't be constructed outside the crate due to field visibility.

sidebar: I'm curious if Scalar could be made into a newtype for ScalarImpl now.

Right, well it's only surprising because there's nothing inherent about Montgomery multiplication that even requires invariant 1 in the first place. In fact, if you change the below line to

let mut bits = core::iter::once(0).chain(scalar.bits_le().rev());

then these asserts actually pass.

What is ScalarImpl?

jrose-signal · 2023-03-28T19:59:26Z

@jrose-signal would you mind taking a look at this and verifying it wouldn't break your libraries?

Yes, this is fine for us (and I had someone more cryptographer than me confirm). I did find one place we were using Scalar::from_bits directly: XEd25519 signatures. But those Scalars are always multiplied by the Edwards basepoint, so switching to from_bytes_mod_order for an eager reduction is fine (and doesn't seem to cause any significant slowdown).

…mpatibility to Makefile and docsrs flags

src/scalar.rs

rozbb · 2023-03-29T04:21:59Z

@jrose-signal thanks for the quick turnaround! We'll be propagating the API change to ed- and x- soon

elichai · 2023-07-17T10:06:39Z

We currently use the Scalar::from_bits function in order to do full-group (non-clamped) Scalar*Point multiplication via the MontgomeryPoint API,
We need this to be over the whole group + twist and not specifically in the prime subgroup,
Is this something that will still be supported?

(Specifically, this is needed for something similar to Moller04 https://www.bmoeller.de/pdf/pke-pseudo-esorics2004.pdf)

tarcieri · 2023-07-17T15:12:27Z

@elichai would a dedicated API for this, e.g. MontgomeryPoint::mul_unclamped similar to MontgomeryPoint::mul_clamped work for your purposes?

elichai · 2023-07-17T15:13:56Z

@elichai would a dedicated API for this, e.g. MontgomeryPoint::mul_unclamped similar to MontgomeryPoint::mul_clamped work for your purposes?

Yep, that would definitely work

elichai · 2023-07-27T13:58:29Z

@elichai would a dedicated API for this, e.g. MontgomeryPoint::mul_unclamped similar to MontgomeryPoint::mul_clamped work for your purposes?

What do you think about this function also overriding invariant #1? i.e. it will allow multiplying with the top bit on, so it can't assume the first bit is zero as in: https://docs.rs/curve25519-dalek/latest/src/curve25519_dalek/montgomery.rs.html#373-374

tarcieri · 2023-07-27T14:04:39Z

Perhaps it would need to be fallible and return an e.g. CtOption?

elichai · 2023-07-27T14:14:47Z

Perhaps it would need to be fallible and return an e.g. CtOption?

Why fallible? if it's bigger than 8 \cdot \ell ?

tarcieri · 2023-07-27T14:15:54Z

Do you have a different solution in mind for it overriding the invariants?

rozbb · 2023-07-27T14:18:25Z

I think it'd be possible to just extend the iterator to all bits. I'd prefer to not copy the code, so I'll just modify this function and see if we get a performance regression

…l_bits_be` (#555) There is occasionally [a need](#519 (comment)) to multiply a non-prime-order Montgomery point by an integer. There's currently no way to do this, since our only methods are multiplication by `Scalar` (doesn't make sense in the non-prime-order case), and `MontgomeryPoint::mul_base_clamped` clamps the integer before multiplying. This defines `MontgomeryPoint::mul_bits_be`, which takes a big-endian representation of an integer and multiplies the point by that integer. Its usage is not recommended by default, but it is also not so unsafe as to be gated behind a `hazmat` feature.

rozbb added 6 commits March 20, 2023 03:59

Removed Scalar::{from_bits, from_bytes_clamped}; all constructible sc…

bafa0f7

…alars are now reduced mod l

Merge branch 'main' into scalar-always-reduced

a9ffe34

Made Scalar::reduce() not pub; fixed test warning

b2862f2

Added benches for scalar add/sub/mul

9867c6c

Fixed build warning

ef4285f

Fixed doctests

3d16f12

rozbb requested a review from tarcieri March 20, 2023 08:58

rozbb added 2 commits March 20, 2023 14:32

Docs

a299343

Added EdwardsPoint::mul_base_clamped and gated Scalar::from_bits behi…

5d1aae2

…nd legacy_compatibility

tarcieri reviewed Mar 21, 2023

View reviewed changes

src/edwards.rs Outdated Show resolved Hide resolved

tarcieri reviewed Mar 21, 2023

View reviewed changes

src/scalar.rs Outdated Show resolved Hide resolved

tarcieri reviewed Mar 21, 2023

View reviewed changes

src/scalar.rs Outdated Show resolved Hide resolved

rozbb mentioned this pull request Mar 21, 2023

Update to new Scalar API dalek-cryptography/ed25519-dalek#293

Merged

Added unit test for Mul impl on unreduced Scalars

c170939

rozbb mentioned this pull request Mar 21, 2023

Update to new Scalar API dalek-cryptography/x25519-dalek#120

Merged

pinkforest mentioned this pull request Mar 26, 2023

4.0.0 Remaining Breaking Changes #521

Closed

rozbb added 4 commits March 27, 2023 01:40

Added Montgomery::mul_base_clamped; tweaked docs

1934019

Added BasepointTable::mul_base_clamped; did lots of docs

dcd3078

Removed invalid scalar arithmetic test; this functionality is no long…

25ee96c

…er supported

Made clamp_integer() const

be9fc8f

rozbb commented Mar 27, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

rozbb added 2 commits March 27, 2023 05:14

Fixed build; updated readme and changelog

fc86a64

Wibble

1663cb6

rozbb commented Mar 27, 2023

View reviewed changes

rozbb marked this pull request as ready for review March 27, 2023 09:24

Clarify changelog items

742bf60

Added BasepointTable::mul_base_clamped to tests

a7a5257

rozbb added 5 commits March 28, 2023 16:48

Added proper deprecation notice to Scalar::from_bits; added legacy_co…

ec9a5d1

…mpatibility to Makefile and docsrs flags

Docs wibble

90ff2cb

Fixed test CI

a64d72a

Added Scalar::reduce removal to changelog

2854d4e

Changelog wibble

239ffa8

rozbb commented Mar 28, 2023

View reviewed changes

src/scalar.rs Show resolved Hide resolved

tarcieri approved these changes Mar 28, 2023

View reviewed changes

rozbb merged commit f460ae1 into dalek-cryptography:main Mar 28, 2023
13 checks passed

tarcieri mentioned this pull request Mar 30, 2023

Scalar arithmetic is inconsistent with point multiplication most of the time #514

Closed

tarcieri mentioned this pull request May 21, 2023

Poor Scalar performance #507

Closed

rozbb mentioned this pull request Jun 27, 2023

Missing Scalar::from_bits_clamped #537

Closed

rozbb mentioned this pull request Jul 30, 2023

curve25519: Add arbitrary integer multiplication with MontgomeryPoint::mul_bits_be #555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scalars always reduced #519

Make scalars always reduced #519

rozbb commented Mar 20, 2023 •

edited

tarcieri commented Mar 21, 2023 •

edited

rozbb commented Mar 26, 2023

rozbb Mar 27, 2023

tarcieri Mar 28, 2023

rozbb Mar 28, 2023

jrose-signal commented Mar 28, 2023

rozbb commented Mar 29, 2023

elichai commented Jul 17, 2023

tarcieri commented Jul 17, 2023

elichai commented Jul 17, 2023

elichai commented Jul 27, 2023

tarcieri commented Jul 27, 2023

elichai commented Jul 27, 2023

tarcieri commented Jul 27, 2023

rozbb commented Jul 27, 2023

Make scalars always reduced #519

Make scalars always reduced #519

Conversation

rozbb commented Mar 20, 2023 • edited

Upstream changes

tarcieri commented Mar 21, 2023 • edited

rozbb commented Mar 26, 2023

rozbb Mar 27, 2023

Choose a reason for hiding this comment

tarcieri Mar 28, 2023

Choose a reason for hiding this comment

rozbb Mar 28, 2023

Choose a reason for hiding this comment

jrose-signal commented Mar 28, 2023

rozbb commented Mar 29, 2023

elichai commented Jul 17, 2023

tarcieri commented Jul 17, 2023

elichai commented Jul 17, 2023

elichai commented Jul 27, 2023

tarcieri commented Jul 27, 2023

elichai commented Jul 27, 2023

tarcieri commented Jul 27, 2023

rozbb commented Jul 27, 2023

rozbb commented Mar 20, 2023 •

edited

tarcieri commented Mar 21, 2023 •

edited