crypto: replace ToECDSAPub with error-checking func UnmarshalPubkey (#16932)

fjl · gballet · commit 0255951587ef · 2018-06-12T15:26:08.000+02:00
ToECDSAPub was unsafe because it returned a non-nil key with nil X, Y in
case of invalid input. This change replaces ToECDSAPub with
UnmarshalPubkey across the codebase.
diff --git a/cmd/wnode/main.go b/cmd/wnode/main.go
@@ -140,8 +140,8 @@ func processArgs() {
 	}
 
 	if *asymmetricMode && len(*argPub) > 0 {
-		pub = crypto.ToECDSAPub(common.FromHex(*argPub))
-		if !isKeyValid(pub) {
+		var err error
+		if pub, err = crypto.UnmarshalPubkey(common.FromHex(*argPub)); err != nil {
 			utils.Fatalf("invalid public key")
 		}
 	}
@@ -321,10 +321,6 @@ func startServer() error {
 	return nil
 }
 
-func isKeyValid(k *ecdsa.PublicKey) bool {
-	return k.X != nil && k.Y != nil
-}
-
 func configureNode() {
 	var err error
 	var p2pAccept bool
@@ -340,9 +336,8 @@ func configureNode() {
 			if b == nil {
 				utils.Fatalf("Error: can not convert hexadecimal string")
 			}
-			pub = crypto.ToECDSAPub(b)
-			if !isKeyValid(pub) {
-				utils.Fatalf("Error: invalid public key")
+			if pub, err = crypto.UnmarshalPubkey(b); err != nil {
+				utils.Fatalf("Error: invalid peer public key")
 			}
 		}
 	}
diff --git a/crypto/crypto.go b/crypto/crypto.go
@@ -39,6 +39,8 @@ var (
 	secp256k1halfN = new(big.Int).Div(secp256k1N, big.NewInt(2))
 )
 
+var errInvalidPubkey = errors.New("invalid secp256k1 public key")
+
 // Keccak256 calculates and returns the Keccak256 hash of the input data.
 func Keccak256(data ...[]byte) []byte {
 	d := sha3.NewKeccak256()
@@ -122,12 +124,13 @@ func FromECDSA(priv *ecdsa.PrivateKey) []byte {
 	return math.PaddedBigBytes(priv.D, priv.Params().BitSize/8)
 }
 
-func ToECDSAPub(pub []byte) *ecdsa.PublicKey {
-	if len(pub) == 0 {
-		return nil
-	}
+// UnmarshalPubkey converts bytes to a secp256k1 public key.
+func UnmarshalPubkey(pub []byte) (*ecdsa.PublicKey, error) {
 	x, y := elliptic.Unmarshal(S256(), pub)
-	return &ecdsa.PublicKey{Curve: S256(), X: x, Y: y}
+	if x == nil {
+		return nil, errInvalidPubkey
+	}
+	return &ecdsa.PublicKey{Curve: S256(), X: x, Y: y}, nil
 }
 
 func FromECDSAPub(pub *ecdsa.PublicKey) []byte {
diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go
@@ -23,9 +23,11 @@ import (
 	"io/ioutil"
 	"math/big"
 	"os"
+	"reflect"
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
 )
 
 var testAddrHex = "970e8128ab834e8eac17ab8e3812f010678cf791"
@@ -56,6 +58,33 @@ func BenchmarkSha3(b *testing.B) {
 	}
 }
 
+func TestUnmarshalPubkey(t *testing.T) {
+	key, err := UnmarshalPubkey(nil)
+	if err != errInvalidPubkey || key != nil {
+		t.Fatalf("expected error, got %v, %v", err, key)
+	}
+	key, err = UnmarshalPubkey([]byte{1, 2, 3})
+	if err != errInvalidPubkey || key != nil {
+		t.Fatalf("expected error, got %v, %v", err, key)
+	}
+
+	var (
+		enc, _ = hex.DecodeString("04760c4460e5336ac9bbd87952a3c7ec4363fc0a97bd31c86430806e287b437fd1b01abc6e1db640cf3106b520344af1d58b00b57823db3e1407cbc433e1b6d04d")
+		dec    = &ecdsa.PublicKey{
+			Curve: S256(),
+			X:     hexutil.MustDecodeBig("0x760c4460e5336ac9bbd87952a3c7ec4363fc0a97bd31c86430806e287b437fd1"),
+			Y:     hexutil.MustDecodeBig("0xb01abc6e1db640cf3106b520344af1d58b00b57823db3e1407cbc433e1b6d04d"),
+		}
+	)
+	key, err = UnmarshalPubkey(enc)
+	if err != nil {
+		t.Fatalf("expected no error, got %v", err)
+	}
+	if !reflect.DeepEqual(key, dec) {
+		t.Fatal("wrong result")
+	}
+}
+
 func TestSign(t *testing.T) {
 	key, _ := HexToECDSA(testPrivHex)
 	addr := common.HexToAddress(testAddrHex)
@@ -69,7 +98,7 @@ func TestSign(t *testing.T) {
 	if err != nil {
 		t.Errorf("ECRecover error: %s", err)
 	}
-	pubKey := ToECDSAPub(recoveredPub)
+	pubKey, _ := UnmarshalPubkey(recoveredPub)
 	recoveredAddr := PubkeyToAddress(*pubKey)
 	if addr != recoveredAddr {
 		t.Errorf("Address mismatch: want: %x have: %x", addr, recoveredAddr)
diff --git a/internal/ethapi/api.go b/internal/ethapi/api.go
@@ -461,13 +461,11 @@ func (s *PrivateAccountAPI) EcRecover(ctx context.Context, data, sig hexutil.Byt
 	}
 	sig[64] -= 27 // Transform yellow paper V from 27/28 to 0/1
 
-	rpk, err := crypto.Ecrecover(signHash(data), sig)
+	rpk, err := crypto.SigToPub(signHash(data), sig)
 	if err != nil {
 		return common.Address{}, err
 	}
-	pubKey := crypto.ToECDSAPub(rpk)
-	recoveredAddr := crypto.PubkeyToAddress(*pubKey)
-	return recoveredAddr, nil
+	return crypto.PubkeyToAddress(*rpk), nil
 }
 
 // SignAndSendTransaction was renamed to SendTransaction. This method is deprecated
diff --git a/p2p/rlpx.go b/p2p/rlpx.go
@@ -528,9 +528,9 @@ func importPublicKey(pubKey []byte) (*ecies.PublicKey, error) {
 		return nil, fmt.Errorf("invalid public key length %v (expect 64/65)", len(pubKey))
 	}
 	// TODO: fewer pointless conversions
-	pub := crypto.ToECDSAPub(pubKey65)
-	if pub.X == nil {
-		return nil, fmt.Errorf("invalid public key")
+	pub, err := crypto.UnmarshalPubkey(pubKey65)
+	if err != nil {
+		return nil, err
 	}
 	return ecies.ImportECDSAPublic(pub), nil
 }
diff --git a/signer/core/api.go b/signer/core/api.go
@@ -432,13 +432,11 @@ func (api *SignerAPI) EcRecover(ctx context.Context, data, sig hexutil.Bytes) (c
 	}
 	sig[64] -= 27 // Transform yellow paper V from 27/28 to 0/1
 	hash, _ := SignHash(data)
-	rpk, err := crypto.Ecrecover(hash, sig)
+	rpk, err := crypto.SigToPub(hash, sig)
 	if err != nil {
 		return common.Address{}, err
 	}
-	pubKey := crypto.ToECDSAPub(rpk)
-	recoveredAddr := crypto.PubkeyToAddress(*pubKey)
-	return recoveredAddr, nil
+	return crypto.PubkeyToAddress(*rpk), nil
 }
 
 // SignHash is a helper function that calculates a hash for the given message that can be
diff --git a/swarm/services/swap/swap.go b/swarm/services/swap/swap.go
@@ -19,6 +19,7 @@ package swap
 import (
 	"context"
 	"crypto/ecdsa"
+	"errors"
 	"fmt"
 	"math/big"
 	"os"
@@ -134,6 +135,11 @@ func NewSwap(local *SwapParams, remote *SwapProfile, backend chequebook.Backend,
 		out *chequebook.Outbox
 	)
 
+	remotekey, err := crypto.UnmarshalPubkey(common.FromHex(remote.PublicKey))
+	if err != nil {
+		return nil, errors.New("invalid remote public key")
+	}
+
 	// check if remote chequebook is valid
 	// insolvent chequebooks suicide so will signal as invalid
 	// TODO: monitoring a chequebooks events
@@ -142,7 +148,7 @@ func NewSwap(local *SwapParams, remote *SwapProfile, backend chequebook.Backend,
 		log.Info(fmt.Sprintf("invalid contract %v for peer %v: %v)", remote.Contract.Hex()[:8], proto, err))
 	} else {
 		// remote contract valid, create inbox
-		in, err = chequebook.NewInbox(local.privateKey, remote.Contract, local.Beneficiary, crypto.ToECDSAPub(common.FromHex(remote.PublicKey)), backend)
+		in, err = chequebook.NewInbox(local.privateKey, remote.Contract, local.Beneficiary, remotekey, backend)
 		if err != nil {
 			log.Warn(fmt.Sprintf("unable to set up inbox for chequebook contract %v for peer %v: %v)", remote.Contract.Hex()[:8], proto, err))
 		}
diff --git a/whisper/whisperv5/api.go b/whisper/whisperv5/api.go
@@ -252,8 +252,7 @@ func (api *PublicWhisperAPI) Post(ctx context.Context, req NewMessage) (bool, er
 
 	// Set asymmetric key that is used to encrypt the message
 	if pubKeyGiven {
-		params.Dst = crypto.ToECDSAPub(req.PublicKey)
-		if !ValidatePublicKey(params.Dst) {
+		if params.Dst, err = crypto.UnmarshalPubkey(req.PublicKey); err != nil {
 			return false, ErrInvalidPublicKey
 		}
 	}
@@ -329,8 +328,7 @@ func (api *PublicWhisperAPI) Messages(ctx context.Context, crit Criteria) (*rpc.
 	}
 
 	if len(crit.Sig) > 0 {
-		filter.Src = crypto.ToECDSAPub(crit.Sig)
-		if !ValidatePublicKey(filter.Src) {
+		if filter.Src, err = crypto.UnmarshalPubkey(crit.Sig); err != nil {
 			return nil, ErrInvalidSigningPubKey
 		}
 	}
@@ -513,8 +511,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) {
 	}
 
 	if len(req.Sig) > 0 {
-		src = crypto.ToECDSAPub(req.Sig)
-		if !ValidatePublicKey(src) {
+		if src, err = crypto.UnmarshalPubkey(req.Sig); err != nil {
 			return "", ErrInvalidSigningPubKey
 		}
 	}
diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go
@@ -272,8 +272,7 @@ func (api *PublicWhisperAPI) Post(ctx context.Context, req NewMessage) (hexutil.
 
 	// Set asymmetric key that is used to encrypt the message
 	if pubKeyGiven {
-		params.Dst = crypto.ToECDSAPub(req.PublicKey)
-		if !ValidatePublicKey(params.Dst) {
+		if params.Dst, err = crypto.UnmarshalPubkey(req.PublicKey); err != nil {
 			return nil, ErrInvalidPublicKey
 		}
 	}
@@ -360,8 +359,7 @@ func (api *PublicWhisperAPI) Messages(ctx context.Context, crit Criteria) (*rpc.
 	}
 
 	if len(crit.Sig) > 0 {
-		filter.Src = crypto.ToECDSAPub(crit.Sig)
-		if !ValidatePublicKey(filter.Src) {
+		if filter.Src, err = crypto.UnmarshalPubkey(crit.Sig); err != nil {
 			return nil, ErrInvalidSigningPubKey
 		}
 	}
@@ -544,8 +542,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) {
 	}
 
 	if len(req.Sig) > 0 {
-		src = crypto.ToECDSAPub(req.Sig)
-		if !ValidatePublicKey(src) {
+		if src, err = crypto.UnmarshalPubkey(req.Sig); err != nil {
 			return "", ErrInvalidSigningPubKey
 		}
 	}

Original file line number	Diff line number	Diff line change
`@@ -140,8 +140,8 @@ func processArgs() {`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`if asymmetricMode && len(argPub) > 0 {`
`143`		`- pub = crypto.ToECDSAPub(common.FromHex(*argPub))`
`144`		`- if !isKeyValid(pub) {`
	`143`	`+ var err error`
	`144`	`+ if pub, err = crypto.UnmarshalPubkey(common.FromHex(*argPub)); err != nil {`
`145`	`145`	`utils.Fatalf("invalid public key")`
`146`	`146`	`}`
`147`	`147`	`}`
`@@ -321,10 +321,6 @@ func startServer() error {`
`321`	`321`	`return nil`
`322`	`322`	`}`
`323`	`323`
`324`		`-func isKeyValid(k *ecdsa.PublicKey) bool {`
`325`		`- return k.X != nil && k.Y != nil`
`326`		`-}`
`327`		`-`
`328`	`324`	`func configureNode() {`
`329`	`325`	`var err error`
`330`	`326`	`var p2pAccept bool`
`@@ -340,9 +336,8 @@ func configureNode() {`
`340`	`336`	`if b == nil {`
`341`	`337`	`utils.Fatalf("Error: can not convert hexadecimal string")`
`342`	`338`	`}`
`343`		`- pub = crypto.ToECDSAPub(b)`
`344`		`- if !isKeyValid(pub) {`
`345`		`- utils.Fatalf("Error: invalid public key")`
	`339`	`+ if pub, err = crypto.UnmarshalPubkey(b); err != nil {`
	`340`	`+ utils.Fatalf("Error: invalid peer public key")`
`346`	`341`	`}`
`347`	`342`	`}`
`348`	`343`	`}`
Original file line number	Diff line number	Diff line change
`@@ -461,13 +461,11 @@ func (s *PrivateAccountAPI) EcRecover(ctx context.Context, data, sig hexutil.Byt`
`461`	`461`	`}`
`462`	`462`	`sig[64] -= 27 // Transform yellow paper V from 27/28 to 0/1`
`463`	`463`
`464`		`- rpk, err := crypto.Ecrecover(signHash(data), sig)`
	`464`	`+ rpk, err := crypto.SigToPub(signHash(data), sig)`
`465`	`465`	`if err != nil {`
`466`	`466`	`return common.Address{}, err`
`467`	`467`	`}`
`468`		`- pubKey := crypto.ToECDSAPub(rpk)`
`469`		`- recoveredAddr := crypto.PubkeyToAddress(*pubKey)`
`470`		`- return recoveredAddr, nil`
	`468`	`+ return crypto.PubkeyToAddress(*rpk), nil`
`471`	`469`	`}`
`472`	`470`
`473`	`471`	`// SignAndSendTransaction was renamed to SendTransaction. This method is deprecated`
Original file line number	Diff line number	Diff line change
`@@ -528,9 +528,9 @@ func importPublicKey(pubKey []byte) (*ecies.PublicKey, error) {`
`528`	`528`	`return nil, fmt.Errorf("invalid public key length %v (expect 64/65)", len(pubKey))`
`529`	`529`	`}`
`530`	`530`	`// TODO: fewer pointless conversions`
`531`		`- pub := crypto.ToECDSAPub(pubKey65)`
`532`		`- if pub.X == nil {`
`533`		`- return nil, fmt.Errorf("invalid public key")`
	`531`	`+ pub, err := crypto.UnmarshalPubkey(pubKey65)`
	`532`	`+ if err != nil {`
	`533`	`+ return nil, err`
`534`	`534`	`}`
`535`	`535`	`return ecies.ImportECDSAPublic(pub), nil`
`536`	`536`	`}`
Original file line number	Diff line number	Diff line change
`@@ -432,13 +432,11 @@ func (api *SignerAPI) EcRecover(ctx context.Context, data, sig hexutil.Bytes) (c`
`432`	`432`	`}`
`433`	`433`	`sig[64] -= 27 // Transform yellow paper V from 27/28 to 0/1`
`434`	`434`	`hash, _ := SignHash(data)`
`435`		`- rpk, err := crypto.Ecrecover(hash, sig)`
	`435`	`+ rpk, err := crypto.SigToPub(hash, sig)`
`436`	`436`	`if err != nil {`
`437`	`437`	`return common.Address{}, err`
`438`	`438`	`}`
`439`		`- pubKey := crypto.ToECDSAPub(rpk)`
`440`		`- recoveredAddr := crypto.PubkeyToAddress(*pubKey)`
`441`		`- return recoveredAddr, nil`
	`439`	`+ return crypto.PubkeyToAddress(*rpk), nil`
`442`	`440`	`}`
`443`	`441`
`444`	`442`	`// SignHash is a helper function that calculates a hash for the given message that can be`
Original file line number	Diff line number	Diff line change
`@@ -252,8 +252,7 @@ func (api *PublicWhisperAPI) Post(ctx context.Context, req NewMessage) (bool, er`
`252`	`252`
`253`	`253`	`// Set asymmetric key that is used to encrypt the message`
`254`	`254`	`if pubKeyGiven {`
`255`		`- params.Dst = crypto.ToECDSAPub(req.PublicKey)`
`256`		`- if !ValidatePublicKey(params.Dst) {`
	`255`	`+ if params.Dst, err = crypto.UnmarshalPubkey(req.PublicKey); err != nil {`
`257`	`256`	`return false, ErrInvalidPublicKey`
`258`	`257`	`}`
`259`	`258`	`}`
`@@ -329,8 +328,7 @@ func (api PublicWhisperAPI) Messages(ctx context.Context, crit Criteria) (rpc.`
`329`	`328`	`}`
`330`	`329`
`331`	`330`	`if len(crit.Sig) > 0 {`
`332`		`- filter.Src = crypto.ToECDSAPub(crit.Sig)`
`333`		`- if !ValidatePublicKey(filter.Src) {`
	`331`	`+ if filter.Src, err = crypto.UnmarshalPubkey(crit.Sig); err != nil {`
`334`	`332`	`return nil, ErrInvalidSigningPubKey`
`335`	`333`	`}`
`336`	`334`	`}`
`@@ -513,8 +511,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) {`
`513`	`511`	`}`
`514`	`512`
`515`	`513`	`if len(req.Sig) > 0 {`
`516`		`- src = crypto.ToECDSAPub(req.Sig)`
`517`		`- if !ValidatePublicKey(src) {`
	`514`	`+ if src, err = crypto.UnmarshalPubkey(req.Sig); err != nil {`
`518`	`515`	`return "", ErrInvalidSigningPubKey`
`519`	`516`	`}`
`520`	`517`	`}`
Original file line number	Diff line number	Diff line change
`@@ -272,8 +272,7 @@ func (api *PublicWhisperAPI) Post(ctx context.Context, req NewMessage) (hexutil.`
`272`	`272`
`273`	`273`	`// Set asymmetric key that is used to encrypt the message`
`274`	`274`	`if pubKeyGiven {`
`275`		`- params.Dst = crypto.ToECDSAPub(req.PublicKey)`
`276`		`- if !ValidatePublicKey(params.Dst) {`
	`275`	`+ if params.Dst, err = crypto.UnmarshalPubkey(req.PublicKey); err != nil {`
`277`	`276`	`return nil, ErrInvalidPublicKey`
`278`	`277`	`}`
`279`	`278`	`}`
`@@ -360,8 +359,7 @@ func (api PublicWhisperAPI) Messages(ctx context.Context, crit Criteria) (rpc.`
`360`	`359`	`}`
`361`	`360`
`362`	`361`	`if len(crit.Sig) > 0 {`
`363`		`- filter.Src = crypto.ToECDSAPub(crit.Sig)`
`364`		`- if !ValidatePublicKey(filter.Src) {`
	`362`	`+ if filter.Src, err = crypto.UnmarshalPubkey(crit.Sig); err != nil {`
`365`	`363`	`return nil, ErrInvalidSigningPubKey`
`366`	`364`	`}`
`367`	`365`	`}`
`@@ -544,8 +542,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) {`
`544`	`542`	`}`
`545`	`543`
`546`	`544`	`if len(req.Sig) > 0 {`
`547`		`- src = crypto.ToECDSAPub(req.Sig)`
`548`		`- if !ValidatePublicKey(src) {`
	`545`	`+ if src, err = crypto.UnmarshalPubkey(req.Sig); err != nil {`
`549`	`546`	`return "", ErrInvalidSigningPubKey`
`550`	`547`	`}`
`551`	`548`	`}`