Cleaner infinity handling in group law and ecmult_const. #791

gmaxwell · 2020-08-08T03:59:00Z

Clean up infinity handling, make x/y/z always initialized for infinity.

Make secp256k1_ecmult_const handle infinity.

Infinity isn't currently needed here, but correctly handling it is a little more safe against future changes.

Update docs for it to make it clear that it is not constant time in Q. It never was constant time in Q (and would be a little complicated to make constant time in Q: needs a constant time addition function that tracks RZR). It isn't typical for ECDH to be constant time in terms of the pubkey.

If it was later made constant time in Q infinity support would be easy to preserve, e.g. by running it on a dummy value and cmoving infinity into the output.

gmaxwell · 2020-08-08T03:59:23Z

The second commit is an alternative to #789.

elichai · 2020-08-08T11:32:45Z

Verified that indeed it isn't constant time on the point:

==2047291== Memcheck, a memory error detector
==2047291== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2047291== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==2047291== Command: /home/elichai2/gits/secp256k12/.libs/lt-valgrind_ctime_test
==2047291== 
==2047291== Conditional jump or move depends on uninitialised value(s)
==2047291==    at 0x484E271: secp256k1_pubkey_load (secp256k1.c:254)
==2047291==    by 0x4856353: secp256k1_ecdh (main_impl.h:47)
==2047291==    by 0x1096B7: main (valgrind_ctime_test.c:74)
==2047291== 
==2047291== Conditional jump or move depends on uninitialised value(s)
==2047291==    at 0x4849B69: secp256k1_fe_normalizes_to_zero_var (field_5x52_impl.h:206)
==2047291==    by 0x4851F1B: secp256k1_gej_add_ge_var (group_impl.h:445)
==2047291==    by 0x4852541: secp256k1_ecmult_odd_multiples_table.constprop.0 (ecmult_impl.h:122)
==2047291==    by 0x4856721: secp256k1_ecmult_odd_multiples_table_globalz_windowa (ecmult_impl.h:152)
==2047291==    by 0x4856721: secp256k1_ecmult_const (ecmult_const_impl.h:178)
==2047291==    by 0x4856721: secp256k1_ecdh (main_impl.h:53)
==2047291==    by 0x1096B7: main (valgrind_ctime_test.c:74)
==2047291== 
==2047291== Conditional jump or move depends on uninitialised value(s)
==2047291==    at 0x4849B7B: secp256k1_fe_normalizes_to_zero_var (field_5x52_impl.h:206)
==2047291==    by 0x4851F1B: secp256k1_gej_add_ge_var (group_impl.h:445)
==2047291==    by 0x4852541: secp256k1_ecmult_odd_multiples_table.constprop.0 (ecmult_impl.h:122)
==2047291==    by 0x4856721: secp256k1_ecmult_odd_multiples_table_globalz_windowa (ecmult_impl.h:152)
==2047291==    by 0x4856721: secp256k1_ecmult_const (ecmult_const_impl.h:178)
==2047291==    by 0x4856721: secp256k1_ecdh (main_impl.h:53)
==2047291==    by 0x1096B7: main (valgrind_ctime_test.c:74)
==2047291== 
==2047291== 
==2047291== HEAP SUMMARY:
==2047291==     in use at exit: 0 bytes in 0 blocks
==2047291==   total heap usage: 1 allocs, 1 frees, 224 bytes allocated
==2047291== 
==2047291== All heap blocks were freed -- no leaks are possible
==2047291== 
==2047291== Use --track-origins=yes to see where uninitialised values come from
==2047291== For lists of detected and suppressed errors, rerun with: -s
==2047291== ERROR SUMMARY: 15 errors from 3 contexts (suppressed: 0 from 0)

gmaxwell · 2020-08-08T23:33:08Z

Can someone with access please kick travis to clear the spurious failure?

sipa · 2020-08-08T23:47:49Z

Can someone with access please kick travis to clear the spurious failure?

Done.

sipa · 2020-08-09T23:32:14Z

It seems s390x doesn't really work; I think we need to disable it again unfortunately. See #794.

gmaxwell · 2020-08-10T02:36:12Z

I'll rebase when 794 is merged.

sipa · 2020-08-10T22:33:16Z

utACK 6e712dc

Seems s390x is green again; let's hope it stays that way.

gmaxwell · 2020-08-11T02:57:09Z

Okay, then this doesn't need to be rebased.

Unsurprisingly, no gross performance loss, FWIW:

Before:
ecdsa_verify: min 54.9us / avg 54.9us / max 54.9us
ecdh: min 65.6us / avg 65.7us / max 65.8us
After:
ecdsa_verify: min 54.9us / avg 55.0us / max 55.1us
ecdh: min 65.6us / avg 65.7us / max 65.8us

I think that tiny difference is real but it might be compiler alignment noise.

real-or-random · 2020-08-11T09:17:58Z

In the first commit, both modified functions currently have VERIFY_CHECK(!a->infinity); lines.

gmaxwell · 2020-08-11T11:14:51Z

Yep. I didn't intend to change their infinity-handling-ness beyond making them less wrong. (for the second function too, making those handle infinities would be non-trivial)

src/ecmult_const_impl.h

real-or-random · 2020-08-13T11:12:35Z

Yep. I didn't intend to change their infinity-handling-ness beyond making them less wrong. (for the second function too, making those handle infinities would be non-trivial)

Yeah okay, I suspect that the fix is not that easy but I didn't bother to understand the code when I wrote the comment.

But then, this seems to me like a little code smell to do VERIFY_CHECK(!a->infinity); but then use that value.

Okay, so... It anyway does not make a lot to for this functions to support infinity but maybe it's a good idea to add an additional comment to the doc header that a must not be infinity.

jonasnick

make x/y/z always initialized for infinity.

That z implies this is for gej only?

gmaxwell · 2020-08-29T21:03:22Z

@jonasnick What non-gej-outputting function are you thinking of?

gmaxwell · 2020-08-29T21:21:22Z

Rebased and added a comment that secp256k1_ecmult_odd_multiples_table's a argument cannot be infinity.

sipa · 2020-09-02T01:19:52Z

As it seems this should be adding a strict invariant that ge/gej objects always have sane coordinates, I added checks for that in VERIFY mode: https://github.com/sipa/secp256k1/commits/202009_pr791. There was one place where this is violated (secp256k1_ge_set_gej_var with infinity input didn't initialize the output ge coordinates), which I added a fix for. Feel free to cherry-pick the commits, or I can do it afterwards in another PR.

gmaxwell · 2020-09-02T03:39:38Z

Your branch needs a patch along the lines of

diff --git a/src/group_impl.h b/src/group_impl.h
index 950b51d..b8ed209 100644
--- a/src/group_impl.h
+++ b/src/group_impl.h
@@ -197,6 +197,8 @@ static void secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a
         r[i].infinity = a[i].infinity;
         if (!a[i].infinity) {
             secp256k1_ge_set_gej_zinv(&r[i], &a[i], &r[i].x);
+        } else {
+            secp256k1_ge_set_infinity(&r[i]);
         }
         secp256k1_ge_verify(&r[i]);
     }

or the tests call ge_verify on incompletely initialized elements.

sipa · 2020-09-02T03:51:47Z

Fixed. The

r[i].infinity = a[i].infinity;

line above can be removed now.

real-or-random · 2020-09-02T10:11:37Z

If we want to initialize the coords here in the set functions, then we also want them in the add functions, see #699.

sipa · 2020-09-02T18:04:44Z

@real-or-random Good to point that out, the changes here not enough to guarantee that field elements in ge/gej are always initialized, and I wonder why the tests don't catch that.

If we're going in the direction of actually enforcing that invariant, we should make both changes.

jonasnick · 2020-09-02T20:04:00Z

@jonasnick What non-gej-outputting function are you thinking of?

Was thinking of ge_set_gej_var but looks like that's fixed now. Ideally we'd clarify the commit message Clean up infinity handling, make x/y/z always initialized for infinity. because without the follow up commits this does not seem to be true.

sipa · 2020-09-09T03:41:24Z

ACK 49a59a1 (+ my own commits, up to 95d8d92).

Should be combined with #699 to deal with the exceptional branches in add functions (which can still leave output coordinates uninitialized otherwise). It can be cleanly merged with this.

real-or-random

ACK 95d8d92 diff looks good, tests pass

Let's address these nits somewhere else.

real-or-random · 2020-09-09T14:16:09Z

