Vulnerability in light client proxy (#1081)

* Vulnerability in light client proxy

When calling GetCertifiedCommit the light client proxy would call
Certify and even on error return the Commit as if it had been correctly
certified.

Now it returns the error correctly and returns an empty Commit on error.

* Improve names for clarity

The lite package now contains StaticCertifier, DynamicCertifier and
InqueringCertifier. This also changes the method receivers from one
letter to two letter names, which will make future refactoring easier
and follows the coding standards.

* Fix test failures

* Rename files

* remove dead code

lite/proxy/certifier.go

@@ -6,7 +6,7 @@ import (
 	"github.com/tendermint/tendermint/lite/files"
 )
 
-func GetCertifier(chainID, rootDir, nodeAddr string) (*lite.Inquiring, error) {
+func GetCertifier(chainID, rootDir, nodeAddr string) (*lite.InquiringCertifier, error) {
 	trust := lite.NewCacheProvider(
 		lite.NewMemStoreProvider(),
 		files.NewProvider(rootDir),
@@ -26,7 +26,7 @@ func GetCertifier(chainID, rootDir, nodeAddr string) (*lite.Inquiring, error) {
 		return nil, err
 	}
 
-	cert, err := lite.NewInquiring(chainID, fc, trust, source)
+	cert, err := lite.NewInquiringCertifier(chainID, fc, trust, source)
 	if err != nil {
 		return nil, err
 	}

lite/proxy/proxy.go

@@ -18,7 +18,11 @@ const (
 // set up the rpc routes to proxy via the given client,
 // and start up an http/rpc server on the location given by bind (eg. :1234)
 func StartProxy(c rpcclient.Client, listenAddr string, logger log.Logger) error {
-	c.Start()
+	err := c.Start()
+	if err != nil {
+		return err
+	}
+
 	r := RPCRoutes(c)
 
 	// build the handler...
@@ -30,7 +34,7 @@ func StartProxy(c rpcclient.Client, listenAddr string, logger log.Logger) error
 	core.SetLogger(logger)
 	mux.HandleFunc(wsEndpoint, wm.WebsocketHandler)
 
-	_, err := rpc.StartHTTPServer(listenAddr, mux, logger)
+	_, err = rpc.StartHTTPServer(listenAddr, mux, logger)
 
 	return err
 }

lite/proxy/query.go

@@ -51,7 +51,7 @@ func GetWithProofOptions(path string, key []byte, opts rpcclient.ABCIQueryOption
 
 	// make sure the proof is the proper height
 	if resp.IsErr() {
-		err = errors.Errorf("Query error %d: %d", resp.Code)
+		err = errors.Errorf("Query error for key %d: %d", key, resp.Code)
 		return nil, nil, err
 	}
 	if len(resp.Key) == 0 || len(resp.Proof) == 0 {
@@ -79,7 +79,7 @@ func GetWithProofOptions(path string, key []byte, opts rpcclient.ABCIQueryOption
 		if err != nil {
 			return nil, nil, errors.Wrap(err, "Couldn't verify proof")
 		}
-		return &ctypes.ResultABCIQuery{resp}, eproof, nil
+		return &ctypes.ResultABCIQuery{Response: resp}, eproof, nil
 	}
 
 	// The key wasn't found, construct a proof of non-existence.
@@ -93,13 +93,12 @@ func GetWithProofOptions(path string, key []byte, opts rpcclient.ABCIQueryOption
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "Couldn't verify proof")
 	}
-	return &ctypes.ResultABCIQuery{resp}, aproof, ErrNoData()
+	return &ctypes.ResultABCIQuery{Response: resp}, aproof, ErrNoData()
 }
 
 // GetCertifiedCommit gets the signed header for a given height
 // and certifies it.  Returns error if unable to get a proven header.
-func GetCertifiedCommit(h int64, node rpcclient.Client,
-	cert lite.Certifier) (empty lite.Commit, err error) {
+func GetCertifiedCommit(h int64, node rpcclient.Client, cert lite.Certifier) (lite.Commit, error) {
 
 	// FIXME: cannot use cert.GetByHeight for now, as it also requires
 	// Validators and will fail on querying tendermint for non-current height.
@@ -107,14 +106,18 @@ func GetCertifiedCommit(h int64, node rpcclient.Client,
 	rpcclient.WaitForHeight(node, h, nil)
 	cresp, err := node.Commit(&h)
 	if err != nil {
-		return
+		return lite.Commit{}, err
 	}
-	commit := client.CommitFromResult(cresp)
 
+	commit := client.CommitFromResult(cresp)
 	// validate downloaded checkpoint with our request and trust store.
 	if commit.Height() != h {
-		return empty, certerr.ErrHeightMismatch(h, commit.Height())
+		return lite.Commit{}, certerr.ErrHeightMismatch(h, commit.Height())
 	}
-	err = cert.Certify(commit)
+
+	if err = cert.Certify(commit); err != nil {
+		return lite.Commit{}, err
+	}
+
 	return commit, nil
 }

lite/proxy/query_test.go

@@ -58,7 +58,7 @@ func _TestAppProofs(t *testing.T) {
 	source := certclient.NewProvider(cl)
 	seed, err := source.GetByHeight(brh - 2)
 	require.NoError(err, "%+v", err)
-	cert := lite.NewStatic("my-chain", seed.Validators)
+	cert := lite.NewStaticCertifier("my-chain", seed.Validators)
 
 	client.WaitForHeight(cl, 3, nil)
 	latest, err := source.LatestCommit()
@@ -117,7 +117,7 @@ func _TestTxProofs(t *testing.T) {
 	source := certclient.NewProvider(cl)
 	seed, err := source.GetByHeight(brh - 2)
 	require.NoError(err, "%+v", err)
-	cert := lite.NewStatic("my-chain", seed.Validators)
+	cert := lite.NewStaticCertifier("my-chain", seed.Validators)
 
 	// First let's make sure a bogus transaction hash returns a valid non-existence proof.
 	key := types.Tx([]byte("bogus")).Hash()
@@ -136,5 +136,4 @@ func _TestTxProofs(t *testing.T) {
 	commit, err := GetCertifiedCommit(br.Height, cl, cert)
 	require.Nil(err, "%+v", err)
 	require.Equal(res.Proof.RootHash, commit.Header.DataHash)
-
 }

lite/proxy/wrapper.go

@@ -15,14 +15,14 @@ var _ rpcclient.Client = Wrapper{}
 // provable before passing it along. Allows you to make any rpcclient fully secure.
 type Wrapper struct {
 	rpcclient.Client
-	cert *lite.Inquiring
+	cert *lite.InquiringCertifier
 }
 
 // SecureClient uses a given certifier to wrap an connection to an untrusted
 // host and return a cryptographically secure rpc client.
 //
 // If it is wrapping an HTTP rpcclient, it will also wrap the websocket interface
-func SecureClient(c rpcclient.Client, cert *lite.Inquiring) Wrapper {
+func SecureClient(c rpcclient.Client, cert *lite.InquiringCertifier) Wrapper {
 	wrap := Wrapper{c, cert}
 	// TODO: no longer possible as no more such interface exposed....
 	// if we wrap http client, then we can swap out the event switch to filter
@@ -34,7 +34,9 @@ func SecureClient(c rpcclient.Client, cert *lite.Inquiring) Wrapper {
 }
 
 // ABCIQueryWithOptions exposes all options for the ABCI query and verifies the returned proof
-func (w Wrapper) ABCIQueryWithOptions(path string, data data.Bytes, opts rpcclient.ABCIQueryOptions) (*ctypes.ResultABCIQuery, error) {
+func (w Wrapper) ABCIQueryWithOptions(path string, data data.Bytes,
+	opts rpcclient.ABCIQueryOptions) (*ctypes.ResultABCIQuery, error) {
+
 	res, _, err := GetWithProofOptions(path, data, opts, w.Client, w.cert)
 	return res, err
 }