Merge PR #2995: Fully verify the signature in gaiacli tx sign

* Implement auxiliary methods/functions for just building a tx sig bytes

* Undo auxiliary methods/functions

* Validate signature

* Remove redundant comment

* Add pending log entry

* Minor cleanup

* Update sign cli doc

* Update cli sign offline flag doc

* Update printAndValidateSigs

* Implement TestGaiaCLIValidateSignatures

* Minor cleanup

* Fix linting

* Update x/auth/client/cli/sign.go

Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>

* Minor reformatting

PENDING.md

@@ -39,6 +39,7 @@ IMPROVEMENTS
 * Gaia REST API (`gaiacli advanced rest-server`)
 
 * Gaia CLI  (`gaiacli`)
+  * \#2991 Fully validate transaction signatures during `gaiacli tx sign --validate-signatures`
 
 * Gaia
 

client/utils/utils.go

@@ -15,13 +15,12 @@ import (
 	"github.com/tendermint/tendermint/libs/common"
 )
 
-// CompleteAndBroadcastTxCli implements a utility function that
-// facilitates sending a series of messages in a signed
-// transaction given a TxBuilder and a QueryContext. It ensures
-// that the account exists, has a proper number and sequence
-// set. In addition, it builds and signs a transaction with the
-// supplied messages.  Finally, it broadcasts the signed
-// transaction to a node.
+// CompleteAndBroadcastTxCli implements a utility function that facilitates
+// sending a series of messages in a signed transaction given a TxBuilder and a
+// QueryContext. It ensures that the account exists, has a proper number and
+// sequence set. In addition, it builds and signs a transaction with the
+// supplied messages. Finally, it broadcasts the signed transaction to a node.
+//
 // NOTE: Also see CompleteAndBroadcastTxREST.
 func CompleteAndBroadcastTxCli(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, msgs []sdk.Msg) error {
 	txBldr, err := prepareTxBuilder(txBldr, cliCtx)
@@ -116,13 +115,15 @@ func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string,
 	if err != nil {
 		return signedStdTx, err
 	}
+
 	info, err := keybase.Get(name)
 	if err != nil {
 		return signedStdTx, err
 	}
+
 	addr := info.GetPubKey().Address()
 
-	// Check whether the address is a signer
+	// check whether the address is a signer
 	if !isTxSigner(sdk.AccAddress(addr), stdTx.GetSigners()) {
 		return signedStdTx, fmt.Errorf(
 			"The generated transaction's intended signer does not match the given signer: %q", name)
@@ -148,6 +149,7 @@ func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string,
 	if err != nil {
 		return signedStdTx, err
 	}
+
 	return txBldr.SignStdTx(name, passphrase, stdTx, appendSig)
 }
 

cmd/gaia/cli_test/cli_test.go

@@ -453,6 +453,75 @@ func TestGaiaCLISubmitProposal(t *testing.T) {
 	cleanupDirs(gaiadHome, gaiacliHome)
 }
 
+func TestGaiaCLIValidateSignatures(t *testing.T) {
+	t.Parallel()
+	chainID, servAddr, port, gaiadHome, gaiacliHome, p2pAddr := initializeFixtures(t)
+	flags := fmt.Sprintf("--home=%s --node=%v --chain-id=%v", gaiacliHome, servAddr, chainID)
+
+	// start gaiad server
+	proc := tests.GoExecuteTWithStdout(
+		t, fmt.Sprintf(
+			"gaiad start --home=%s --rpc.laddr=%v --p2p.laddr=%v", gaiadHome, servAddr, p2pAddr,
+		),
+	)
+
+	defer proc.Stop(false)
+	tests.WaitForTMStart(port)
+	tests.WaitForNextNBlocksTM(1, port)
+
+	fooAddr, _ := executeGetAddrPK(t, fmt.Sprintf("gaiacli keys show foo --home=%s", gaiacliHome))
+	barAddr, _ := executeGetAddrPK(t, fmt.Sprintf("gaiacli keys show bar --home=%s", gaiacliHome))
+
+	// generate sendTx with default gas
+	success, stdout, stderr := executeWriteRetStdStreams(
+		t, fmt.Sprintf(
+			"gaiacli tx send %v --amount=10%s --to=%s --from=foo --generate-only",
+			flags, stakeTypes.DefaultBondDenom, barAddr,
+		),
+		[]string{}...,
+	)
+	require.True(t, success)
+	require.Empty(t, stderr)
+
+	// write  unsigned tx to file
+	unsignedTxFile := writeToNewTempFile(t, stdout)
+	defer os.Remove(unsignedTxFile.Name())
+
+	// validate we can successfully sign
+	success, stdout, _ = executeWriteRetStdStreams(
+		t, fmt.Sprintf("gaiacli tx sign %v --name=foo %v", flags, unsignedTxFile.Name()),
+		app.DefaultKeyPass,
+	)
+	require.True(t, success)
+
+	stdTx := unmarshalStdTx(t, stdout)
+	require.Equal(t, len(stdTx.Msgs), 1)
+	require.Equal(t, 1, len(stdTx.GetSignatures()))
+	require.Equal(t, fooAddr.String(), stdTx.GetSigners()[0].String())
+
+	// write signed tx to file
+	signedTxFile := writeToNewTempFile(t, stdout)
+	defer os.Remove(signedTxFile.Name())
+
+	// validate signatures
+	success, _, _ = executeWriteRetStdStreams(
+		t, fmt.Sprintf("gaiacli tx sign %v --validate-signatures %v", flags, signedTxFile.Name()),
+	)
+	require.True(t, success)
+
+	// modify the transaction
+	stdTx.Memo = "MODIFIED-ORIGINAL-TX-BAD"
+	bz := marshalStdTx(t, stdTx)
+	modSignedTxFile := writeToNewTempFile(t, string(bz))
+	defer os.Remove(modSignedTxFile.Name())
+
+	// validate signature validation failure due to different transaction sig bytes
+	success, _, _ = executeWriteRetStdStreams(
+		t, fmt.Sprintf("gaiacli tx sign %v --validate-signatures %v", flags, modSignedTxFile.Name()),
+	)
+	require.False(t, success)
+}
+
 func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) {
 	t.Parallel()
 	chainID, servAddr, port, gaiadHome, gaiacliHome, p2pAddr := initializeFixtures(t)
@@ -612,6 +681,13 @@ func initializeFixtures(t *testing.T) (chainID, servAddr, port, gaiadHome, gaiac
 	return
 }
 
+func marshalStdTx(t *testing.T, stdTx auth.StdTx) []byte {
+	cdc := app.MakeCodec()
+	bz, err := cdc.MarshalBinaryBare(stdTx)
+	require.NoError(t, err)
+	return bz
+}
+
 func unmarshalStdTx(t *testing.T, s string) (stdTx auth.StdTx) {
 	cdc := app.MakeCodec()
 	require.Nil(t, cdc.UnmarshalJSON([]byte(s), &stdTx))

x/auth/client/cli/sign.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -11,7 +12,6 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth"
 	authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder"
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 	"github.com/tendermint/go-amino"
@@ -37,10 +37,12 @@ If the flag --signature-only flag is on, it outputs a JSON representation
 of the generated signature only.
 
 If the flag --validate-signatures is on, then the command would check whether all required
-signers have signed the transactions and whether the signatures were collected in the right
-order.
+signers have signed the transactions, whether the signatures were collected in the right
+order, and if the signature is valid over the given transaction. If the --offline
+flag is also provided, signature validation over the transaction will be not be
+performed as that will require communication with a full node.
 
-The --offline flag makes sure that the client will not reach out to the local cache.
+The --offline flag makes sure that the client will not reach out to an external node.
 Thus account number or sequence number lookups will not be performed and it is
 recommended to set such parameters manually.`,
 		RunE: makeSignCmd(codec),
@@ -52,7 +54,7 @@ recommended to set such parameters manually.`,
 	cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit.")
 	cmd.Flags().Bool(flagValidateSigs, false, "Print the addresses that must sign the transaction, "+
 		"those who have already signed it, and make sure that signatures are in the correct order.")
-	cmd.Flags().Bool(flagOffline, false, "Offline mode. Do not query local cache.")
+	cmd.Flags().Bool(flagOffline, false, "Offline mode. Do not query a full node.")
 	cmd.Flags().String(flagOutfile, "",
 		"The document will be written to the given file instead of STDOUT")
 
@@ -67,24 +69,27 @@ func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error
 			return
 		}
 
+		offline := viper.GetBool(flagOffline)
+		cliCtx := context.NewCLIContext().WithCodec(cdc).WithAccountDecoder(cdc)
+		txBldr := authtxb.NewTxBuilderFromCLI()
+
 		if viper.GetBool(flagValidateSigs) {
-			if !printSignatures(stdTx) {
+			if !printAndValidateSigs(cliCtx, txBldr.ChainID, stdTx, offline) {
 				return fmt.Errorf("signatures validation failed")
 			}
+
 			return nil
 		}
 
 		name := viper.GetString(client.FlagName)
 		if name == "" {
 			return errors.New("required flag \"name\" has not been set")
 		}
-		cliCtx := context.NewCLIContext().WithCodec(cdc).WithAccountDecoder(cdc)
-		txBldr := authtxb.NewTxBuilderFromCLI()
 
 		// if --signature-only is on, then override --append
 		generateSignatureOnly := viper.GetBool(flagSigOnly)
 		appendSig := viper.GetBool(flagAppend) && !generateSignatureOnly
-		newTx, err := utils.SignStdTx(txBldr, cliCtx, name, stdTx, appendSig, viper.GetBool(flagOffline))
+		newTx, err := utils.SignStdTx(txBldr, cliCtx, name, stdTx, appendSig, offline)
 		if err != nil {
 			return err
 		}
@@ -107,6 +112,7 @@ func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error
 				json, err = cdc.MarshalJSON(newTx)
 			}
 		}
