Verify that secp256k1_ge_set_gej_zinv does not operate on infinity.

roconnor-blockstream commented at 2:12 pm on May 12, 2021: contributor

a->x and a->y should not be used if the infinity flag is set.

roconnor-blockstream commented at 2:14 pm on May 12, 2021: contributor

There is a minor question as to whether the last line should be replaced with r->infinity = 0;

real-or-random approved

real-or-random commented at 9:51 am on May 14, 2021: contributor

utACK https://github.com/bitcoin-core/secp256k1/commit/32338175cf1c1dca0b1bca7f2e1974297fc9e4d0

Setting the infinity flag: I think it’s okay to keep this line. It errs on the safe side.

roconnor-blockstream force-pushed on Aug 28, 2021

roconnor-blockstream commented at 6:03 pm on August 28, 2021: contributor

rebased.

jonasnick commented at 3:38 pm on October 15, 2021: contributor

a->x and a->y should not be used if the infinity flag is set.

Why exactly? x and y are cleared in ge{,j}_set_infinity.

One could argue that it’s a bug to call this function with a being infinity because there’s no corresponding zi. On the other hand, since the behavior is not documented one may think that zi is ignored if a is infinity. It seems like there are quite a few call sites to go through to verify that gej_zinv is guaranteed to not be called with such an a.

roconnor-blockstream commented at 3:47 pm on October 15, 2021: contributor

It certainly used to be the case that the infinity flag could be set without the coordinates being initialized at all. I have been working at eliminating these cases: e.g. https://github.com/bitcoin-core/secp256k1/pull/937/commits/31c0f6de413e521731ad0e63424431b3dd49cec8

As part of “defense in depth” I think we should both endeavor to both always initialize all fields of these structures (because even copying structures with uninitialized fields is questionable) while also maintaining the invariant that the coordinates are never accessed when the infinity flag is set.

To this end I want to get to a state where fe_clear will set the memory to uninitialized for the purposes of valgrind, and use set_int(...,0) for those cases were were explicitly want the value to be 0.

real-or-random commented at 3:55 pm on October 15, 2021: contributor

To this end I want to get to a state where fe_clear will set the memory to uninitialized for the purposes of valgrind, and use set_int(...,0) for those cases were were explicitly want the value to be 0.

This will be https://github.com/bitcoin-core/secp256k1/pull/636/commits/b33a8e49e89f5a1ea02a9b9b9b208cb4f3d59e44 (and maybe the following commit in that PR for other modules).

I can take these out of #636 after this PR.

Verify that secp256k1_ge_set_gej_zinv does not operate on infinity.

a->x and a->y should not be used if the infinity flag is set.

6c0be857f8

Comment and check a parameter for inf in secp256k1_ecmult_const. 099bad945e

in src/group_impl.h:70 in 44956337f6 outdated

66@@ -67,6 +67,7 @@ static const secp256k1_fe secp256k1_fe_const_b = SECP256K1_FE_CONST(0, 0, 0, 0,
67 static void secp256k1_ge_set_gej_zinv(secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zi) {
68     secp256k1_fe zi2;
69     secp256k1_fe zi3;
70+    VERIFY_CHECK(!a->infinity);

robot-dreams commented at 3:12 am on December 3, 2021:

Do we need to account for secp256k1_ecmult_const, where the caller can possibly pass a = infinity and cause this check to fail?

https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_const_impl.h#L172 https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h#L113 https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h#L81

roconnor-blockstream commented at 3:34 am on December 3, 2021:

secp256k1_ecmult_odd_multiples_table already has a VERIFY_CHECK for infinity, if I’m reading this correctly.

src/modules/ecdh/main_impl.h seems to be the only place where secp256k1_ecmult_const is used, and it is only used with a loaded public key, which is never infinity.

I’m somewhat skeptical that the table building function will work when given infinity.

Perhaps the documentation for secp256k1_ecmult_const should be updated to explicitly state that the input point cannot be infinity, and maybe a VERIFY_CHECK added there too.

robot-dreams commented at 3:49 am on December 3, 2021:

Oops, you’re right, the failed check (which I triggered by calling secp256k1_ecmult_const with a = infinity in a test) probably came from this path instead:

https://github.com/bitcoin-core/secp256k1/blob/master/src/ecmult_impl.h#L115 https://github.com/bitcoin-core/secp256k1/blob/master/src/group_impl.h#L183

It looks like all other calls (in secp256k1_ecmult_odd_multiples_table and secp256k1_ge_set_all_gej_var) are protected by a check !a.infinity.

robot-dreams commented at 3:54 am on December 3, 2021:

Adding a comment and a VERIFY_CHECK to secp256k1_ecmult_const sounds good to me, I will ACK after that.

roconnor-blockstream commented at 6:58 pm on December 3, 2021:

Done.

roconnor-blockstream force-pushed on Dec 3, 2021

robot-dreams commented at 7:03 pm on December 3, 2021: contributor

ACK 099bad945e9a7c5237cdd764eca420285a9de279

real-or-random commented at 10:05 am on December 7, 2021: contributor

ACK 099bad945e9a7c5237cdd764eca420285a9de279 I inspected all call sites, they all ensure that a is not infinity

By the way, secp256k1_ecmult_odd_multiples_table_globalz_windowa is in the wrong module. This function had been used within ecmult in the past but since 8c1c831bdb083dfa8b50fac16a0e17a7e1df4064, it is only used in ecmult_const. This also means its comment is wrong. We can fix this in another PR.

real-or-random cross-referenced this on Dec 7, 2021 from issue Move secp256k1_ecmult_odd_multiples_table_globalz_windowa and fix docs by real-or-random

real-or-random merged this on Dec 7, 2021

real-or-random closed this on Dec 7, 2021

sipa referenced this in commit 86dbc4d075 on Dec 15, 2021

sipa cross-referenced this on Dec 15, 2021 from issue Update libsecp256k1 subtree to current master by sipa

jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick

real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022

gwillen referenced this in commit 35d6112a72 on May 25, 2022

janus referenced this in commit 879a9a27b9 on Jul 10, 2022

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023

str4d referenced this in commit 6de4698bf9 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

Verify that secp256k1_ge_set_gej_zinv does not operate on infinity. #942