Unify secp256k1_zkp usage #2385

andrewkozlik · 2022-07-14T14:02:33Z

Resolves #1864.

Moves the dispatching of zkp_ecdsa_*() vs. ecdsa_*() functions to ecdsa.c to simplify usage (no more #ifdefs for this in core and legacy code).
- As a side-result the above change automatically causes crypto/bip32.c to use the secp256k1_zkp functions for public key derivation and signing without any code changes in bip32.c.
- I renamed the trezor-crypto native implementations of the relevant ecdsa_* functions to tc_ecdsa_* so that they can be tested independently in the crypto tests.
- It seems that the usage of secp256k1_zkp in bip32.c causes a different amount of data to be taken from the PRNG, which in turn causes the PIN matrix layouts to change, hence the changes to UI tests.
Implements Ethereum and EOS canonical ECDSA signing using the secp256k1_zkp library.
- Some of the EOS signatures (10/16) had to be changed, because the signing retry works differently than in the trezor-crypto native implementation.
- Signing of EOS operations will need to be tested extensively by QA!

crypto/ecdsa_internal.h

crypto/zkp_ecdsa.c

onvej-sl · 2022-07-15T13:05:28Z

crypto/ecdsa.c

+                               int recid) {
+#ifdef USE_SECP256K1_ZKP_ECDSA
+  if (curve == &secp256k1) {
+    return zkp_ecdsa_recover_pub_from_sig(curve, pub_key, sig, digest, recid);


What do you think about moving these wrappers into a separate file?

I'm thinking about something like

renaming ecdsa.c to native_ecdsa.c,

renaming ecdsa_internal.h to native_ecdsa.h and

moving the wrappers back to ecdsa.c.

So let me get this straight. Based on the described scheme all the other ecdsa functions such as scalar_multiply() would be implemented in native_ecdsa.c, but their declaration would remain in ecdsa.h while there exists a file called native_ecdsa.h? That seems confusing.

Based on the described scheme all the other ecdsa functions such as scalar_multiply() would be implemented in native_ecdsa.c, but their declaration would remain in ecdsa.h while there exists a file called native_ecdsa.h?

A would move the declaration of scalar_multiply() to native_ecdsa.h.

The thing I don't like about the architecture is ecdsa.c contains both the ecdsa wrappers and the native implementation of ecdsa. These functions should be in separate files, in my opinion.

[I] would move the declaration of scalar_multiply() to native_ecdsa.h.

Then any code that uses functions that were originally declared in ecdsa.h but do not have a counterpart in the secp256k1 library, i.e. are not wrapped, would have to include native_ecdsa.h instead of ecdsa.h (or both). Again seems confusing and technically breaks the API although only a little.

The thing I don't like about the architecture is ecdsa.c contains both the ecdsa wrappers and the native implementation of ecdsa.

I don't see a problem with that other than unit testing, which is an internal matter.

Then any code that uses functions that were originally declared in ecdsa.h but do not have a counterpart in the secp256k1 library, i.e. are not wrapped, would have to include native_ecdsa.h instead of ecdsa.h (or both).

Exactly.

Again seems confusing and technically breaks the API although only a little.

What is confusing about it? Which API does it breaks and how?

I think it's the most natural approach. I see it as ecdsa.c being an interface and zkp_ecdsa.c and native_ecdsa.c being two implementations.

What is confusing about it? Which API does it breaks and how?

It moves certain functions to different header files, such as scalar_multiply() and others.

I think it's the most natural approach. I see it as ecdsa.c being an interface and zkp_ecdsa.c and native_ecdsa.c being two implementations.

If it's meant to be an "interface", then we would also have to add dummy wrappers to ecdsa.c for the functions that are in native_ecdsa.c only. The caller shouldn't have to deal with the question of whether the implementation is native or non-native.

It moves certain functions to different header files, such as scalar_multiply() and others.

What is wrong about it? What is confusing about it?

If it's meant to be an "interface", then we would also have to add dummy wrappers to ecdsa.c for the functions that are in native_ecdsa.c only. The caller shouldn't have to deal with the question of whether the implementation is native or non-native.

This may surprise you but I'm all for it. Nevertheless, I don't think it's necessary because I see nothing wrong with using an "implementation" directly instead of an "interface".

What is wrong about it? What is confusing about it?

As I stated, the caller shouldn't have to deal with the question of whether the implementation is native or non-native. The whole point of this PR is to simplify the usage of the ecdsa calls.

I am not a fan of creating dummy code where it serves no purpose. So I don't like that option either. This is purely a matter of style. It makes no difference in terms of functionality, usability, maintainability or in any other respect. Feel free to create a separate issue.

crypto/zkp_ecdsa.c

Hannsek · 2023-04-14T12:27:18Z

@andrewkozlik What is the state of this pr?

andrewkozlik · 2023-04-14T12:39:46Z

@andrewkozlik What is the state of this pr?

Awaiting review from @onvej-sl. Probably needs manual rebase afterwards.

onvej-sl · 2023-05-26T14:50:02Z

I announce in advance I'm not going to approve this pull request. The pull request is ok in terms of functionality and security (this is the last comment that has to be resolved in this sense) but I don't agree with one of the architectural decisions made. To be specific, I don't like that ecdsa.c contains both

the native implementation of the elliptic curve cryptography and
the wrappers that call either the native implementation or the zkp-secp256k1 implementation.

See this thread. In my opinion, ecdsa.c already contains functions that should be split between multiple files and this pull request makes it worse.

I and Andrew have a different perspective on this. Let someone else review this part of the pull request.

andrewkozlik · 2023-05-29T15:38:07Z

@matejcik please review this discussion. @onvej-sl already reviewed this PR from the functional and security perspectives, but disagrees about the code style.

matejcik · 2023-06-14T11:41:55Z

I did a very brief review of the ecdsa.h interface, and ISTM as of now it can't be considered a good interface. I would basically agree that there's too many functions already, but splitting off the wrappers and tc_ prefixed internal implementations doesn't help anything besides raw line count which is not interesting.

A larger refactor, with possible header API breakage, would be nice -- but this PR is not it and doesn't need to be it.

(that said, my view into trezor-crypto architecture is limited and i might be missing something)

As such I'm approving the PR as it is now. Please resolve the conflicts and let's get this merged.

[no changelog]

The last unresolved requested change is unrelated to this PR.

andrewkozlik added crypto Stand-alone cryptography library used by both Trezor Core and the Trezor Legacy firmware code Code improvements labels Jul 14, 2022

andrewkozlik requested review from matejcik, prusnak and onvej-sl as code owners July 14, 2022 14:02

andrewkozlik removed request for prusnak and matejcik July 14, 2022 14:02

andrewkozlik mentioned this pull request Aug 12, 2022

Investigate how to save flash space #1947

Closed

prusnak added the flash reduction Decrease required size of flash label Aug 30, 2022

Hannsek assigned andrewkozlik Feb 8, 2023

onvej-sl previously requested changes Feb 13, 2023

View reviewed changes

andrewkozlik requested a review from matejcik May 29, 2023 15:38

matejcik approved these changes Jun 14, 2023

View reviewed changes

andrewkozlik force-pushed the andrewkozlik/1864 branch from eaa0afe to 81f081f Compare June 19, 2023 14:57

andrewkozlik added 2 commits June 21, 2023 00:04

refactor: Unify secp256k1_zkp usage.

a1a36d9

[no changelog]

feat(crypto): Support is_canonical parameter in zkp_ecdsa_sign_digest().

ad31d1b

andrewkozlik force-pushed the andrewkozlik/1864 branch from 81f081f to ad31d1b Compare June 20, 2023 22:05

andrewkozlik merged commit c3f6e8f into master Jun 21, 2023

andrewkozlik deleted the andrewkozlik/1864 branch June 21, 2023 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify secp256k1_zkp usage #2385

Unify secp256k1_zkp usage #2385

andrewkozlik commented Jul 14, 2022 •

edited

Loading

onvej-sl Jul 15, 2022

andrewkozlik Feb 16, 2023

onvej-sl May 23, 2023

andrewkozlik May 24, 2023

onvej-sl May 24, 2023

andrewkozlik May 24, 2023

onvej-sl May 24, 2023

andrewkozlik May 25, 2023

Hannsek commented Apr 14, 2023 •

edited

Loading

andrewkozlik commented Apr 14, 2023

onvej-sl commented May 26, 2023

andrewkozlik commented May 29, 2023

matejcik commented Jun 14, 2023

Unify secp256k1_zkp usage #2385

Unify secp256k1_zkp usage #2385

Conversation

andrewkozlik commented Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hannsek commented Apr 14, 2023 • edited Loading

andrewkozlik commented Apr 14, 2023

onvej-sl commented May 26, 2023

andrewkozlik commented May 29, 2023

matejcik commented Jun 14, 2023

andrewkozlik commented Jul 14, 2022 •

edited

Loading

Hannsek commented Apr 14, 2023 •

edited

Loading