Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Clearing up the reason behind "Ethereum Signed Message" #8828

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Jun 6, 2018

I think this makes more sense.
First because there aren't really chosen-plaintext attacks against asymmetric encryption keys out there.
Second because faking a transaction to be signed by the user is a viable attack vector.

@parity-cla-bot
Copy link

It looks like @elichai hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@elichai
Copy link
Contributor Author

elichai commented Jun 6, 2018

[clabot:check].

@parity-cla-bot
Copy link

It looks like @elichai signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M3-docs 📑 Documentation. labels Jun 6, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 6, 2018
@@ -237,7 +237,7 @@ pub fn fetch_gas_price_corpus(

/// Returns a eth_sign-compatible hash of data to sign.
/// The data is prepended with special message to prevent
/// chosen-plaintext attacks.
/// malicious DApps from using the function to sign forge transactions.
Copy link
Contributor

Choose a reason for hiding this comment

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

forged transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Don't know how it happened 👍

@@ -223,7 +223,7 @@ pub fn fetch_gas_price_corpus(
.map(move |prices| {
// produce a corpus from the vector and cache it.
// It's later used to get a percentile for default gas price.
let corpus: ::stats::Corpus<_> = prices.into();
let corpus: ::stats::Corpus<_> =forge prices.into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is mistake?

Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Revert the changes in rpc/src/v1/helpers/dispatch.rs and it's good to merge!

@niklasad1 niklasad1 added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Jun 10, 2018
@elichai elichai force-pushed the master branch 2 times, most recently from bb675e4 to 46bf82e Compare June 10, 2018 15:04
@elichai
Copy link
Contributor Author

elichai commented Jun 10, 2018

Reverted. now it's good and clean.

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 10, 2018
@5chdn 5chdn merged commit 986f485 into openethereum:master Jun 11, 2018
dvdplm added a commit that referenced this pull request Jun 11, 2018
* master:
  Fix subcrate test compile (#8862)
  network-devp2p: downgrade logging to debug, add target (#8784)
  Clearing up a comment about the prefix for signing (#8828)
  Disable parallel verification and skip verifiying already imported txs. (#8834)
  devp2p: Move UDP socket handling from Discovery to Host. (#8790)
  Fixed AuthorityRound deadlock on shutdown, closes #8088 (#8803)
  Specify critical release flag per network (#8821)
  Fix `deadlock_detection` feature branch compilation (#8824)
  Use system allocator when profiling memory (#8831)
  added from and to to Receipt (#8756)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. M3-docs 📑 Documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants