ecdsa sign-to-contract module, with anti nonce covert chan util functions #637
Conversation
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
2 times, most recently
from
006407f to
a4537c0
Compare
|
cc @jonasnick Is this the right direction? If so I can add the host functions and some unit tests. |
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
from
a4537c0 to
ba34c6e
Compare
There was a problem hiding this comment.
I'm happy to see that someone cares about this for ECDSA. Backwards compatibility is important, so creating a new signing function is the right way to go. We should probably replace the word sidechan with covertchan because it's more precise. Fwiw your naming it's only semi-consistent with mine ;P
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
2 times, most recently
from
fc2f847 to
034c512
Compare
|
By the way feel free to squash. |
I am very much interested in moving this from an experiment to production, so I will definitely add those. I will only get a chance to work on it after next week. Should I move all of this over to a new module? I assume modifications directly to |
I think that's a good idea. Having this as an experimental module would probably reduce the barrier for getting merged |
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
3 times, most recently
from
4895cf9 to
5408a2f
Compare
benma
changed the title
|
@jonasnick I pushed a re-do, please take a look.
Is this sufficient for security? Otherwise, we'd need to do one of:
|
There was a problem hiding this comment.
I did another relatively shallow review round without taking a look at the covert channel protection. It would make sense to split the ecdsa_s2c and anti_covert_chan things into two commits.
Is this sufficient for security?
Yes, it's still binding. Informally, to break the binding property an attacker would need to find data and original_pubnonce for a given R such that R = commit(data, original_pubnonce) = original_pubnonce + hash(original_pubnonce, data)*G. Since we're just comparing x-coordinates here, the attacker would have to find x(R) = x(commit(data, original_pubnonce)) which is not asymptotically easier because there are only two points with the same x-coordinates.
Also, since we can't batch verify these commitments anyway comparing the x-coordinates is just fine.
Regarding tests it seems like API tests are missing. They verify that the the right context flags are used and the correct ARG_CHECKS are there. Also, there need to be some negative s2c_verify tests, where you flip a bit in the committed data, etc. (see my PR), also a test where the opening is not initialized. Ideally you check with gcov that every line that's reachable is hit by the tests.
| #endif | ||
|
|
||
| /** Same as secp256k1_ecdsa_sign, but s2c_data32 is committed to by adding `hash(R1, s2c_data32)` to | ||
| * the nonce generated by noncefp, with `ndata=hash(s2c_data32, ndata)`. |
There was a problem hiding this comment.
nit: The with [...] part is a bit difficult to understand. Also noncefp is always secp256k1_nonce_function_default. Perhaps for this function we can remove the noncefp and ndata argument because they don't have a purpose anyway.
There was a problem hiding this comment.
Are we sure that there is no use case for ndata when you could also provide s2c_data? I am not familiar with uses of ndata in general, but if s2c_data alone is enough for all cases, then ndata could also be removed from secp256k1_schnorrsig_sign?
I can remove noncefp, but we could also keep it so other functions could be allowed in the future without breaking API compatibility.
There was a problem hiding this comment.
ndata is for giving additional information to the nonce function. Since it is hashed with s2c_data I don't see how you'd transmit usable information to the nonce function in that way. ndata shouldn't be removed from schnorrsig_sign because schnorrsig_sign can be used with other nonce functions than just the default. Imo, simpler is better than optimizing for some unknown future usecase.
There was a problem hiding this comment.
Dropped the noncefp and ndata params from the function in the alternative here. Can also do it for this version if we choose this alternative over the other.
| * s2c_opening: pointer to an secp256k1_s2c_opening structure which can be | ||
| * NULL but is required to be not NULL if this signature creates | ||
| * a sign-to-contract commitment (i.e. the `s2c_data` argument | ||
| * is not NULL). nonce_is_negated is always 0 for ecdsa. |
There was a problem hiding this comment.
nonce_is_negated is irrelevant because users of this API should never look into s2c_opening structs.
There was a problem hiding this comment.
removed mention of it
|
|
||
| #include "include/secp256k1_ecdsa_sign_to_contract.h" | ||
|
|
||
| int secp256k1_ecdsa_s2c_sign( |
There was a problem hiding this comment.
Imo much easier to read if it was consistent with the rest of secp where the function signature is a single line.
There was a problem hiding this comment.
imho long lines are less readable, but consistency first 👍 fixed
| return ret; | ||
| } | ||
|
|
||
| int secp256k1_ecdsa_s2c_verify_commit( |
There was a problem hiding this comment.
misses VERIFY and ARG_CHECKs.
| /* Check that sigr (x coordinate of R) matches the x coordinate of the commitment. */ | ||
| secp256k1_ecdsa_signature_load(ctx, &sigr, &sigs, sig); | ||
|
|
||
| secp256k1_pubkey_load(ctx, &commitment_ge, &commitment); |
There was a problem hiding this comment.
Return value should be checked.
There was a problem hiding this comment.
fixed, though the negative branch cannot be hit I think, as secp256k1_ec_commit() always produces a valid pubkey.
| secp256k1_pubkey_load(ctx, &commitment_ge, &commitment); | ||
| secp256k1_fe_normalize(&commitment_ge.x); | ||
| secp256k1_fe_get_b32(b, &commitment_ge.x); | ||
| secp256k1_scalar_set_b32(&computed_r, b, NULL); |
There was a problem hiding this comment.
Let's avoid having to deal with subtle issues regarding modular reduction and either check the overflow argument or compare b with scalar_get_b32(sigr) using memcmp.
There was a problem hiding this comment.
Done, went with memcmp
|
|
||
| #include "include/secp256k1_ecdsa_sign_to_contract.h" | ||
|
|
||
| int secp256k1_ecdsa_s2c_sign( |
There was a problem hiding this comment.
The previous version in this PR was easier to review. I don't remember how exactly it looked like but you'd get there by doing something like this:
- move this function to
secp256k1.cand rename it tosecp256k1_ecdsa_sign_helper. - change regular
ecdsa_signfunction to just callecdsa_sign_helperfunction but withs2c_dataands2c_openingset to NULL.
Then we wouldn't have all this code duplication. The downside is that we wouldn't be able to fully test ecdsa_sign_helper without enabling the s2c module.
There was a problem hiding this comment.
I copied it to the module with the goal of not touching secp256k1_ecdsa_sign. I thought that was the goal, otherwise wouldn't this nullify the purpose of an experimental module and increase the barrier to merge?
For comparison, the recoverable module also copied the whole function with the minimal change of extracting the recid.
Here, it is also a copy except for the if(s2c_data32 != NULL) { clauses and s2c_data32 related arg checks.
Imho it would make sense to postpone this refactoring to a future PR, and also cover the recoverable module. Since the tests for the helper and the main signing functions would overlap >90%, the tests should also be refactored to not copy all the tests with slightly different call signature.
There was a problem hiding this comment.
wouldn't this nullify the purpose of an experimental module and increase the barrier to merge?
No because the point is that people can use sign-to-contract only if they pass the --enable-experimental-modules option. That the helper doesn't change ecdsa signing if s2c_data is not set is much easier to check in review than having to diff ecdsa_sign and ecdsa_s2c_sign.
@sipa Summarizing above discussion, do you prefer copying the ecdsa code into the ecdsa_s2c module or should we make the main ecdsa function accept s2c_data but don't expose the argument in secp256k1.h - only in secp256k1_ecdsa_sign_to_contract?
There was a problem hiding this comment.
Just to give a quick update: I have been a bit busy the past weeks, but I still intend to move this to production on the bitbox02 (prototype already working).
I will happily refactor the current code like you suggested, then we can compare. I expect to be able to do this two weeks from now.
In the meantime, I'd still be eager to hear @sipa's opinion on how to best structure this.
There was a problem hiding this comment.
I rebased this PR, and made PR to compare both alternatives: #669
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
2 times, most recently
from
2c82686 to
d459059
Compare
|
Thanks again! I split it into two commits (s2c / nonce covert channel funcs) and added a lot of tests. There are very few lines which I'm not sure how to exercise, e.g. how to get a zero/overflowing nonce with |
Yeah, not possible. In the future we should consistently mark unreachable lines. |
benma
changed the title
…eaks of public keys. The functionality is not exposed.
Sign a message while comitting to some data at the same time by by adding `hash(k1*G, data)` to the rfc6979 deterministic nonce. Includes verification function to check that the signature commits to the data.
benma
force-pushed
the
ecdsa_nonce_sidechan
branch
from
d459059 to
18cb435
Compare
|
Closing in favour of #669 as discussed here. Thanks for the detailed review! |
This is based on the description of the fix by Stepan: https://medium.com/cryptoadvance/hardware-wallets-can-be-hacked-but-this-is-fine-a6156bbd199
The protocol wording and functions are copied/adapted from Jonas
Nick's PRs which do the same for BIP-Schnorr:
#590
ae5fb7f#diff-b19c5ee427283d4d82bc5beb4e2f4777R59
ae5fb7f#diff-313ca26f0048bc16a608709915d0111eR70
Add secp256k1_ecdsa_anti_nonce_sidechan_client_commit to return the
curve point committing to the signing client nonce.
This is a convenience function and can technically be emulated by
calling secp256k1_ecdsa_sign() and reconstructing the curve point from
the signature r/s values.
secp256k1_ecdsa_sign_nonce_tweak_add, which is the same as
secp256k1_ecdsa_sign_nonce, but with an additional optional tweak parameter to
add to the nonce.
The nicer way to do this is to redefine
secp256k1_nonce_functiontohave a tweak param, but this would break API compatiblity. The way it
is implemented is fully backwards compatible.