Return 0 if invalid seckey is given to ec_privkey_negate #668
Conversation
src/tests.c
Outdated
| CHECK(secp256k1_ec_privkey_negate(ctx, seckey) == 1); | ||
| CHECK(memcmp(seckey, seckey_tmp, 32) == 0); | ||
|
|
||
| /* Negating an overflowing seckey does not work */ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
jonasnick
force-pushed
the
privkey-negate
branch
from
e9fe720 to
1748318
Compare
include/secp256k1.h
Outdated
| * In/Out: seckey: pointer to the 32-byte private key to be negated (cannot be NULL) | ||
| * In/Out: seckey: pointer to the 32-byte private key to be negated. The private key | ||
| * interpreted as an integer (most significant byte first) must be less than | ||
| * the curve order. (cannot be NULL) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
jonasnick
force-pushed
the
privkey-negate
branch
from
16f924a to
3f5512c
Compare
|
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 |
… valid secret keys to verify_seckey
|
ACK 3f5512c 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 |
|
Superseded by #701 |
7e3952a Clarify documentation of tweak functions. (Jonas Nick) 89853a0 Make tweak function documentation more consistent. (Jonas Nick) 41fc785 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul (Jonas Nick) 22911ee Rename private key to secret key in public API (with the exception of function names) (Jonas Nick) 5a73f14 Mention that value is unspecified for In/Out parameters if the function returns 0 (Jonas Nick) f03df0e Define valid ECDSA keys in the documentation of seckey_verify (Jonas Nick) 5894e1f Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul (Jonas Nick) 8f814cd Add test for boundary conditions of scalar_set_b32 with respect to overflows (Jonas Nick) 3fec982 Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify (Jonas Nick) 9ab2cbe Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key (Jonas Nick) Pull request description: Fixes #671. Supersedes #668. This PR unifies handling of invalid secret keys by introducing a new function `scalar_set_b32_secret` which returns false if the b32 overflows or is 0. By using this in `privkey_{negate, tweak_add, tweak_mul}` these function will now return 0 if the secret key is invalid which matches the behavior of `ecdsa_sign` and `pubkey_create`. Instead of deciding whether to zeroize the secret key on failure, I only added documentation for now that the value is undefined on failure. ACKs for top commit: real-or-random: ACK 7e3952a I read the diff carefully and tested the changes apoelstra: ACK 7e3952a Tree-SHA512: 8e9a66799cd3b6ec1c3acb731d6778035417e3dca9300d840e2437346ff0ac94f0c9be4de20aa2fac9bb4ae2f8a36d4e6a34795a640b9cfbfee8311decb102f0
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.