Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roconnor-blockstream
Copy link
Contributor

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.

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.
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

real-or-random commented Jan 26, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@roconnor-blockstream
Copy link
Contributor Author

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants