From 5b42e83bc2b69db97f854c3042b7a84bbf88747c Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Thu, 6 Dec 2018 02:02:04 -0500 Subject: [PATCH] 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 * Minor reformatting --- PENDING.md | 1 + client/utils/utils.go | 18 +++++---- cmd/gaia/cli_test/cli_test.go | 76 +++++++++++++++++++++++++++++++++++ x/auth/client/cli/sign.go | 67 ++++++++++++++++++++++++------ 4 files changed, 141 insertions(+), 21 deletions(-) diff --git a/PENDING.md b/PENDING.md index dd4fe4bb6..9c4db6c45 100644 --- a/PENDING.md +++ b/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 diff --git a/client/utils/utils.go b/client/utils/utils.go index 08b8d87f2..2d506e462 100644 --- a/client/utils/utils.go +++ b/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) } diff --git a/cmd/gaia/cli_test/cli_test.go b/cmd/gaia/cli_test/cli_test.go index 6fc60e1ba..f7d3b2d83 100644 --- a/cmd/gaia/cli_test/cli_test.go +++ b/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)) diff --git a/x/auth/client/cli/sign.go b/x/auth/client/cli/sign.go index 73facdfd9..39d9b6694 100644 --- a/x/auth/client/cli/sign.go +++ b/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,10 +69,15 @@ 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 } @@ -78,13 +85,11 @@ func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error 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 }