Merge pull request #1984 from fjl/secp256k1-recover-id-verify

crypto/secp256k1: verify recovery ID before calling libsecp256k1

3 files changed, 89 insertions, 81 deletions

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 <http://www.gnu.org/licenses/>.
+
+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 a2104016a..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
@@ -35,11 +35,14 @@ 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"
 
 import (
-	"bytes"
 	"errors"
 	"unsafe"
 
@@ -62,8 +65,16 @@ 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 (
+	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 +188,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 +210,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 +229,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) {