Skip to content
This repository has been archived by the owner on Jul 15, 2018. It is now read-only.

PubKeyFromBytes should return a blank PubKey on failing to parse bytes #48

Closed
odeke-em opened this issue Oct 28, 2017 · 0 comments
Closed

Comments

@odeke-em
Copy link
Contributor

The current implementation of PubKeyFromBytes could be improved. It currently is

go-crypto/pub_key.go

Lines 16 to 19 in dd20358

func PubKeyFromBytes(pubKeyBytes []byte) (pubKey PubKey, err error) {
err = wire.ReadBinaryBytes(pubKeyBytes, &pubKey)
return
}

Notice that even if err != nil we took the address of the named return value so set some stuff in it.
The problem though is that if we try to parse a key with invalid bytes and that fails, later on test
pubKey.Empty() will return false which is wrong.

This behavior skewed my fuzzing tests and here is a simple repro

package main

import (
	"log"

	"github.com/tendermint/go-crypto"
)

func main() {
	data := []byte{0x02, 0x03}
	pk, err := crypto.PubKeyFromBytes(data)
	log.Printf("empty: %v\ninner: %#v\n", pk.Empty(), pk.PubKeyInner)
	if err != nil {
		log.Fatalf("err: %v\n", err)
	}
	log.Printf("pk: %#v\n", pk)
}

which gives

$ go run main.go 
2017/10/28 14:05:55 empty: false
inner: crypto.PubKeySecp256k1{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
2017/10/28 14:05:55 err: unexpected EOF
exit status 1

yet it should actually give

2017/10/28 14:06:21 empty: true
inner: <nil>
2017/10/28 14:06:21 err: unexpected EOF
exit status 1
odeke-em added a commit that referenced this issue Oct 28, 2017
Fixes #48.

This previously skewed up my fuzzing tests so ensure
that on error we return the zero value PubKey.
@ebuchman ebuchman closed this as completed Nov 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants