Avoid using out-of-bounds field elements (in impossible cases)

[roconnor-blockstream]

secp256k1_fe_set_b32_limit says that when it returns 0, one is not allowed to use the resulting output value.

This refactors the code so that the existing value of t is maintained in cases where sha256 would output an out-of-bounds hash value.

Note: This situation is cryptographically impossible to occur.

[real-or-random]

Hm, yeah, good catch! This is similar to bitcoin-core/secp256k1#1316 .

I wonder if there's a better fix in this case. Looking at the implementation of _limit, the field element is actually a valid one. Just in the tests, we mark it as invalid by setting the magnitude to -1, so that _fe_verify will catch any read accesses to it.

I guess we could declassify ret and return early when it's 0.

[roconnor-blockstream]

Since we are already changing cryptographically inaccessable behavior in #286, maybe we ought to switch to secp256k1_fe_set_b32_mod. Yes, it is infinitesimally slower to use _mod, but it pales in comparison the the expense of everything else here.

@apoelstra do you have an opinion?

[roconnor-blockstream]

Ah this bug was introduced in #256 when secp256k1_fe_set_b32 was split into _mod and _limit variants. The _limit version isn't quite a drop in replacement due to the fact the post condition says we cannot use the value if failure is returned.

Anyhow, I think we should seriously entertain just using the _mod version and not failing.

Honestly I don't understand why people insist on failing when hashes return values larger than the secp256k1 field size, rather than just wrapping.

[real-or-random]

Yes, it is infinitesimally slower to use _mod, but it pales in comparison the the expense of everything else here.

It's the other way around! _limit first calls _mod, so _mod faster...

[real-or-random]

Anyhow, I think we should seriously entertain just using the _mod version and not failing.

Agreed, that's the best fix.

Honestly I don't understand why people insist on failing when hashes return values larger than the secp256k1 field size, rather than just wrapping.

For the record, I fully agree. We have debated negligible cases a lot in the past, but I've personally settled on this conclusion: Since these cases won't occur in practice, we should not discuss what happens if we hit these cases. We instead just should do whatever is convenient for us.

And in this case, this is for sure taking a mod. This avoids a branch (and an annoying branch because we cannot test it, etc...), it makes sure static analysis tools won't complain about invalid uses / out-of-bound fields, and it's faster...

[roconnor-blockstream]

Okay. I will build and alternative PR that uses _mod.

[roconnor-blockstream]

I wrote up a version that uses _mod and the testsuite (correctly) fails. There is actually a problem with that proposal.

secp256k1-zkp/src/modules/generator/main_impl.h

Line 200 in 03aecaf

secp256k1_fe_cmov(&ge->y, &tmp, secp256k1_fe_is_odd(t));

shallue_van_de_woestijne calls secp256k1_fe_is_odd(t) which means that shallue_van_de_woestijne has a precondition that t is normalized. The _limit version guarantees that the the result is normalized but the _mod version does not.