Issue#1675

[mccleeary-galois]

Closes #1675

This seems to be the root cause bug, but I think there are some other improvements @marsella had in mind as well that we should go ahead and get in.

[marsella]

I haven't done a full audit of the EC stuff but here are some concerns I saw while poking into this bug:

• The ec_negate function has the same bug as ec_sub -- it mis-handles any point whose y-coordinate is zero.
• In ec_mult we deviate from the spec in the way that we iterate over the bits of (d, 3d). The spec initializes the result so that we can skip iterating over the most significant bits of those two values. The implementation does iterate over the MSBs, and based on a first look, I think it doesn't change the result initialization accordingly. Note that this wouldn't be triggered by the test case for PrimeEC::ec_mult can produce incorrect results #1675; I can poke at this a bit more to come up with a case that does fail.
• In ec_mult, I think the error message on L297 is incorrect -- looks like it may have been pasted from a slightly different conditional in a previous iteration.

[RyanGlScott]

The fix here looks quite plausible, although I am admittedly not an expert on elliptic curves. I do have one question about the implementation.

[marsella]

This will fix the reported bug, thank you!

I spent some time looking around in the NIST document with EC algorithms you shared. In that doc, the subtraction algorithm matches what was here previously. I think the missing link is that the doc notes elsewhere that the three coordinates are all field elements, so the mod is implicit.

If we have users who depend on the elliptic curve operations in here, I would recommend making a separate issue to review the full file and bring it closer in line with a reference implementation. The two things I'd focus on are:

1. Making sure there are no other places where BigNums are being used as normal numbers instead of the field elements they are supposed to represent.
2. Reviewing the names and conditions in this file compared to the reference. I noticed a few checks that are either missing or are in different places than I expected, so it might be worth confirming that this is fully correct.

[yav]

Not an export, but the changes seem reasonable to me.