From 70e9f52f76c6340f7b7573d72d955a3cb9ad38ee Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 4 Mar 2018 01:54:13 -0500 Subject: [PATCH 1/9] examples/bascoin/app: clean tests --- examples/basecoin/app/app_test.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index affa01415..1349b6767 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -28,21 +28,12 @@ var ( priv1 = crypto.GenPrivKeyEd25519() addr1 = priv1.PubKey().Address() addr2 = crypto.GenPrivKeyEd25519().PubKey().Address() + coins = sdk.Coins{{"foocoin", 10}} + // Construct a SendMsg sendMsg = bank.SendMsg{ - Inputs: []bank.Input{ - { - Address: addr1, - Coins: sdk.Coins{{"foocoin", 10}}, - Sequence: 1, - }, - }, - Outputs: []bank.Output{ - { - Address: addr2, - Coins: sdk.Coins{{"foocoin", 10}}, - }, - }, + Inputs: []bank.Input{bank.NewInput(addr1, coins)}, + Outputs: []bank.Output{bank.NewOutput(addr2, coins)}, } whatCoolMsg1 = cool.WhatCoolMsg{ From 9b8db6b37cbeb41b314ee172975e0ea60fca63e3 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 4 Mar 2018 02:32:39 -0500 Subject: [PATCH 2/9] x/auth: clean up ante handler --- x/auth/ante.go | 121 ++++++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/x/auth/ante.go b/x/auth/ante.go index 11aa03c0d..17e9ccdd9 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -9,82 +9,91 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { ctx sdk.Context, tx sdk.Tx, ) (_ sdk.Context, _ sdk.Result, abort bool) { - // Deduct the fee from the fee payer. - // This is done first because it only - // requires fetching 1 account. - payerAddr := tx.GetFeePayer() - if payerAddr != nil { - payerAcc := accountMapper.GetAccount(ctx, payerAddr) - if payerAcc == nil { - return ctx, - sdk.ErrUnrecognizedAddress(payerAddr).Result(), - true - } - // TODO: Charge fee from payerAcc. - // TODO: accountMapper.SetAccount(ctx, payerAddr) - } else { - // TODO: Ensure that some other spam prevention is used. - } - - var sigs = tx.GetSignatures() - // Assert that there are signatures. + var sigs = tx.GetSignatures() if len(sigs) == 0 { return ctx, sdk.ErrUnauthorized("no signers").Result(), true } - // Ensure that sigs are correct. - var msg = tx.GetMsg() - var signerAddrs = msg.GetSigners() - var signerAccs = make([]sdk.Account, len(signerAddrs)) + // TODO: can tx just implement message? + msg := tx.GetMsg() // Assert that number of signatures is correct. + var signerAddrs = msg.GetSigners() if len(sigs) != len(signerAddrs) { return ctx, sdk.ErrUnauthorized("wrong number of signers").Result(), true } - // Check each nonce and sig. - // TODO Refactor out. - for i, sig := range sigs { + // Collect accounts to set in the context + var signerAccs = make([]sdk.Account, len(signerAddrs)) - var signerAcc = accountMapper.GetAccount(ctx, signerAddrs[i]) + signBytes := msg.GetSignBytes() + + // First sig is the fee payer. + // Check sig and nonce, deduct fee. + // This is done first because it only + // requires fetching 1 account. + payerAcc, res := processSig(ctx, accountMapper, signerAddrs[0], sigs[0], signBytes) + if !res.IsOK() { + return ctx, res, true + } + signerAccs[0] = payerAcc + // TODO: Charge fee from payerAcc. + // TODO: accountMapper.SetAccount(ctx, payerAddr) + + // Check sig and nonce for the rest. + for i := 1; i < len(sigs); i++ { + signerAcc, res := processSig(ctx, accountMapper, signerAddrs[i], sigs[i], signBytes) + if !res.IsOK() { + return ctx, res, true + } signerAccs[i] = signerAcc - - // If no pubkey, set pubkey. - if signerAcc.GetPubKey().Empty() { - err := signerAcc.SetPubKey(sig.PubKey) - if err != nil { - return ctx, - sdk.ErrInternal("setting PubKey on signer").Result(), - true - } - } - - // Check and increment sequence number. - seq := signerAcc.GetSequence() - if seq != sig.Sequence { - return ctx, - sdk.ErrInvalidSequence("").Result(), - true - } - signerAcc.SetSequence(seq + 1) - - // Check sig. - if !sig.PubKey.VerifyBytes(msg.GetSignBytes(), sig.Signature) { - return ctx, - sdk.ErrUnauthorized("").Result(), - true - } - - // Save the account. - accountMapper.SetAccount(ctx, signerAcc) } ctx = WithSigners(ctx, signerAccs) return ctx, sdk.Result{}, false // continue... } } + +// verify the signature and increment the sequence. +// if the account doesn't have a pubkey, set it as well. +func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk.StdSignature, signBytes []byte) (acc sdk.Account, res sdk.Result) { + + // Get the account + acc = am.GetAccount(ctx, addr) + if acc == nil { + return nil, sdk.ErrUnrecognizedAddress(addr).Result() + } + + // Check and increment sequence number. + seq := acc.GetSequence() + if seq != sig.Sequence { + return nil, sdk.ErrInvalidSequence("").Result() + } + acc.SetSequence(seq + 1) + + // Check and possibly set pubkey. + pubKey := acc.GetPubKey() + if pubKey.Empty() { + pubKey = sig.PubKey + err := acc.SetPubKey(pubKey) + if err != nil { + return nil, sdk.ErrInternal("setting PubKey on signer").Result() + } + } + // TODO: should we enforce pubKey == sig.PubKey ? + // If not, ppl can send useless PubKeys after first tx + + // Check sig. + if !sig.PubKey.VerifyBytes(signBytes, sig.Signature) { + return nil, sdk.ErrUnauthorized("").Result() + } + + // Save the account. + am.SetAccount(ctx, acc) + return +} From b906c141bd47f555abe3a57d2ae8e27291c24cd1 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 4 Mar 2018 02:36:10 -0500 Subject: [PATCH 3/9] x/bank: remove Sequence from Input --- types/tx_msg.go | 13 ++++++++++++- x/bank/tx.go | 19 ++----------------- x/bank/tx_test.go | 40 +++++++++++++++------------------------- 3 files changed, 29 insertions(+), 43 deletions(-) diff --git a/types/tx_msg.go b/types/tx_msg.go index c6c96c364..e97fc331f 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -66,7 +66,18 @@ func (tx StdTx) GetMsg() Msg { return tx.Msg } func (tx StdTx) GetFeePayer() Address { return tx.Signatures[0].PubKey.Address() } // XXX but PubKey is optional! func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } -//__________________________________________________________ +// StdSignDoc is replay-prevention structure. +// It includes the result of msg.GetSignBytes(), +// as well as the ChainID (prevent cross chain replay) +// and the Sequence (prevent inchain replay). +type StdSignDoc struct { + ChainID string `json:"chain_id"` + Sequence int64 `json:"sequence"` + MsgBytes []byte `json:"msg_bytes"` + AltBytes []byte `json:"alt_bytes"` // TODO: do we really want this ? +} + +//------------------------------------- // Application function variable used to unmarshal transaction bytes type TxDecoder func(txBytes []byte) (Tx, Error) diff --git a/x/bank/tx.go b/x/bank/tx.go index eef2a087d..58af93721 100644 --- a/x/bank/tx.go +++ b/x/bank/tx.go @@ -4,8 +4,6 @@ import ( "encoding/json" "fmt" - crypto "github.com/tendermint/go-crypto" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -142,11 +140,8 @@ func (msg IssueMsg) GetSigners() []sdk.Address { // Transaction Output type Input struct { - Address sdk.Address `json:"address"` - Coins sdk.Coins `json:"coins"` - Sequence int64 `json:"sequence"` - - signature crypto.Signature + Address sdk.Address `json:"address"` + Coins sdk.Coins `json:"coins"` } // ValidateBasic - validate transaction input @@ -154,9 +149,6 @@ func (in Input) ValidateBasic() sdk.Error { if len(in.Address) == 0 { return ErrInvalidAddress(in.Address.String()) } - if in.Sequence < 0 { - return ErrInvalidSequence("negative sequence") - } if !in.Coins.IsValid() { return ErrInvalidCoins(in.Coins.String()) } @@ -179,13 +171,6 @@ func NewInput(addr sdk.Address, coins sdk.Coins) Input { return input } -// NewInputWithSequence - create a transaction input, used with SendMsg -func NewInputWithSequence(addr sdk.Address, coins sdk.Coins, seq int64) Input { - input := NewInput(addr, coins) - input.Sequence = seq - return input -} - //---------------------------------------- // Output diff --git a/x/bank/tx_test.go b/x/bank/tx_test.go index 099e81dbf..6d544626a 100644 --- a/x/bank/tx_test.go +++ b/x/bank/tx_test.go @@ -13,20 +13,12 @@ func TestNewSendMsg(t *testing.T) {} func TestSendMsgType(t *testing.T) { // Construct a SendMsg + addr1 := sdk.Address([]byte("input")) + addr2 := sdk.Address([]byte("output")) + coins := sdk.Coins{{"atom", 10}} var msg = SendMsg{ - Inputs: []Input{ - { - Address: sdk.Address([]byte("input")), - Coins: sdk.Coins{{"atom", 10}}, - Sequence: 1, - }, - }, - Outputs: []Output{ - { - Address: sdk.Address([]byte("output")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Inputs: []Input{NewInput(addr1, coins)}, + Outputs: []Output{NewOutput(addr2, coins)}, } // TODO some failures for bad result @@ -53,18 +45,16 @@ func TestInputValidation(t *testing.T) { }{ // auth works with different apps {true, NewInput(addr1, someCoins)}, - {true, NewInputWithSequence(addr1, someCoins, 100)}, - {true, NewInputWithSequence(addr2, someCoins, 100)}, - {true, NewInputWithSequence(addr2, multiCoins, 100)}, + {true, NewInput(addr2, someCoins)}, + {true, NewInput(addr2, multiCoins)}, - {false, NewInput(emptyAddr, someCoins)}, // empty address - {false, NewInputWithSequence(addr1, someCoins, -1)}, // negative sequence - {false, NewInput(addr1, emptyCoins)}, // invalid coins - {false, NewInput(addr1, emptyCoins2)}, // invalid coins - {false, NewInput(addr1, someEmptyCoins)}, // invalid coins - {false, NewInput(addr1, minusCoins)}, // negative coins - {false, NewInput(addr1, someMinusCoins)}, // negative coins - {false, NewInput(addr1, unsortedCoins)}, // unsorted coins + {false, NewInput(emptyAddr, someCoins)}, // empty address + {false, NewInput(addr1, emptyCoins)}, // invalid coins + {false, NewInput(addr1, emptyCoins2)}, // invalid coins + {false, NewInput(addr1, someEmptyCoins)}, // invalid coins + {false, NewInput(addr1, minusCoins)}, // negative coins + {false, NewInput(addr1, someMinusCoins)}, // negative coins + {false, NewInput(addr1, unsortedCoins)}, // unsorted coins } for i, tc := range cases { @@ -144,7 +134,7 @@ func TestSendMsgValidation(t *testing.T) { {false, SendMsg{Inputs: []Input{input1}}}, // just input {false, SendMsg{Outputs: []Output{output1}}}, // just ouput {false, SendMsg{ - Inputs: []Input{NewInputWithSequence(emptyAddr, atom123, 1)}, // invalid input + Inputs: []Input{NewInput(emptyAddr, atom123)}, // invalid input Outputs: []Output{output1}}}, {false, SendMsg{ Inputs: []Input{input1}, From 087721dc049b9f1a8fd4fc9dbf0fb048b6eec713 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 4 Mar 2018 02:50:57 -0500 Subject: [PATCH 4/9] x/bank: clean up tests --- x/bank/tx_test.go | 115 +++++++++++++++------------------------------- 1 file changed, 38 insertions(+), 77 deletions(-) diff --git a/x/bank/tx_test.go b/x/bank/tx_test.go index 6d544626a..bbfdc62ff 100644 --- a/x/bank/tx_test.go +++ b/x/bank/tx_test.go @@ -179,82 +179,54 @@ func TestSendMsgValidation(t *testing.T) { func TestSendMsgString(t *testing.T) { // Construct a SendMsg + addr1 := sdk.Address([]byte("input")) + addr2 := sdk.Address([]byte("output")) + coins := sdk.Coins{{"atom", 10}} var msg = SendMsg{ - Inputs: []Input{ - { - Address: sdk.Address([]byte("input")), - Coins: sdk.Coins{{"atom", 10}}, - Sequence: 1, - }, - }, - Outputs: []Output{ - { - Address: sdk.Address([]byte("output")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Inputs: []Input{NewInput(addr1, coins)}, + Outputs: []Output{NewOutput(addr2, coins)}, } + res := msg.String() // TODO some failures for bad results assert.Equal(t, res, "SendMsg{[Input{696E707574,10atom}]->[Output{364637353734373037353734,10atom}]}") } func TestSendMsgGet(t *testing.T) { + addr1 := sdk.Address([]byte("input")) + addr2 := sdk.Address([]byte("output")) + coins := sdk.Coins{{"atom", 10}} var msg = SendMsg{ - Inputs: []Input{ - { - Address: sdk.Address([]byte("input")), - Coins: sdk.Coins{{"atom", 10}}, - Sequence: 1, - }, - }, - Outputs: []Output{ - { - Address: sdk.Address([]byte("output")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Inputs: []Input{NewInput(addr1, coins)}, + Outputs: []Output{NewOutput(addr2, coins)}, } res := msg.Get(nil) assert.Nil(t, res) } func TestSendMsgGetSignBytes(t *testing.T) { + addr1 := sdk.Address([]byte("input")) + addr2 := sdk.Address([]byte("output")) + coins := sdk.Coins{{"atom", 10}} var msg = SendMsg{ - Inputs: []Input{ - { - Address: sdk.Address([]byte("input")), - Coins: sdk.Coins{{"atom", 10}}, - Sequence: 1, - }, - }, - Outputs: []Output{ - { - Address: sdk.Address([]byte("output")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Inputs: []Input{NewInput(addr1, coins)}, + Outputs: []Output{NewOutput(addr2, coins)}, } res := msg.GetSignBytes() // TODO bad results - assert.Equal(t, string(res), `{"inputs":[{"address":"696E707574","coins":[{"denom":"atom","amount":10}],"sequence":1}],"outputs":[{"address":"6F7574707574","coins":[{"denom":"atom","amount":10}]}]}`) + assert.Equal(t, string(res), `{"inputs":[{"address":"696E707574","coins":[{"denom":"atom","amount":10}]}],"outputs":[{"address":"6F7574707574","coins":[{"denom":"atom","amount":10}]}]}`) } func TestSendMsgGetSigners(t *testing.T) { var msg = SendMsg{ Inputs: []Input{ - { - Address: sdk.Address([]byte("input1")), - }, - { - Address: sdk.Address([]byte("input2")), - }, - { - Address: sdk.Address([]byte("input3")), - }, + NewInput(sdk.Address([]byte("input1")), nil), + NewInput(sdk.Address([]byte("input2")), nil), + NewInput(sdk.Address([]byte("input3")), nil), }, } res := msg.GetSigners() + // TODO: fix this ! assert.Equal(t, fmt.Sprintf("%v", res), "[696E70757431 696E70757432 696E70757433]") } @@ -287,14 +259,11 @@ func TestNewIssueMsg(t *testing.T) { func TestIssueMsgType(t *testing.T) { // Construct an IssueMsg + addr := sdk.Address([]byte("loan-from-bank")) + coins := sdk.Coins{{"atom", 10}} var msg = IssueMsg{ - Banker: sdk.Address([]byte("input")), - Outputs: []Output{ - { - Address: sdk.Address([]byte("loan-from-bank")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Banker: sdk.Address([]byte("input")), + Outputs: []Output{NewOutput(addr, coins)}, } // TODO some failures for bad result @@ -307,42 +276,34 @@ func TestIssueMsgValidation(t *testing.T) { func TestIssueMsgString(t *testing.T) { // Construct a IssueMsg + addr := sdk.Address([]byte("loan-from-bank")) + coins := sdk.Coins{{"atom", 10}} var msg = IssueMsg{ - Banker: sdk.Address([]byte("input")), - Outputs: []Output{ - { - Address: sdk.Address([]byte("loan-from-bank")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Banker: sdk.Address([]byte("input")), + Outputs: []Output{NewOutput(addr, coins)}, } res := msg.String() + // TODO: FIX THIS OUTPUT! assert.Equal(t, res, "IssueMsg{696E707574#[Output{36433646363136453244363637323646364432443632363136453642,10atom}]}") } func TestIssueMsgGet(t *testing.T) { + addr := sdk.Address([]byte("loan-from-bank")) + coins := sdk.Coins{{"atom", 10}} var msg = IssueMsg{ - Banker: sdk.Address([]byte("input")), - Outputs: []Output{ - { - Address: sdk.Address([]byte("loan-from-bank")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Banker: sdk.Address([]byte("input")), + Outputs: []Output{NewOutput(addr, coins)}, } res := msg.Get(nil) assert.Nil(t, res) } func TestIssueMsgGetSignBytes(t *testing.T) { + addr := sdk.Address([]byte("loan-from-bank")) + coins := sdk.Coins{{"atom", 10}} var msg = IssueMsg{ - Banker: sdk.Address([]byte("input")), - Outputs: []Output{ - { - Address: sdk.Address([]byte("loan-from-bank")), - Coins: sdk.Coins{{"atom", 10}}, - }, - }, + Banker: sdk.Address([]byte("input")), + Outputs: []Output{NewOutput(addr, coins)}, } res := msg.GetSignBytes() // TODO bad results From a05ea5fcc95579db2eaa2c79e44cf59172cfca72 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 4 Mar 2018 03:15:26 -0500 Subject: [PATCH 5/9] implement replay protection --- examples/basecoin/app/app_test.go | 19 +++++++++++++++++++ types/errors.go | 16 +++++----------- types/tx_msg.go | 14 ++++++++++++++ x/auth/ante.go | 16 ++++++++++------ 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index 1349b6767..a187058a8 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -169,6 +169,9 @@ func TestSendMsgWithAccounts(t *testing.T) { assert.Equal(t, acc1, res1) // Sign the tx + chainID := "" // TODO: InitChain should get the ChainID + sequence := int64(0) + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequence, sendMsg)) tx := sdk.NewStdTx(sendMsg, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: priv1.Sign(sendMsg.GetSignBytes()), @@ -189,6 +192,22 @@ func TestSendMsgWithAccounts(t *testing.T) { res3 := bapp.accountMapper.GetAccount(ctxDeliver, addr2) assert.Equal(t, fmt.Sprintf("%v", res2.GetCoins()), "67foocoin") assert.Equal(t, fmt.Sprintf("%v", res3.GetCoins()), "10foocoin") + + // Delivering again should cause replay error + res = bapp.Deliver(tx) + assert.Equal(t, sdk.CodeInvalidSequence, res.Code, res.Log) + + // bumping the txnonce number without resigning should be an auth error + sequence += 1 + tx.Signatures[0].Sequence = sequence + res = bapp.Deliver(tx) + assert.Equal(t, sdk.CodeUnauthorized, res.Code, res.Log) + + // resigning the tx with the bumped sequence should work + sig = priv1.Sign(sdk.StdSignBytes(chainID, sequence, tx.Msg)) + tx.Signatures[0].Signature = sig + res = bapp.Deliver(tx) + assert.Equal(t, sdk.CodeOK, res.Code, res.Log) } //func TestWhatCoolMsg(t *testing.T) { diff --git a/types/errors.go b/types/errors.go index e52fe625f..9d8175e30 100644 --- a/types/errors.go +++ b/types/errors.go @@ -22,12 +22,11 @@ const ( CodeOK CodeType = 0 CodeInternal CodeType = 1 CodeTxParse CodeType = 2 - CodeBadNonce CodeType = 3 + CodeInvalidSequence CodeType = 3 CodeUnauthorized CodeType = 4 CodeInsufficientFunds CodeType = 5 CodeUnknownRequest CodeType = 6 CodeUnrecognizedAddress CodeType = 7 - CodeInvalidSequence CodeType = 8 CodeGenesisParse CodeType = 0xdead // TODO: remove ? ) @@ -41,8 +40,8 @@ func CodeToDefaultMsg(code CodeType) string { return "Tx parse error" case CodeGenesisParse: return "Genesis parse error" - case CodeBadNonce: - return "Bad nonce" + case CodeInvalidSequence: + return "Invalid sequence" case CodeUnauthorized: return "Unauthorized" case CodeInsufficientFunds: @@ -51,8 +50,6 @@ func CodeToDefaultMsg(code CodeType) string { return "Unknown request" case CodeUnrecognizedAddress: return "Unrecognized address" - case CodeInvalidSequence: - return "Invalid sequence" default: return fmt.Sprintf("Unknown code %d", code) } @@ -72,8 +69,8 @@ func ErrTxParse(msg string) Error { func ErrGenesisParse(msg string) Error { return newError(CodeGenesisParse, msg) } -func ErrBadNonce(msg string) Error { - return newError(CodeBadNonce, msg) +func ErrInvalidSequence(msg string) Error { + return newError(CodeInvalidSequence, msg) } func ErrUnauthorized(msg string) Error { return newError(CodeUnauthorized, msg) @@ -87,9 +84,6 @@ func ErrUnknownRequest(msg string) Error { func ErrUnrecognizedAddress(addr Address) Error { return newError(CodeUnrecognizedAddress, addr.String()) } -func ErrInvalidSequence(msg string) Error { - return newError(CodeInvalidSequence, msg) -} //---------------------------------------- // Error & sdkError diff --git a/types/tx_msg.go b/types/tx_msg.go index e97fc331f..c88107c51 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -1,5 +1,7 @@ package types +import "encoding/json" + // Transactions messages must fulfill the Msg type Msg interface { @@ -77,6 +79,18 @@ type StdSignDoc struct { AltBytes []byte `json:"alt_bytes"` // TODO: do we really want this ? } +func StdSignBytes(chainID string, sequence int64, msg Msg) []byte { + bz, err := json.Marshal(StdSignDoc{ + ChainID: chainID, + Sequence: sequence, + MsgBytes: msg.GetSignBytes(), + }) + if err != nil { + panic(err) + } + return bz +} + //------------------------------------- // Application function variable used to unmarshal transaction bytes diff --git a/x/auth/ante.go b/x/auth/ante.go index 17e9ccdd9..7ab677e88 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -31,13 +31,16 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // Collect accounts to set in the context var signerAccs = make([]sdk.Account, len(signerAddrs)) - signBytes := msg.GetSignBytes() - // First sig is the fee payer. - // Check sig and nonce, deduct fee. + // signBytes uses the sequence of the fee payer + // (ie. the first account) + payerAddr, payerSig := signerAddrs[0], sigs[0] + signBytes := sdk.StdSignBytes(ctx.ChainID(), payerSig.Sequence, msg) + + // Check fee payer sig and nonce, and deduct fee. // This is done first because it only // requires fetching 1 account. - payerAcc, res := processSig(ctx, accountMapper, signerAddrs[0], sigs[0], signBytes) + payerAcc, res := processSig(ctx, accountMapper, payerAddr, payerSig, signBytes) if !res.IsOK() { return ctx, res, true } @@ -47,7 +50,8 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // Check sig and nonce for the rest. for i := 1; i < len(sigs); i++ { - signerAcc, res := processSig(ctx, accountMapper, signerAddrs[i], sigs[i], signBytes) + signerAddr, sig := signerAddrs[i], sigs[i] + signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, signBytes) if !res.IsOK() { return ctx, res, true } @@ -90,7 +94,7 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk // Check sig. if !sig.PubKey.VerifyBytes(signBytes, sig.Signature) { - return nil, sdk.ErrUnauthorized("").Result() + return nil, sdk.ErrUnauthorized("signature verification failed").Result() } // Save the account. From e4da8ebee2356460db1f94dab65137238155c11b Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 11 Mar 2018 00:28:46 +0100 Subject: [PATCH 6/9] fixes from rebase --- examples/basecoin/app/app_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index a187058a8..595d75f38 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -21,16 +21,13 @@ import ( "github.com/tendermint/tmlibs/log" ) -// helper variables and functions - +// Construct some global addrs and txs for tests. var ( - // Construct genesis key/accounts priv1 = crypto.GenPrivKeyEd25519() addr1 = priv1.PubKey().Address() addr2 = crypto.GenPrivKeyEd25519().PubKey().Address() coins = sdk.Coins{{"foocoin", 10}} - // Construct a SendMsg sendMsg = bank.SendMsg{ Inputs: []bank.Input{bank.NewInput(addr1, coins)}, Outputs: []bank.Output{bank.NewOutput(addr2, coins)}, @@ -71,8 +68,10 @@ func TestMsgs(t *testing.T) { {setWhatCoolMsg}, } + chainID := "" + sequence := int64(0) for i, m := range msgs { - sig := priv1.Sign(m.msg.GetSignBytes()) + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequence, m.msg)) tx := sdk.NewStdTx(m.msg, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: sig, @@ -174,7 +173,7 @@ func TestSendMsgWithAccounts(t *testing.T) { sig := priv1.Sign(sdk.StdSignBytes(chainID, sequence, sendMsg)) tx := sdk.NewStdTx(sendMsg, []sdk.StdSignature{{ PubKey: priv1.PubKey(), - Signature: priv1.Sign(sendMsg.GetSignBytes()), + Signature: sig, }}) // Run a Check From 661d0fd7e8055b9710a73a3967041cb14363a6d0 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 11 Mar 2018 00:57:39 +0100 Subject: [PATCH 7/9] types: StdSignDoc includes sequence for each sig --- examples/basecoin/app/app_test.go | 14 +++++++------- types/tx_msg.go | 19 ++++++++++--------- x/auth/ante.go | 12 +++++++----- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index 595d75f38..148f0411f 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -69,9 +69,9 @@ func TestMsgs(t *testing.T) { } chainID := "" - sequence := int64(0) + sequences := []int64{0} for i, m := range msgs { - sig := priv1.Sign(sdk.StdSignBytes(chainID, sequence, m.msg)) + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, m.msg)) tx := sdk.NewStdTx(m.msg, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: sig, @@ -169,8 +169,8 @@ func TestSendMsgWithAccounts(t *testing.T) { // Sign the tx chainID := "" // TODO: InitChain should get the ChainID - sequence := int64(0) - sig := priv1.Sign(sdk.StdSignBytes(chainID, sequence, sendMsg)) + sequences := []int64{0} + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, sendMsg)) tx := sdk.NewStdTx(sendMsg, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: sig, @@ -197,13 +197,13 @@ func TestSendMsgWithAccounts(t *testing.T) { assert.Equal(t, sdk.CodeInvalidSequence, res.Code, res.Log) // bumping the txnonce number without resigning should be an auth error - sequence += 1 - tx.Signatures[0].Sequence = sequence + tx.Signatures[0].Sequence = 1 res = bapp.Deliver(tx) assert.Equal(t, sdk.CodeUnauthorized, res.Code, res.Log) // resigning the tx with the bumped sequence should work - sig = priv1.Sign(sdk.StdSignBytes(chainID, sequence, tx.Msg)) + sequences = []int64{1} + sig = priv1.Sign(sdk.StdSignBytes(chainID, sequences, tx.Msg)) tx.Signatures[0].Signature = sig res = bapp.Deliver(tx) assert.Equal(t, sdk.CodeOK, res.Code, res.Log) diff --git a/types/tx_msg.go b/types/tx_msg.go index c88107c51..63f934726 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -71,19 +71,20 @@ func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } // StdSignDoc is replay-prevention structure. // It includes the result of msg.GetSignBytes(), // as well as the ChainID (prevent cross chain replay) -// and the Sequence (prevent inchain replay). +// and the Sequence numbers for each signature (prevent +// inchain replay and enforce tx ordering per account). type StdSignDoc struct { - ChainID string `json:"chain_id"` - Sequence int64 `json:"sequence"` - MsgBytes []byte `json:"msg_bytes"` - AltBytes []byte `json:"alt_bytes"` // TODO: do we really want this ? + ChainID string `json:"chain_id"` + Sequences []int64 `json:"sequences"` + MsgBytes []byte `json:"msg_bytes"` + AltBytes []byte `json:"alt_bytes"` // TODO: do we really want this ? } -func StdSignBytes(chainID string, sequence int64, msg Msg) []byte { +func StdSignBytes(chainID string, sequences []int64, msg Msg) []byte { bz, err := json.Marshal(StdSignDoc{ - ChainID: chainID, - Sequence: sequence, - MsgBytes: msg.GetSignBytes(), + ChainID: chainID, + Sequences: sequences, + MsgBytes: msg.GetSignBytes(), }) if err != nil { panic(err) diff --git a/x/auth/ante.go b/x/auth/ante.go index 7ab677e88..f54435e21 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -31,15 +31,17 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // Collect accounts to set in the context var signerAccs = make([]sdk.Account, len(signerAddrs)) - // First sig is the fee payer. - // signBytes uses the sequence of the fee payer - // (ie. the first account) - payerAddr, payerSig := signerAddrs[0], sigs[0] - signBytes := sdk.StdSignBytes(ctx.ChainID(), payerSig.Sequence, msg) + // Get the sign bytes by collecting all sequence numbers + sequences := make([]int64, len(signerAddrs)) + for i := 0; i < len(signerAddrs); i++ { + sequences[i] = sigs[i].Sequence + } + signBytes := sdk.StdSignBytes(ctx.ChainID(), sequences, msg) // Check fee payer sig and nonce, and deduct fee. // This is done first because it only // requires fetching 1 account. + payerAddr, payerSig := signerAddrs[0], sigs[0] payerAcc, res := processSig(ctx, accountMapper, payerAddr, payerSig, signBytes) if !res.IsOK() { return ctx, res, true From 1955e3436fd9d0c6bedcc2c1196b6d40920df5fa Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 12 Mar 2018 23:25:55 +0100 Subject: [PATCH 8/9] x/auth: mapper_test.go --- x/auth/mapper.go | 4 +++ x/auth/mapper_test.go | 81 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 x/auth/mapper_test.go diff --git a/x/auth/mapper.go b/x/auth/mapper.go index b50e4405f..1176db3cb 100644 --- a/x/auth/mapper.go +++ b/x/auth/mapper.go @@ -11,6 +11,9 @@ import ( wire "github.com/cosmos/cosmos-sdk/wire" ) +var _ sdk.AccountMapper = (*accountMapper)(nil) +var _ sdk.AccountMapper = (*sealedAccountMapper)(nil) + // Implements sdk.AccountMapper. // This AccountMapper encodes/decodes accounts using the // go-wire (binary) encoding/decoding library. @@ -108,6 +111,7 @@ func (sam sealedAccountMapper) WireCodec() *wire.Codec { //---------------------------------------- // misc. +// NOTE: currently unused func (am accountMapper) clonePrototypePtr() interface{} { protoRt := reflect.TypeOf(am.proto) if protoRt.Kind() == reflect.Ptr { diff --git a/x/auth/mapper_test.go b/x/auth/mapper_test.go new file mode 100644 index 000000000..4ac96c381 --- /dev/null +++ b/x/auth/mapper_test.go @@ -0,0 +1,81 @@ +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + abci "github.com/tendermint/abci/types" + crypto "github.com/tendermint/go-crypto" + oldwire "github.com/tendermint/go-wire" + dbm "github.com/tendermint/tmlibs/db" + + "github.com/cosmos/cosmos-sdk/store" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +func setupMultiStore() (sdk.MultiStore, *sdk.KVStoreKey) { + db := dbm.NewMemDB() + capKey := sdk.NewKVStoreKey("capkey") + ms := store.NewCommitMultiStore(db) + ms.MountStoreWithDB(capKey, sdk.StoreTypeIAVL, db) + ms.LoadLatestVersion() + + // wire registration while we're at it ... TODO + var _ = oldwire.RegisterInterface( + struct{ sdk.Account }{}, + oldwire.ConcreteType{&BaseAccount{}, 0x1}, + ) + + return ms, capKey +} + +func TestAccountMapperGetSet(t *testing.T) { + ms, capKey := setupMultiStore() + + // make context and mapper + ctx := sdk.NewContext(ms, abci.Header{}, false, nil) + mapper := NewAccountMapper(capKey, &BaseAccount{}) + + addr := sdk.Address([]byte("some-address")) + + // no account before its created + acc := mapper.GetAccount(ctx, addr) + assert.Nil(t, acc) + + // create account and check default values + acc = mapper.NewAccountWithAddress(ctx, addr) + assert.NotNil(t, acc) + assert.Equal(t, addr, acc.GetAddress()) + assert.EqualValues(t, crypto.PubKey{}, acc.GetPubKey()) + assert.EqualValues(t, 0, acc.GetSequence()) + + // NewAccount doesn't call Set, so it's still nil + assert.Nil(t, mapper.GetAccount(ctx, addr)) + + // set some values on the account and save it + newSequence := int64(20) + acc.SetSequence(newSequence) + mapper.SetAccount(ctx, acc) + + // check the new values + acc = mapper.GetAccount(ctx, addr) + assert.NotNil(t, acc) + assert.Equal(t, newSequence, acc.GetSequence()) +} + +func TestAccountMapperSealed(t *testing.T) { + _, capKey := setupMultiStore() + + // normal mapper exposes the wire codec + mapper := NewAccountMapper(capKey, &BaseAccount{}) + assert.NotNil(t, mapper.WireCodec()) + + // seal mapper, should panic when we try to get the codec + mapperSealed := mapper.Seal() + assert.Panics(t, func() { mapperSealed.WireCodec() }) + + // another way to get a sealed mapper + mapperSealed = NewAccountMapperSealed(capKey, &BaseAccount{}) + assert.Panics(t, func() { mapperSealed.WireCodec() }) +} From 20e2c24f9dfe821fdc34181ceab67f63668ba3b9 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 13 Mar 2018 01:40:04 +0100 Subject: [PATCH 9/9] x/auth: ante_test.go --- x/auth/ante.go | 5 +- x/auth/ante_test.go | 163 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 x/auth/ante_test.go diff --git a/x/auth/ante.go b/x/auth/ante.go index f54435e21..48e2344c6 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -1,6 +1,8 @@ package auth import ( + "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -78,7 +80,8 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk // Check and increment sequence number. seq := acc.GetSequence() if seq != sig.Sequence { - return nil, sdk.ErrInvalidSequence("").Result() + return nil, sdk.ErrInvalidSequence( + fmt.Sprintf("Invalid sequence. Got %d, expected %d", sig.Sequence, seq)).Result() } acc.SetSequence(seq + 1) diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go new file mode 100644 index 000000000..17ea204d3 --- /dev/null +++ b/x/auth/ante_test.go @@ -0,0 +1,163 @@ +package auth + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/assert" + abci "github.com/tendermint/abci/types" + crypto "github.com/tendermint/go-crypto" +) + +// msg type for testing +type testMsg struct { + signBytes []byte + signers []sdk.Address +} + +func newTestMsg(addrs ...sdk.Address) *testMsg { + return &testMsg{ + signBytes: []byte("some sign bytes"), + signers: addrs, + } +} + +func (msg *testMsg) Type() string { return "testMsg" } +func (msg *testMsg) Get(key interface{}) (value interface{}) { return nil } +func (msg *testMsg) GetSignBytes() []byte { + return msg.signBytes +} +func (msg *testMsg) ValidateBasic() sdk.Error { return nil } +func (msg *testMsg) GetSigners() []sdk.Address { + return msg.signers +} + +// generate a priv key and return it with its address +func privAndAddr() (crypto.PrivKey, sdk.Address) { + priv := crypto.GenPrivKeyEd25519() + addr := priv.PubKey().Address() + return priv.Wrap(), addr +} + +// run the tx through the anteHandler and ensure its valid +func checkValidTx(t *testing.T, anteHandler sdk.AnteHandler, ctx sdk.Context, tx sdk.Tx) { + _, result, abort := anteHandler(ctx, tx) + assert.False(t, abort) + assert.Equal(t, sdk.CodeOK, result.Code) + assert.True(t, result.IsOK()) +} + +// run the tx through the anteHandler and ensure it fails with the given code +func checkInvalidTx(t *testing.T, anteHandler sdk.AnteHandler, ctx sdk.Context, tx sdk.Tx, code sdk.CodeType) { + _, result, abort := anteHandler(ctx, tx) + assert.True(t, abort) + assert.Equal(t, code, result.Code) +} + +func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, seqs []int64) sdk.Tx { + signBytes := sdk.StdSignBytes(ctx.ChainID(), seqs, msg) + sigs := make([]sdk.StdSignature, len(privs)) + for i, priv := range privs { + sigs[i] = sdk.StdSignature{PubKey: priv.PubKey(), Signature: priv.Sign(signBytes), Sequence: seqs[i]} + } + return sdk.NewStdTx(msg, sigs) +} + +// Test various error cases in the AnteHandler control flow. +func TestAnteHandlerSigErrors(t *testing.T) { + // setup + ms, capKey := setupMultiStore() + mapper := NewAccountMapper(capKey, &BaseAccount{}) + anteHandler := NewAnteHandler(mapper) + ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) + + // keys and addresses + priv1, addr1 := privAndAddr() + priv2, addr2 := privAndAddr() + + // msg and signatures + var tx sdk.Tx + msg := newTestMsg(addr1, addr2) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{0, 0}) + + // test no signatures + tx = newTestTx(ctx, msg, []crypto.PrivKey{}, []int64{}) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) + + // test num sigs dont match GetSigners + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) + + // test an unrecognized account + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{0, 0}) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnrecognizedAddress) + + // save the first account, but second is still unrecognized + acc1 := mapper.NewAccountWithAddress(ctx, addr1) + mapper.SetAccount(ctx, acc1) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnrecognizedAddress) +} + +// Test logic around sequence checking with one signer and many signers. +func TestAnteHandlerSequences(t *testing.T) { + // setup + ms, capKey := setupMultiStore() + mapper := NewAccountMapper(capKey, &BaseAccount{}) + anteHandler := NewAnteHandler(mapper) + ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) + + // keys and addresses + priv1, addr1 := privAndAddr() + priv2, addr2 := privAndAddr() + + // set the accounts + acc1 := mapper.NewAccountWithAddress(ctx, addr1) + mapper.SetAccount(ctx, acc1) + acc2 := mapper.NewAccountWithAddress(ctx, addr2) + mapper.SetAccount(ctx, acc2) + + // msg and signatures + var tx sdk.Tx + msg := newTestMsg(addr1) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + + // test good tx from one signer + checkValidTx(t, anteHandler, ctx, tx) + + // test sending it again fails (replay protection) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) + + // fix sequence, should pass + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{1}) + checkValidTx(t, anteHandler, ctx, tx) + + // new tx with another signer and correct sequences + msg = newTestMsg(addr1, addr2) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{2, 0}) + checkValidTx(t, anteHandler, ctx, tx) + + // replay fails + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) + + // tx from just second signer with incorrect sequence fails + msg = newTestMsg(addr2) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{0}) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) + + // fix the sequence and it passes + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}) + checkValidTx(t, anteHandler, ctx, tx) + + // another tx from both of them that passes + msg = newTestMsg(addr1, addr2) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{3, 2}) + checkValidTx(t, anteHandler, ctx, tx) +} + +func TestAnteHandlerBadSignBytes(t *testing.T) { + // TODO: test various cases of bad sign bytes +} + +func TestAnteHandlerSetPubKey(t *testing.T) { + // TODO: test cases where pubkey is already set on the account +}