p384: scalar multiplication test vectors

[tarcieri]

Tests scalar multiplication using vectors from:

http://point-at-infinity.org/ecc/nisttv

[tarcieri]

Note: currently failing on the first vector, which is one of the repeated addition of the generator vectors.

I haven't investigated why yet

cc @jedisct1 @brycx

[tarcieri]

It looks like the From<u64> impl for Scalar was bugged and setting the last limb rather than the first.

With that fixed it gets farther but still fails on one of the vectors.

[tarcieri]

Sidebar: it looks like <Scalar as PrimeField>::from_repr isn't properly checking for overflow.

Confirmed this: fiat_p384_scalar_from_bytes states the following precondition:

/// Preconditions:
///   0 ≤ bytes_eval arg1 < m

...so the input scalar needs to be checked to ensure it doesn't overflow the order

[jedisct1]

Why do we need From<u64> at all? That seems like something dangerous to have in a public API. Even internally, there's no specialized code for multiplication with small scalars. The real-world use cases for that are very specific (ex: cofactor clearing, which is irrelevant here) and would be rather addressed by dedicated functions.

The semantics of from_repr() are not very clear. The endianness is documented as "implementation specific", so my interpretation is that it was only meant to be used internally. Looking at this being used in this PR's test vectors got me even more confused about what a "repr" is, if it's supposed to be used by applications and if it is guaranteed to be stable across versions.

[jedisct1]

so the input scalar needs to be checked to ensure it doesn't overflow the order

This is done in from_le_bytes() and thus indirectly everywhere else besides from_repr().

from_repr() can be replaced by a simple call to from_be_bytes().

[tarcieri]

Why do we need From at all? That seems like something dangerous to have in a public API. Even internally, there's no specialized code for multiplication with small scalars. The real-world use cases for that are very specific (ex: cofactor clearing, which is irrelevant here) and would be rather addressed by dedicated functions.

At least in this particular case, it's being exercised by the test suite which is checking the equivalence of repeated doublings of the generator and scalar multiplication by the generator.

I'm not aware of external-facing applications offhand, but regardless, it was easy to fix.

The semantics of from_repr() are not very clear. The endianness is documented as "implementation specific", so my interpretation is that it was only meant to be used internally. Looking at this being used in this PR's test vectors got me even more confused about what a "repr" is, if it's supposed to be used by applications and if it is guaranteed to be stable across versions.

There is some ongoing discussion about this from users of ff and group. See here for example:

zkcrypto/ff#16 (comment)

k256 and p256 use a big endian representation.

This is done in from_le_bytes() and thus indirectly everywhere else besides from_repr().

from_repr() can be replaced by a simple call to from_be_bytes().

This doesn't really uphold the contract of the function, which returns a CtOption.

It's addressed easily enough using e.g. U384 to check for overflow, which can return a Choice.

[tarcieri]

Here's my proposed fix, which makes Scalar consistent with the ScalarCore API in the elliptic-curve crate: #571

This allows Scalar::from_repr to call Scalar::from_be_bytes.

[tarcieri]

@jedisct1 the From<u64> bound is actually a requirement for the PrimeField trait:

https://docs.rs/ff/0.12.0/ff/trait.PrimeField.html

pub trait PrimeField: Field + From<u64> { ... }