+
 		if err != nil {
 			return err
 		}
@@ -122,35 +128,70 @@ func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error
 		if err != nil {
 			return err
 		}
+
 		defer fp.Close()
 		fmt.Fprintf(fp, "%s\n", json)
+
 		return
 	}
 }
 
-func printSignatures(stdTx auth.StdTx) bool {
+// printAndValidateSigs will validate the signatures of a given transaction over
+// its expected signers. In addition, if offline has not been supplied, the
+// signature is verified over the transaction sign bytes.
+func printAndValidateSigs(
+	cliCtx context.CLIContext, chainID string, stdTx auth.StdTx, offline bool,
+) bool {
+
 	fmt.Println("Signers:")
+
 	signers := stdTx.GetSigners()
 	for i, signer := range signers {
 		fmt.Printf(" %v: %v\n", i, signer.String())
 	}
 
+	success := true
 	sigs := stdTx.GetSignatures()
+
 	fmt.Println("")
 	fmt.Println("Signatures:")
-	success := true
+
 	if len(sigs) != len(signers) {
 		success = false
 	}
-	for i, sig := range stdTx.GetSignatures() {
+
+	for i, sig := range sigs {
 		sigAddr := sdk.AccAddress(sig.Address())
 		sigSanity := "OK"
+
 		if i >= len(signers) || !sigAddr.Equals(signers[i]) {
-			sigSanity = fmt.Sprintf("ERROR: signature %d does not match its respective signer", i)
+			sigSanity = "ERROR: signature does not match its respective signer"
 			success = false
 		}
+
+		// Validate the actual signature over the transaction bytes since we can
+		// reach out to a full node to query accounts.
+		if !offline && success {
+			acc, err := cliCtx.GetAccount(sigAddr)
+			if err != nil {
+				fmt.Printf("failed to get account: %s\n", sigAddr)
+				return false
+			}
+
+			sigBytes := auth.StdSignBytes(
+				chainID, acc.GetAccountNumber(), acc.GetSequence(),
+				stdTx.Fee, stdTx.GetMsgs(), stdTx.GetMemo(),
+			)
+
+			if ok := sig.VerifyBytes(sigBytes, sig.Signature); !ok {
+				sigSanity = "ERROR: signature invalid"
+				success = false
+			}
+		}
+
 		fmt.Printf(" %v: %v\t[%s]\n", i, sigAddr.String(), sigSanity)
 	}
+
 	fmt.Println("")
 	return success
 }