From 1b29aed1283bd050ac7b782b352a7c87d88d82ab Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Mon, 16 Nov 2015 17:11:26 +0100 Subject: [PATCH 1/3] crypto/secp256k1: verify recovery ID before calling libsecp256k1 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 #1983. Also remove VerifySignature because we don't use it. --- crypto/secp256k1/secp256.go | 112 ++++++++++--------------------- crypto/secp256k1/secp256_test.go | 15 +++-- 2 files changed, 48 insertions(+), 79 deletions(-) diff --git a/crypto/secp256k1/secp256.go b/crypto/secp256k1/secp256.go index a2104016a..634e1c0b8 100644 --- a/crypto/secp256k1/secp256.go +++ b/crypto/secp256k1/secp256.go @@ -39,7 +39,6 @@ package secp256k1 import "C" import ( - "bytes" "errors" "unsafe" @@ -64,6 +63,12 @@ func init() { context = C.secp256k1_context_create(3) // SECP256K1_START_SIGN | SECP256K1_START_VERIFY } +var ( + ErrInvalidMsgLen = errors.New("invalid message length for signature recovery") + ErrInvalidSignatureLen = errors.New("invalid signature length") + ErrInvalidRecoveryID = errors.New("invalid signature recovery id") +) + func GenerateKeyPair() ([]byte, []byte) { var seckey []byte = randentropy.GetEntropyCSPRNG(32) var seckey_ptr *C.uchar = (*C.uchar)(unsafe.Pointer(&seckey[0])) @@ -177,69 +182,20 @@ func VerifySeckeyValidity(seckey []byte) error { return nil } -func VerifySignatureValidity(sig []byte) bool { - //64+1 - if len(sig) != 65 { - return false - } - //malleability check, highest bit must be 1 - if (sig[32] & 0x80) == 0x80 { - return false - } - //recovery id check - if sig[64] >= 4 { - return false - } - - return true -} - -//for compressed signatures, does not need pubkey -func VerifySignature(msg []byte, sig []byte, pubkey1 []byte) error { - if msg == nil || sig == nil || pubkey1 == nil { - return errors.New("inputs must be non-nil") - } - if len(sig) != 65 { - return errors.New("invalid signature length") - } - if len(pubkey1) != 65 { - return errors.New("Invalid public key length") - } - - //to enforce malleability, highest bit of S must be 0 - //S starts at 32nd byte - if (sig[32] & 0x80) == 0x80 { //highest bit must be 1 - return errors.New("Signature not malleable") - } - - if sig[64] >= 4 { - return errors.New("Recover byte invalid") - } - - // if pubkey recovered, signature valid - pubkey2, err := RecoverPubkey(msg, sig) - if err != nil { - return err - } - if len(pubkey2) != 65 { - return errors.New("Invalid recovered public key length") - } - if !bytes.Equal(pubkey1, pubkey2) { - return errors.New("Public key does not match recovered public key") - } - - return nil -} - -// recovers a public key from the signature +// RecoverPubkey returns the the public key of the signer. +// msg must be the 32-byte hash of the message to be signed. +// sig must be a 65-byte compact ECDSA signature containing the +// recovery id as the last element. func RecoverPubkey(msg []byte, sig []byte) ([]byte, error) { - if len(sig) != 65 { - return nil, errors.New("Invalid signature length") + if len(msg) != 32 { + return nil, ErrInvalidMsgLen + } + if err := checkSignature(sig); err != nil { + return nil, err } msg_ptr := (*C.uchar)(unsafe.Pointer(&msg[0])) sig_ptr := (*C.uchar)(unsafe.Pointer(&sig[0])) - pubkey := make([]byte, 64) /* this slice is used for both the recoverable signature and the @@ -248,17 +204,15 @@ func RecoverPubkey(msg []byte, sig []byte) ([]byte, error) { pubkey recovery is one bottleneck during load in Ethereum */ bytes65 := make([]byte, 65) - pubkey_ptr := (*C.secp256k1_pubkey)(unsafe.Pointer(&pubkey[0])) recoverable_sig_ptr := (*C.secp256k1_ecdsa_recoverable_signature)(unsafe.Pointer(&bytes65[0])) - recid := C.int(sig[64]) + ret := C.secp256k1_ecdsa_recoverable_signature_parse_compact( context, recoverable_sig_ptr, sig_ptr, recid) - if ret == C.int(0) { return nil, errors.New("Failed to parse signature") } @@ -269,20 +223,28 @@ func RecoverPubkey(msg []byte, sig []byte) ([]byte, error) { recoverable_sig_ptr, msg_ptr, ) - if ret == C.int(0) { return nil, errors.New("Failed to recover public key") - } else { - serialized_pubkey_ptr := (*C.uchar)(unsafe.Pointer(&bytes65[0])) - - var output_len C.size_t - C.secp256k1_ec_pubkey_serialize( // always returns 1 - context, - serialized_pubkey_ptr, - &output_len, - pubkey_ptr, - 0, // SECP256K1_EC_COMPRESSED - ) - return bytes65, nil } + + serialized_pubkey_ptr := (*C.uchar)(unsafe.Pointer(&bytes65[0])) + var output_len C.size_t + C.secp256k1_ec_pubkey_serialize( // always returns 1 + context, + serialized_pubkey_ptr, + &output_len, + pubkey_ptr, + 0, // SECP256K1_EC_COMPRESSED + ) + return bytes65, nil +} + +func checkSignature(sig []byte) error { + if len(sig) != 65 { + return ErrInvalidSignatureLen + } + if sig[64] >= 4 { + return ErrInvalidRecoveryID + } + return nil } diff --git a/crypto/secp256k1/secp256_test.go b/crypto/secp256k1/secp256_test.go index 45c448f3c..cb71ea5e7 100644 --- a/crypto/secp256k1/secp256_test.go +++ b/crypto/secp256k1/secp256_test.go @@ -56,6 +56,17 @@ func TestSignatureValidity(t *testing.T) { } } +func TestInvalidRecoveryID(t *testing.T) { + _, seckey := GenerateKeyPair() + msg := randentropy.GetEntropyCSPRNG(32) + sig, _ := Sign(msg, seckey) + sig[64] = 99 + _, err := RecoverPubkey(msg, sig) + if err != ErrInvalidRecoveryID { + t.Fatalf("got %q, want %q", err, ErrInvalidRecoveryID) + } +} + func TestSignAndRecover(t *testing.T) { pubkey1, seckey := GenerateKeyPair() msg := randentropy.GetEntropyCSPRNG(32) @@ -70,10 +81,6 @@ func TestSignAndRecover(t *testing.T) { if !bytes.Equal(pubkey1, pubkey2) { t.Errorf("pubkey mismatch: want: %x have: %x", pubkey1, pubkey2) } - err = VerifySignature(msg, sig, pubkey1) - if err != nil { - t.Errorf("signature verification error: %s", err) - } } func TestRandomMessagesWithSameKey(t *testing.T) { From 5159f8f6493bfa3cfb0ee3b1517f4576d4c9d2de Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Mon, 16 Nov 2015 18:04:56 +0100 Subject: [PATCH 2/3] crypto/secp256k1: raise internal errors as recoverable Go panic --- crypto/secp256k1/panic_cb.go | 33 +++++++++++++++++++++++++++++++++ crypto/secp256k1/secp256.go | 6 ++++++ 2 files changed, 39 insertions(+) create mode 100644 crypto/secp256k1/panic_cb.go diff --git a/crypto/secp256k1/panic_cb.go b/crypto/secp256k1/panic_cb.go new file mode 100644 index 000000000..e0e9034ee --- /dev/null +++ b/crypto/secp256k1/panic_cb.go @@ -0,0 +1,33 @@ +// Copyright 2015 The go-ethereum Authors +// This file is part of the go-ethereum library. +// +// The go-ethereum library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The go-ethereum library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the go-ethereum library. If not, see . + +package secp256k1 + +import "C" +import "unsafe" + +// Callbacks for converting libsecp256k1 internal faults into +// recoverable Go panics. + +//export secp256k1GoPanicIllegal +func secp256k1GoPanicIllegal(msg *C.char, data unsafe.Pointer) { + panic("illegal argument: " + C.GoString(msg)) +} + +//export secp256k1GoPanicError +func secp256k1GoPanicError(msg *C.char, data unsafe.Pointer) { + panic("internal error: " + C.GoString(msg)) +} diff --git a/crypto/secp256k1/secp256.go b/crypto/secp256k1/secp256.go index 634e1c0b8..4269b6a8c 100644 --- a/crypto/secp256k1/secp256.go +++ b/crypto/secp256k1/secp256.go @@ -35,6 +35,10 @@ package secp256k1 #define NDEBUG #include "./libsecp256k1/src/secp256k1.c" #include "./libsecp256k1/src/modules/recovery/main_impl.h" + +typedef void (*callbackFunc) (const char* msg, void* data); +extern void secp256k1GoPanicIllegal(const char* msg, void* data); +extern void secp256k1GoPanicError(const char* msg, void* data); */ import "C" @@ -61,6 +65,8 @@ var context *C.secp256k1_context func init() { // around 20 ms on a modern CPU. context = C.secp256k1_context_create(3) // SECP256K1_START_SIGN | SECP256K1_START_VERIFY + C.secp256k1_context_set_illegal_callback(context, C.callbackFunc(C.secp256k1GoPanicIllegal), nil) + C.secp256k1_context_set_error_callback(context, C.callbackFunc(C.secp256k1GoPanicError), nil) } var ( From e344e1d4903f47d525258305d986f4aa0dd0ab52 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Mon, 16 Nov 2015 18:23:51 +0100 Subject: [PATCH 3/3] crypto/secp256k1: drop pkgsrc paths from CFLAGS They cause compiler warnings for people who don't have these directories. People with pkgsrc can add the directory through CGO_CFLAGS instead. --- crypto/secp256k1/secp256.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/secp256k1/secp256.go b/crypto/secp256k1/secp256.go index 4269b6a8c..41a5608a5 100644 --- a/crypto/secp256k1/secp256.go +++ b/crypto/secp256k1/secp256.go @@ -20,11 +20,11 @@ package secp256k1 /* #cgo CFLAGS: -I./libsecp256k1 -#cgo darwin CFLAGS: -I/usr/local/include -I/opt/pkg/include +#cgo darwin CFLAGS: -I/usr/local/include #cgo freebsd CFLAGS: -I/usr/local/include #cgo linux,arm CFLAGS: -I/usr/local/arm/include #cgo LDFLAGS: -lgmp -#cgo darwin LDFLAGS: -L/usr/local/lib -L/opt/pkg/lib +#cgo darwin LDFLAGS: -L/usr/local/lib #cgo freebsd LDFLAGS: -L/usr/local/lib #cgo linux,arm LDFLAGS: -L/usr/local/arm/lib #define USE_NUM_GMP