Verify that secp256k1_ge_set_gej_zinv does not operate on infinity. #942

pull roconnor-blockstream wants to merge 2 commits into bitcoin-core:master from roconnor-blockstream:20210512_VERIFY_zinv changing 3 files +3 −0
  1. 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.
  2. 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;
  3. real-or-random approved
  4. 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.

  5. roconnor-blockstream force-pushed on Aug 28, 2021
  6. roconnor-blockstream commented at 6:03 pm on August 28, 2021: contributor
    rebased.
  7. 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.

  8. 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.

  9. 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.

  10. 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
  11. Comment and check a parameter for inf in secp256k1_ecmult_const. 099bad945e
  12. 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:

    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.
  13. roconnor-blockstream force-pushed on Dec 3, 2021
  14. robot-dreams commented at 7:03 pm on December 3, 2021: contributor
    ACK 099bad945e9a7c5237cdd764eca420285a9de279
  15. 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.

  16. 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
  17. real-or-random merged this on Dec 7, 2021
  18. real-or-random closed this on Dec 7, 2021

  19. sipa referenced this in commit 86dbc4d075 on Dec 15, 2021
  20. sipa cross-referenced this on Dec 15, 2021 from issue Update libsecp256k1 subtree to current master by sipa
  21. jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick
  22. real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022
  23. gwillen referenced this in commit 35d6112a72 on May 25, 2022
  24. janus referenced this in commit 879a9a27b9 on Jul 10, 2022
  25. patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
  26. patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
  27. backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023
  28. str4d referenced this in commit 6de4698bf9 on Apr 21, 2023
  29. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  30. vmta referenced this in commit 8f03457eed on Jul 1, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me