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

crypto/secp256k1: verify recovery ID before calling libsecp256k1 #1984

Merged
merged 3 commits into from
Nov 17, 2015

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Nov 16, 2015

The C library treats the recovery ID as trusted input and crashes
the process for invalid values, so it needs to be verified before
calling into C.

@fjl fjl added this to the 1.4.0 milestone Nov 16, 2015
@robotally
Copy link

Vote Count Reviewers
👍 2 @Gustav-Simonsson @obscuren
👎 0

Updated: Mon Nov 16 18:34:54 UTC 2015

func RecoverPubkey(msg []byte, sig []byte) ([]byte, error) {
if len(sig) != 65 {
return nil, errors.New("Invalid signature length")
if err := checkSignature(sig); err != nil {

Choose a reason for hiding this comment

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

While we're at it, let's error if len(msg) != 32

@fjl fjl force-pushed the secp256k1-recover-id-verify branch from 0e3207b to 04a790d Compare November 16, 2015 17:27
@codecov-io
Copy link

Current coverage is 46.57%

Merging #1984 into develop will increase coverage by +0.08% as of 1cd93c9

Powered by Codecov. Updated on successful CI builds.

@Gustav-Simonsson
Copy link

👍

1 similar comment
@obscuren
Copy link
Contributor

👍

fjl added 3 commits November 17, 2015 09:51
The C library treats the recovery ID as trusted input and crashes
the process for invalid values, so it needs to be verified before
calling into C. This will inhibit the crash in ethereum#1983.

Also remove VerifySignature because we don't use it.
They cause compiler warnings for people who don't have these
directories. People with pkgsrc can add the directory through CGO_CFLAGS
instead.
@fjl fjl force-pushed the secp256k1-recover-id-verify branch from 04a790d to e344e1d Compare November 17, 2015 08:53
obscuren added a commit that referenced this pull request Nov 17, 2015
crypto/secp256k1: verify recovery ID before calling libsecp256k1
@obscuren obscuren merged commit 10475f4 into ethereum:develop Nov 17, 2015
@obscuren obscuren removed the review label Nov 17, 2015
@obscuren obscuren modified the milestones: 1.4.0, 1.3.2 Nov 24, 2015
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.

5 participants