curve25519: Add arbitrary integer multiplication with MontgomeryPoint::mul_bits_be

[rozbb]

There is occasionally a need 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.

So we define MontgomeryPoint::mul_bits_be, gated behind hazmat, which takes a big-endian representation of an integer and multiplies the point by that integer.

cc @elichai

[elichai]

ACK ed53307 reviewed the code and it seems correct, haven't tested it yet

[rozbb]

@tarcieri ready for review. One thing I wanna make sure of personally is that this hasn't strayed too far from Alg 8 of CS17

[tarcieri]

Is this supposed to be pub?

It looks like it calls itself and infinitely recurses? Is it supposed to call self._mul_bits_be?

If the intent is for this to be pub, I worry about people accidentally discovering it. Feature-gating on hazmat alone isn't a great deterrent since other dependencies can activate the feature. Putting it in the hazmat module would better prevent accidental discovery/misuse IMO (and free up the mul_bits_be name so you don't need a _-prefixed version)

At the very least, it could use a more stringent warning label.

[rozbb]

Oh good points. I agree it'd be best to put it in the hazmat module. Will fix.

[rozbb]

Acutally, this is a question: should this method be feature-gated? There's two main misuses I can think of:

1. If someone used this method instead of mul_clamped. This could leak up to 3 secret bits. Not great.
2. If someone used this method instead of Mul<Scalar> for MontgomeryPoint. This would be fine, actually. In the worst case, this is equivalent to doing the scalar mul. In the best case, this is more correct than the scalar mul, because the point being multiplied might not be in the prime-order subgroup, and so the scalar mul made no sense anyway.

So the question is if case (1) is likely and worth avoiding. I'm not sure. If someone wanted to multiply by some arbitrary bytestring today, they can already do Scalar::from_bytes_mod_order(bytes) * point. If the bytestring is uniform, then this is already equivalent to mul_bits_be with 50% probability.

My vote is to let this function be public, and let it have the documentation note I just added.

[elichai]

I have a wild suggestion, how do people think about a RingElement type that is over the full curve's order? is this too much for that kind of feature?

[tarcieri]

My vote is to let this function be public, and let it have the documentation note I just added.

@rozbb seems okay if it comes with a warning and steers people towards the other options unless they know what they're doing.

Edit: looks like you do already have a note about using mul_clamped, so that's fine

is this too much for that kind of feature?

@elichai might be overkill

[rozbb]

RingElement is overkill imo. We don't define a way to add or multiply two numbers mod 8p, so it'd be a pretty useless ring element. Integer multiplication is broad enough for this use case imo

[elichai]

Do we care that this doesn't enforce how many bits are passed? ie you can pass an empty iter and it will be equivalent to multiplying by zero, or you can pass a longer iterator and compute a "longer" multiplication

[elichai]

Should it use ExactSizeIterator and assert len == 256? that would still allow multiplying by something bigger than the curve's order though

[tarcieri]

I do like how it's infallible now.

Perhaps it could have a debug_assert!? That wouldn't necessarily require ExactSizeIterator, it would just need a sort of postcondition check that it iterated over 256 bits.

[rozbb]

Why limit the function at all? Multiplication by arbitrary integers is always well-defined. If you don't want to reduce then you're free not to