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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jun 12, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e2f6e8
crypto: replace ToECDSAPub with error-checking func UnmarshalPubkey
fjl Jun 8, 2018
9295467
crypto: add test for UnmarshalPubkey
fjl Jun 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions cmd/wnode/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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")
}
}
}
Expand Down
13 changes: 8 additions & 5 deletions crypto/crypto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ var (
secp256k1halfN = new(big.Int).Div(secp256k1N, big.NewInt(2))
)

var errInvalidPubkey = errors.New("invalid secp256k1 public key")
Copy link
Member

Choose a reason for hiding this comment

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

I would make it exportable, so that whoever calls it can compare the error, like:

err := MyFunctionThatCallsUnmarshalPubkeyAtAVeryNestedLevel()
if err == crypto.ErrInvalidPubkey {
  panic("crypto breach!")
} else {
 // recover the error
}

I'm aware you can still String()-compare, but it's nice to keep the type info as well.


// Keccak256 calculates and returns the Keccak256 hash of the input data.
func Keccak256(data ...[]byte) []byte {
d := sha3.NewKeccak256()
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be no unit test that covers the case when len(pub) == 0. This is a problem, because even though a 0-length pub will return nil in the current implementation, we don't know how things could change if the key and signature schemes change because they turn out to be unsafe.

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 {
Copy link
Member

Choose a reason for hiding this comment

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

I would also check that y != nil, because if it's true that in the current implementation x and y are either both nil or both non-nil, this is an undocumented feature and it could change in the future (btw, the fact that elliptic.Unmarshal returns x == nil on error is a bit strange to me, in the context of Go, which is what prompts me to write this nitpick because the interface should change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

package elliptic is a standard library package and returning nil on failure is a known shortcoming of that API. That's one of the reasons we have the wrapper in the first place.

return nil, errInvalidPubkey
}
return &ecdsa.PublicKey{Curve: S256(), X: x, Y: y}, nil
}

func FromECDSAPub(pub *ecdsa.PublicKey) []byte {
Expand Down
2 changes: 1 addition & 1 deletion crypto/crypto_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,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)
Expand Down
6 changes: 2 additions & 4 deletions internal/ethapi/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,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
Expand Down
6 changes: 3 additions & 3 deletions p2p/rlpx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
6 changes: 2 additions & 4 deletions signer/core/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion swarm/services/swap/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package swap
import (
"context"
"crypto/ecdsa"
"errors"
"fmt"
"math/big"
"os"
Expand Down Expand Up @@ -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
Expand All @@ -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))
}
Expand Down
9 changes: 3 additions & 6 deletions whisper/whisperv5/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down
9 changes: 3 additions & 6 deletions whisper/whisperv6/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down