Return 0 if invalid seckey is given to ec_privkey_negate #668

1. 
   jonasnick commented at 6:51 pm on October 7, 2019: contributor

   Fixes #488.

   Instead of just fixing the documentation it’s much better for misuse resistance to return 0 if the seckey overflows. I think most people who use this function and expect it to return 1 always will prefer their code to crash instead of getting a wrong result.

2. in src/tests.c:4120 in e9fe72059f outdated

   4115+    CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1);
   4116+    CHECK(memcmp(seckey, seckey_tmp, 32) != 0);
   4117+    CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1);
   4118+    CHECK(memcmp(seckey, seckey_tmp, 32) == 0);
   4119+
   4120+    /* Negating an overflowing seckey does not work */
   

   ---

   ---

   

   gmaxwell commented at 1:27 am on October 8, 2019:

   Consistency of the actual behaviour would be better verified by a test that sets 16 bytes as 0xFF and then sets the remaining bytes randomly, in order to detect a change to a behaviour that always sets the output to all 0xFF.

   The documentation doesn’t promise one behaviour vs the other, but preserving any entropy in the bad input is the safer behaviour (e.g. the data in it might get fed elsewhere into some rng hash or something) and the test should catch if it gets changed.

   A more extensive test would also(?) check the boundary conditions (smallest non-overflowing, smallest overflowing, maximum value). I don’t think that’s particularly important so long as scalar-set’s overflow behaviour is checked precisely else where (I didn’t look).

   ---

   

   jonasnick commented at 9:10 am on October 8, 2019:

   I improved the test according to your suggestion. scalar_set_b32 just uses secp256k1_scalar_reduce which I assume is well tested but I added a commit with boundary condition tests explicitly for scalar_set_b32.
3. jonasnick force-pushed on Oct 8, 2019
4. Return 0 if invalid seckey is given to ec_privkey_negate 809103663f
5. Add test for boundary conditions of scalar_set_b32 with respect to overflows 1748318f54
6. in include/secp256k1.h:584 in 809103663f outdated

   581+ *  Returns: 1 if seckey was successfully negated and 0 otherwise
   582  *  Args:   ctx:        pointer to a context object
   583- *  In/Out: seckey:     pointer to the 32-byte private key to be negated (cannot be NULL)
   584+ *  In/Out: seckey:     pointer to the 32-byte private key to be negated. The private key
   585+ *                      interpreted as an integer (most significant byte first) must be less than
   586+ *                      the curve order. (cannot be NULL)
   

   ---

   ---

   

   real-or-random commented at 1:32 am on October 9, 2019:

   less than the curve order and not be 0?

   (Actually we should just document what a valid private key is, e.g., here https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L552)

   ---

   

   jonasnick commented at 5:43 pm on October 9, 2019:

   It won’t fail for 0 though. I think ideally we document what’s a valid secret key is (as you said) in the doc of seckey_verify and then just refer to that instead of having to restate it over and over. I can rewrite this PR to do that.
7. jonasnick cross-referenced this on Oct 9, 2019 from issue Make ec_ arithmetic more consistent and add documentation by jonasnick
8. jonasnick force-pushed on Oct 28, 2019
9. 
   jonasnick commented at 1:25 pm on October 28, 2019: contributor

   I added a commit to return 0 on an all 0 key because otherwise the result is an invalid seckey. Also moved definition of valid secret keys to verify_seckey as suggested by @real-or-random
10. Return 0 in privkey_negate for an all 0 seckey and move definition of valid secret keys to verify_seckey 3f5512c81b
11. 
    real-or-random commented at 1:37 pm on October 28, 2019: contributor

    ACK 3f5512c81b17f1412d7992f8fe418a1ab629d955 I read the code and tested it, also with valgrind

    It should be noted that this changes the observable behavior of ec_privkey_negate, which is also used in CKey::Negate() in Bitcoin Core, see https://github.com/bitcoin/bitcoin/blob/d0f81a96d9c158a9226dc946bdd61d48c4d42959/src/key.cpp#L168. But CKey::Negate() is only used in the tests, as far as I can tell.

12. jonasnick cross-referenced this on Dec 17, 2019 from issue Make ec_ arithmetic more consistent and add documentation by jonasnick
13. 
    jonasnick commented at 5:37 pm on December 17, 2019: contributor

    Superseded by #701
14. jonasnick closed this on Dec 17, 2019

    ---

15. real-or-random referenced this in commit f39f99be0e on Apr 30, 2020
16. elichai cross-referenced this on May 4, 2020 from issue Should passing invalid recid to sig parsing call the callback? by elichai

Contributors
jonasnick gmaxwell real-or-random