ecdh/ellswift/schnorrsig: return early for invalid inputs (consistent handling w.r.t. constant-time) - #1919
Open
theStack wants to merge 3 commits into
Open
Conversation
theStack
changed the title
Applies the suggested "function is constant-time only for valid inputs" suggestion in issue bitcoin-core#1621 ("Handle invalid inputs consistently w.r.t. constant-time") for the API function `secp256k1_ecdh`. While being at it, also introduce a check for the pubkey object validity for consistency with other modules. Strictly speaking this is a behaviour change, but it should be fine as it only improves things for the user (from potentially arbitrary behaviour to defined one) and as developers we don't have to reason anymore about code-paths with how the remainder of the function behaves if the pubkey wasn't loaded successfully.
Applies the suggested "function is constant-time only for valid inputs" suggestion in issue bitcoin-core#1621 ("Handle invalid inputs consistently w.r.t. constant-time") for the API functions `secp256k1_ellswift_create` and `secp256k1_ellswift_xdh`.
Applies the suggested "function is constant-time only for valid inputs" suggestion in issue bitcoin-core#1621 ("Handle invalid inputs consistently w.r.t. constant-time") for the API functions `secp256k1_schnorrsig_sign32` and `secp256k1_schnorrsig_sign_custom` (both calling the non-public `secp256k1_schnorrsig_internal` function).
theStack
force-pushed
the
ctime-only_for_valid_inputs
branch
from
August 24, 2026 15:05
c451973 to
971bc49
Compare
Comment on lines
+53
to
+56
| if (!is_sec_valid) { | ||
| secp256k1_ge_clear(&pt); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Should we also clear s here? I understand that since we already know that s is not a valid secret key, it really doesn't matter that much. However, originally the invalid input path would reach the end of the function and s would be cleared anyway.
Contributor
Author
There was a problem hiding this comment.
Good question. I'd argue that there is not much value in protecting invalid key material from leaking (see also recent review comment #1802 (comment)), but being conservative and still clearing it is of course also an option. Curious what other reviewers think.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR partly addresses issue #1621, handling invalid inputs consistently w.r.t. constant-time, by applying the suggested "functions are constant-time only for valid inputs" approach (number 2. in the issue), for the following modules and API functions:
secp256k1_ecdhsecp256k1_ellswift_createandsecp256k1_ellswift_xdhsecp256k1_schnorrsig_sign{32,_custom}As stated in #1621, this leads to more readable and maintainable code, getting rid of constructs like
return (!!ret) & is_sec_valid;or intermediateret &= ...;statements. Conditional assignments likesecp256k1_scalar_cmovcan be removed as well.Tests are added for the newly introduced branches if there were none. There are still further instances to tackle (mostly the main module API functions in secp256k1.c, and potential cleanups like e.g. removing the dummy assignments in
_keypair_load), happy to add these here or in another PR, mostly chasing Conecpt ACKs for now if this is we want to handle constant-time behavior for current and future modules.