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.

Sign up for GitHub

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

Jump to bottom

Allow pubkey recovery for all-zero messages #369

Merged
merged 7 commits into from
Apr 10, 2020
Merged

Allow pubkey recovery for all-zero messages #369

merged 7 commits into from
Apr 10, 2020

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Apr 9, 2020 •
edited
Loading

After https://github.com/openethereum/openethereum/pull/11406 it is no longer possible to do public key recovery from messages that are all-zero. This creates issues when using the ecrecover builtin because externally produced signatures may well provide a message (i.e. a preimage) that is all-zeroes.
This works around the problem at the cost of cloning the incoming message and create a ZeroesAllowedMessage wrapper around it. The ZeroesAllowedMessage implements the ThirtyTwoByteHash trait from rust-secp256k1 which circumvents the zero-check.

In a follow-up PR we'll likely change the interface of recover() to take a ZeroesAllowedMessage directly, thus removing the unneeded clone.

@dvdplm
Allow pubkey recovery for all-zero messages
6a14615 
After https://github.com/openethereum/openethereum/pull/11406 it is no longer possible to to public key recovery from messages that are all-zero. This create issues when using the `ecrecover` builtin because externally produced signatures may well provide a message (i.e. a preimage) that is all-zeroes.
This works around the problem at the cost of cloning the incoming message and create a `ZeroesAllowedMessage` wrapper around it. The `ZeroesAllowedMessage` implements the `ThirtyTwoByteHash` trait from `rust-secp256k1` which circumvents the zero-check.

In a follow-up PR we'll likely change the interface of `recover()` to take a `ZeroesAllowedMessage` directly, thus removing the unneeded clone.
@dvdplm dvdplm requested review from ordian and sorpaas April 9, 2020 21:01
@dvdplm dvdplm self-assigned this Apr 9, 2020
@dvdplm
Copy link
Contributor Author

dvdplm commented Apr 9, 2020

The alternative to this is to add an all-zeros-allowed version of recover() and use that for the ecrecover precompile. It has the advantage of not being a breaking change and it isolates this peculiar use-case from more normal usage. Thoughts?

@ordian
Copy link
Member

ordian commented Apr 9, 2020

The alternative to this is to add an all-zeros-allowed version of recover() and use that for the ecrecover precompile. It has the advantage of not being a breaking change and it isolates this peculiar use-case from more normal usage. Thoughts?

I think this change can be viewed as a bug fix (as we changed the behavior for zero messages unintentionally) and thus is not a breaking change.

dvdplm and others added 3 commits April 10, 2020 00:07
@dvdplm @ordian
Update parity-crypto/src/publickey/ecdsa_signature.rs
31a14e7 
Co-Authored-By: Andronik Ordian <[email protected]>
@dvdplm
Docs and review grumbles
0ad4caa
@dvdplm
Merge branch 'dp/fix/change-parity-crypto-to-allow-all-zeroes-message…
7568cd6 
…s' of github.com:paritytech/parity-common into dp/fix/change-parity-crypto-to-allow-all-zeroes-messages

* 'dp/fix/change-parity-crypto-to-allow-all-zeroes-messages' of github.com:paritytech/parity-common:
  Update parity-crypto/src/publickey/ecdsa_signature.rs
@sorpaas
Copy link
Member

sorpaas commented Apr 10, 2020

The alternative to this is to add an all-zeros-allowed version of recover() and use that for the ecrecover precompile. It has the advantage of not being a breaking change and it isolates this peculiar use-case from more normal usage. Thoughts?

TBH I really think this is a better idea. All-zero message is not safe after all, and not allowing it in default function can prevent misuses.

@dvdplm
Copy link
Contributor Author

dvdplm commented Apr 10, 2020

Ok, thank you @sorpaas @ordian. I'll refactor this to add a new method.

dvdplm added 2 commits April 10, 2020 10:45
@dvdplm
Add recover_allowing_all_zero_message()
88f3d44 
Revert `recover()` to previous behaviour: no zero-messages allowed
Docs and cleanup
@dvdplm
Obey the fmt
634ecb5
@dvdplm dvdplm marked this pull request as ready for review April 10, 2020 09:50
@dvdplm dvdplm requested review from ordian and niklasad1 April 10, 2020 09:50
@sorpaas sorpaas merged commit b87aee1 into master Apr 10, 2020
@sorpaas sorpaas deleted the dp/fix/change-parity-crypto-to-allow-all-zeroes-messages branch April 10, 2020 10:43
@dvdplm dvdplm mentioned this pull request Apr 10, 2020
Merged
ordian added a commit that referenced this pull request Apr 14, 2020
@ordian
Merge branch 'master' into ao-prefix-iter-optimized
ddb330e 
* master:
  keccak-hash: fix bench and add one for range (#372)
  [parity-crypto] Release 0.6.1 (#373)
  keccak-hash: bump version to 0.5.1 (#371)
  keccak-hash: add keccak256_range and keccak512_range functions (#370)
  Allow pubkey recovery for all-zero messages (#369)
ordian added a commit that referenced this pull request Apr 22, 2020
@ordian
Merge branch 'master' into ao-rocksdb-new-format
08af3ed 
* master:
  kvdb-rocksdb: optimize and rename iter_from_prefix  (#365)
  bump parity-util-mem (#376)
  parity-util-mem: fix for windows (#375)
  keccak-hash: fix bench and add one for range (#372)
  [parity-crypto] Release 0.6.1 (#373)
  keccak-hash: bump version to 0.5.1 (#371)
  keccak-hash: add keccak256_range and keccak512_range functions (#370)
  Allow pubkey recovery for all-zero messages (#369)
  Delete by prefix operator in kvdb (#360)
  kvdb: no overlay (#313)
  Ban duplicates of parity-uil-mem from being linked into the same program (#363)
  Use correct license ID (#362)
  Memtest example for Rocksdb (#349)
  Prep for release (#361)
  parity-util-mem: prepare release for 0.5.2 (#359)
  travis: test parity-util-mem on android (#358)
  parity-util-mem: update mimalloc feature (#352)
  kvdb: remove parity-bytes dependency (#351)
  parity-util-mem: use malloc for usable_size on android (#355)
  CI: troubleshoot macOS build (#356)
ordian added a commit that referenced this pull request May 5, 2020
@ordian
Merge branch 'master' into ao-type-metadata
dc92ac3 
* master: (56 commits)
  primitive-types: add no_std support for serde feature (#385)
  Add Rocksdb Secondary Instance Api (#384)
  kvdb-rocksdb: update rocksdb to 0.14 (#379)
  prepare releases for a few crates (#382)
  uint: fix UB in uint::from_big_endian (#381)
  Fix limit prefix delete case (#368)
  Add arbitrary trait implementation (#378)
  kvdb-rocksdb: optimize and rename iter_from_prefix  (#365)
  bump parity-util-mem (#376)
  parity-util-mem: fix for windows (#375)
  keccak-hash: fix bench and add one for range (#372)
  [parity-crypto] Release 0.6.1 (#373)
  keccak-hash: bump version to 0.5.1 (#371)
  keccak-hash: add keccak256_range and keccak512_range functions (#370)
  Allow pubkey recovery for all-zero messages (#369)
  Delete by prefix operator in kvdb (#360)
  kvdb: no overlay (#313)
  Ban duplicates of parity-uil-mem from being linked into the same program (#363)
  Use correct license ID (#362)
  Memtest example for Rocksdb (#349)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorpaas sorpaas sorpaas approved these changes

@ordian ordian ordian approved these changes

@niklasad1 niklasad1 Awaiting requested review from niklasad1

Assignees

@dvdplm dvdplm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdplm @ordian @sorpaas