Allow pubkey recovery for all-zero messages#369
Merged
sorpaas merged 7 commits intoApr 10, 2020
Conversation
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.
Contributor
Author
|
The alternative to this is to add an all-zeros-allowed version of |
ordian
reviewed
Apr 9, 2020
ordian
approved these changes
Apr 9, 2020
ordian
reviewed
Apr 9, 2020
Contributor
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. |
Co-Authored-By: Andronik Ordian <write@reusable.software>
…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
Contributor
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. |
Contributor
Author
Revert `recover()` to previous behaviour: no zero-messages allowed Docs and cleanup
ordian
approved these changes
Apr 10, 2020
sorpaas
approved these changes
Apr 10, 2020
sorpaas
deleted the
dp/fix/change-parity-crypto-to-allow-all-zeroes-messages
branch
ordian
added a commit
that referenced
this pull request
Apr 22, 2020
* 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
* 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) ...
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
3 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.
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
ecrecoverbuiltin 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
ZeroesAllowedMessagewrapper around it. TheZeroesAllowedMessageimplements theThirtyTwoByteHashtrait fromrust-secp256k1which circumvents the zero-check.In a follow-up PR we'll likely change the interface of
recover()to take aZeroesAllowedMessagedirectly, thus removing the unneeded clone.