src/group_impl.h

 }

 static void secp256k1_gej_clear(secp256k1_gej *r) {
    r->infinity = 0;
    secp256k1_fe_clear(&r->x);
    secp256k1_fe_clear(&r->y);
    secp256k1_fe_clear(&r->z);
+    secp256k1_gej_verify(r);


nit: I believe this is conceptually wrong. The purpose of the clear function is nuke the contents of the element, so those functions are not supposed to uphold any invariants. But it doesn't matter for this PR, I can clean this up in #636 .

This got lost in rebase as well.

real-or-random · 2020-09-09T14:18:37Z

src/ecmult_const_impl.h

+    if (secp256k1_ge_is_infinity(a)) {
+      secp256k1_gej_set_infinity(r);
+      return;
+    }


nit: Indentation

real-or-random · 2020-09-13T09:38:08Z

I think after this PR (and #814) we initialize group elements fully everywhere, and we could introduce VG_CHECKs for this.

sipa · 2020-09-13T19:06:35Z

Yeah, there could be VG_CHECKs in secp256k1_{fe,ge,gej}_verify after this and #814.

elichai · 2020-09-14T09:12:26Z

ACK 95d8d92 the diff looks correct and reasonable
idk how I feel about moving the VERIFY ifdef from the secp256k1_*_verify functions to their content, but I guess it makes the code smaller and cleaner (less ifdefs all over), and the comment clearly says it's a no-op on non VERIFY builds.

sipa · 2020-10-05T22:09:24Z

A rebased version of this PR, with @real-or-random's nit above addressed is here: https://github.com/sipa/secp256k1/commits/202009_pr791

gmaxwell · 2020-10-06T03:28:33Z

I switched this to sipa's rebase.

jonasnick · 2020-10-23T09:16:03Z

This seems to be ready for merge but needs rebase.

real-or-random · 2020-10-23T10:22:47Z

This seems to be ready for merge but needs rebase.

Oh sorry, we should just have merged this earlier, I think we had the ACKs. :/

Infinity isn't currently needed here, but correctly handling it is a little more safe against future changes. Update docs for it to make it clear that it is not constant time in Q. It never was constant time in Q (and would be a little complicated to make constant time in Q). If it was later made constant time in Q infinity support would be easy to preserve, e.g. by running it on a dummy value and cmoving infinity into the output.

Also define it even when VERIFY is not set (as a no-op), to avoid conditions when calling it.

sipa

re-ACK by verifying against rebasing myself, but a few small improvements got lost.

sipa · 2020-10-27T03:23:39Z

src/ecmult_const_impl.h

@@ -150,6 +150,10 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons

    /* build wnaf representation for q. */
    int rsize = size;
+    if (secp256k1_ge_is_infinity(a)) {
+      secp256k1_gej_set_infinity(r);


I believe this got lost in rebase: wrong indentation

sipa · 2020-10-27T03:24:41Z

src/group_impl.h

 }

 static void secp256k1_gej_clear(secp256k1_gej *r) {
    r->infinity = 0;
    secp256k1_fe_clear(&r->x);
    secp256k1_fe_clear(&r->y);
    secp256k1_fe_clear(&r->z);
+    secp256k1_gej_verify(r);


This got lost in rebase as well.

sipa · 2020-10-27T03:24:49Z

src/group_impl.h

 }

 static void secp256k1_ge_clear(secp256k1_ge *r) {
    r->infinity = 0;
    secp256k1_fe_clear(&r->x);
    secp256k1_fe_clear(&r->y);
+    secp256k1_ge_verify(r);


real-or-random · 2021-06-10T11:39:43Z

needs rebase

sipa · 2023-05-09T16:51:55Z

Rebased as #1299.

…up verification bbc8344 Avoid secp256k1_ge_set_gej_zinv with uninitialized z (Pieter Wuille) 0a2e0b2 Make secp256k1_{fe,ge,gej}_verify work as no-op if non-VERIFY (Pieter Wuille) f202667 Add invariant checking to group elements (Pieter Wuille) a18821d Always initialize output coordinates in secp256k1_ge_set_gej (Pieter Wuille) 3086cb9 Expose secp256k1_fe_verify to other modules (Pieter Wuille) a0e696f Make secp256k1_ecmult_const handle infinity (Gregory Maxwell) Pull request description: Rebase of #791. * Clean up infinity handling, make x/y/z always initialized for infinity. * Make secp256k1_ecmult_const handle infinity. * Infinity isn't currently needed here, but correctly handling it is a little more safe against future changes. * Update docs for it to make it clear that it is not constant time in Q. It never was constant time in Q (and would be a little complicated to make constant time in Q: needs a constant time addition function that tracks RZR). It isn't typical for ECDH to be constant time in terms of the pubkey. If it was later made constant time in Q infinity support would be easy to preserve, e.g. by running it on a dummy value and cmoving infinity into the output. * Add group verification (`secp256k1_ge_verify` and `secp256k1_gej_verify`, mimicking `secp256k1_fe_verify`). * Make the `secp256k1_{fe,ge,gej}_verify` functions also defined (as no-ops) in non-VERIFY mode. ACKs for top commit: jonasnick: ACK bbc8344 real-or-random: ACK bbc8344 Tree-SHA512: 82cb51faa2c207603aa10359a311ea618fcb5a81ba175bf15515bf84043223db6428434875854cdfce9ae95f9cfd68c74e4e415f26bd574f1791b5dec1615d19

LLFourn mentioned this pull request Aug 8, 2020

ecmult implementation is not constant time on the point LLFourn/secp256kfun#14

Closed

LLFourn reviewed Aug 13, 2020

View reviewed changes

src/ecmult_const_impl.h Show resolved Hide resolved

LLFourn mentioned this pull request Aug 13, 2020

Test Check multiplying by infinity returns infinity LLFourn/secp256kfun#13

Closed

jonasnick reviewed Aug 23, 2020

View reviewed changes

sipa mentioned this pull request Sep 2, 2020

Initialize field elements when resulting in infinity #699

Merged

real-or-random approved these changes Sep 9, 2020

View reviewed changes

sipa mentioned this pull request Oct 10, 2020

Rip out non-endomorphism code #826

Closed

gmaxwell and others added 5 commits October 23, 2020 21:15

Clean up infinity handling, make x/y/z always initialized for infinity.

1864490

Expose secp256k1_fe_verify to other modules

c8739ee

Also define it even when VERIFY is not set (as a no-op), to avoid conditions when calling it.

Always initialize output coordinates in ge_set_[all_]gej_var

e5f1b88

Add invariant checking to group elements

666d210

sipa reviewed Oct 27, 2020

View reviewed changes

real-or-random mentioned this pull request Jun 10, 2021

Valgrind errors with struct assignment on ARM and PPC64LE #776

Open

sipa mentioned this pull request May 9, 2023

Infinity handling: ecmult_const(infinity) works, and group verification #1299

Merged

sipa closed this May 9, 2023

Cleaner infinity handling in group law and ecmult_const. #791

Cleaner infinity handling in group law and ecmult_const. #791

Conversation

gmaxwell commented Aug 8, 2020 • edited Loading

gmaxwell commented Aug 8, 2020

elichai commented Aug 8, 2020

gmaxwell commented Aug 8, 2020

sipa commented Aug 8, 2020

sipa commented Aug 9, 2020

gmaxwell commented Aug 10, 2020

sipa commented Aug 10, 2020 • edited Loading

gmaxwell commented Aug 11, 2020

real-or-random commented Aug 11, 2020

gmaxwell commented Aug 11, 2020

real-or-random commented Aug 13, 2020

jonasnick left a comment

Choose a reason for hiding this comment

gmaxwell commented Aug 29, 2020

gmaxwell commented Aug 29, 2020

sipa commented Sep 2, 2020

gmaxwell commented Sep 2, 2020

sipa commented Sep 2, 2020 • edited Loading

real-or-random commented Sep 2, 2020

sipa commented Sep 2, 2020

jonasnick commented Sep 2, 2020 • edited Loading

sipa commented Sep 9, 2020 • edited Loading

real-or-random left a comment

Choose a reason for hiding this comment

real-or-random Sep 9, 2020

Choose a reason for hiding this comment

sipa Sep 9, 2020

Choose a reason for hiding this comment

sipa Oct 27, 2020

Choose a reason for hiding this comment

real-or-random Sep 9, 2020

Choose a reason for hiding this comment

real-or-random commented Sep 13, 2020

sipa commented Sep 13, 2020

elichai commented Sep 14, 2020

sipa commented Oct 5, 2020

gmaxwell commented Oct 6, 2020

jonasnick commented Oct 23, 2020

real-or-random commented Oct 23, 2020

sipa left a comment

Choose a reason for hiding this comment

sipa Oct 27, 2020

Choose a reason for hiding this comment

sipa Oct 27, 2020

Choose a reason for hiding this comment

sipa Oct 27, 2020

Choose a reason for hiding this comment

real-or-random commented Jun 10, 2021

sipa commented May 9, 2023

gmaxwell commented Aug 8, 2020 •

edited

Loading

sipa commented Aug 10, 2020 •

edited

Loading

sipa commented Sep 2, 2020 •

edited

Loading

jonasnick commented Sep 2, 2020 •

edited

Loading

sipa commented Sep 9, 2020 •

edited

Loading