Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use domain separation for wallet message signing #5400

Merged

Conversation

AaronFeickert
Copy link
Collaborator

Description

Uses domain separation for wallet message Schnorr signatures to prevent context misuse. Supersedes PR 5394.

Motivation and Context

Wallets can sign and verify arbitrary messages using their secret keys. Because such messages are signed using Schnorr signatures with a default Fiat-Shamir challenge hash domain, it may be possible to replay them in different contexts, with possibly dangerous consequences.

Another pending PR suggests prepending fixed data to wallet messages, but this can still lead to collisions.

Fortunately, the tari-crypto signature API provides a handy type alias that makes domain separation straightforward. Using this, it is not possible to produce signature collisions (up to the collision resistance of the hash function itself). This is a safer and more idiomatic design.

How Has This Been Tested?

An existing test passes.

What process can a PR reviewer use to test or verify this change?

Confirm that the tari-crypto signature API is being used correctly, and that the macro-derived domain separator is unique within the codebase.

Breaking Changes

Existing signatures will fail to verify, but this is unlikely to be problematic.

@AaronFeickert AaronFeickert force-pushed the wallet-message-signing branch 2 times, most recently from ce1cfd5 to cb338c9 Compare May 19, 2023 17:55
@AaronFeickert AaronFeickert force-pushed the wallet-message-signing branch from cb338c9 to d6e7927 Compare May 19, 2023 17:56
@AaronFeickert AaronFeickert marked this pull request as ready for review May 19, 2023 17:56
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It does potentially limit verifying in non-rust code, but I don't think that is a usecase for this in particular, and can be resolved with a WASM build at worst

@ghpbot-tari-project ghpbot-tari-project added the P-acks_required Process - Requires more ACKs or utACKs label May 22, 2023
@github-actions
Copy link

Test Results (CI)

1 141 tests  ±0   1 141 ✔️ ±0   9m 5s ⏱️ -5s
     37 suites ±0          0 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit d6e7927. ± Comparison against base commit abe0e6c.

@github-actions
Copy link

Test Results (Integration tests)

  2 files  +  2  12 suites  +12   41m 5s ⏱️ + 41m 5s
28 tests +28  26 ✔️ +26  0 💤 ±0  2 +2 
31 runs  +31  28 ✔️ +28  0 💤 ±0  3 +3 

For more details on these failures, see this check.

Results for commit d6e7927. ± Comparison against base commit abe0e6c.

@SWvheerden SWvheerden merged commit 7d71f8b into tari-project:development May 22, 2023
@AaronFeickert
Copy link
Collaborator Author

AaronFeickert commented May 22, 2023

Nice. It does potentially limit verifying in non-rust code, but I don't think that is a usecase for this in particular, and can be resolved with a WASM build at worst

Well, and there isn't a common standard for Schnorr-like signatures over arbitrary curves. Domain separation is handled in a really haphazard (and sometimes nonexistent) way across standards including things like RFC 8032. Writing a compatible verifier outside of Rust wouldn't be that tricky,

@AaronFeickert AaronFeickert deleted the wallet-message-signing branch May 22, 2023 12:55
@AaronFeickert
Copy link
Collaborator Author

While it's not directly applicable here, this is a good read on how subtle even something as seemingly simple as ed25519 verification can get, and a good lesson on the importance of careful standardization.

SWvheerden added a commit that referenced this pull request May 29, 2023
##
[0.50.0-pre.2](v0.50.0-pre.1...v0.50.0-pre.2)
(2023-05-29)

### ⚠ BREAKING CHANGES

* add optional range proof types (5372)

### Features

* add metadata signature check
([5411](#5411))
([9c2bf41](9c2bf41))
* add optional range proof types
([5372](#5372))
([f24784f](f24784f))
* added burn feature to the console wallet
([5322](#5322))
([45685b9](45685b9))
* improved base node monitoring
([5390](#5390))
([c704890](c704890))


### Bug Fixes

* **comms:** only set final forward address if configured to port 0
([5406](#5406))
([ff7fb6d](ff7fb6d))
* deeplink to rfc spec
([5342](#5342))
([806d3b8](806d3b8))
* don't use in memory datastores for chat client dht in integration
tests ([#5399](#5399))
([cbdca6f](cbdca6f))
* fix panic when no public addresses
([5367](#5367))
([49be2a2](49be2a2))
* loop on mismatched passphrase entry
([5396](#5396))
([ed120b2](ed120b2))
* use domain separation for wallet message signing
([5400](#5400))
([7d71f8b](7d71f8b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants