Skip to content

crypto: ensure that VerifySignature rejects malleable signatures#15708

Merged
karalabe merged 3 commits into
ethereum:masterfrom
fjl:crypto-verify-malleable
Dec 20, 2017
Merged

crypto: ensure that VerifySignature rejects malleable signatures#15708
karalabe merged 3 commits into
ethereum:masterfrom
fjl:crypto-verify-malleable

Conversation

@fjl
Copy link
Copy Markdown
Contributor

@fjl fjl commented Dec 19, 2017

It already rejected them when using libsecp256k1, make sure the nocgo
version does the same thing.

It already rejected them when using libsecp256k1, make sure the nocgo
version does the same thing.
@holiman
Copy link
Copy Markdown
Contributor

holiman commented Dec 19, 2017

This change hardcodes homestead to true - could there be any backwards incompatible effects due to this, where we reject historical transaction if we do a full-sync from scratch ?

@fjl
Copy link
Copy Markdown
Contributor Author

fjl commented Dec 19, 2017

VerifySignature is a new function and is not used anywhere in the codebase, so I doubt that there will be any issues with consensus.

Comment thread crypto/signature_nocgo.go
}
if !ValidateSignatureValues(0, sig.R, sig.S, true) {
return false
}
Copy link
Copy Markdown
Member

@karalabe karalabe Dec 20, 2017

Choose a reason for hiding this comment

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

Out of curiosity, why is the v always 0 here?

@fjl
Copy link
Copy Markdown
Contributor Author

fjl commented Dec 20, 2017

There seems to be a lot of confusion about the use of ValidateSignatureValues here. The check is simpler now, PTAL.

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@karalabe karalabe added this to the 1.8.0 milestone Dec 20, 2017
@karalabe karalabe merged commit ce823c9 into ethereum:master Dec 20, 2017
b00ris pushed a commit to b00ris/go-ethereum that referenced this pull request Jan 19, 2018
…ereum#15708)

* crypto: ensure that VerifySignature rejects malleable signatures

It already rejected them when using libsecp256k1, make sure the nocgo
version does the same thing.

* crypto: simplify check

* crypto: fix build
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
…ereum#15708)

* crypto: ensure that VerifySignature rejects malleable signatures

It already rejected them when using libsecp256k1, make sure the nocgo
version does the same thing.

* crypto: simplify check

* crypto: fix build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants