Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Prevent signing from wrong key in multisig #12446

Merged
merged 9 commits into from
Jul 12, 2022
Merged

fix: Prevent signing from wrong key in multisig #12446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5427e73
check if signing key is part of a multisig ke while using multi-sign
likhita-809 Jul 5, 2022
4d82385
fix a test
likhita-809 Jul 5, 2022
f219b5e
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Jul 6, 2022
fe4fca0
wip
likhita-809 Jul 6, 2022
15eb5ce
add some logs for testing
likhita-809 Jul 12, 2022
b19f524
wip
likhita-809 Jul 12, 2022
dcb2ff0
add a test case
likhita-809 Jul 12, 2022
032d822
add more tests
likhita-809 Jul 12, 2022
cbdf185
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/keys/show_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func Test_multiSigKey_Properties(t *testing.T) {
require.Equal(t, "myMultisig", k.Name)
require.Equal(t, keyring.TypeMulti, k.GetType())

pub, err := k.GetPubKey()
pub, err := k.GetMultisigPubKey()
require.NoError(t, err)
require.Equal(t, "D3923267FA8A3DD367BB768FA8BDC8FF7F89DA3F", pub.Address().String())

Expand Down
2 changes: 1 addition & 1 deletion crypto/keyring/output_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestBech32KeysOutput(t *testing.T) {
k, err := NewMultiRecord("multisig", multisigPk)
require.NotNil(t, k)
require.NoError(t, err)
pubKey, err := k.GetPubKey()
pubKey, err := k.GetMultisigPubKey()
require.NoError(t, err)
accAddr := sdk.AccAddress(pubKey.Address())
expectedOutput, err := NewKeyOutput(k.Name, k.GetType(), accAddr, multisigPk)
Expand Down
11 changes: 11 additions & 0 deletions crypto/keyring/record.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/types"
)
Expand Down Expand Up @@ -59,6 +60,16 @@ func NewMultiRecord(name string, pk cryptotypes.PubKey) (*Record, error) {
return newRecord(name, pk, recordMultiItem)
}

// GetMultisigPubKey fetches a public key of the multi type record
func (k *Record) GetMultisigPubKey() (*multisig.LegacyAminoPubKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good API on Record: there should only be a method to get the PubKey.

Then, in tx_sign.go, we can cast .GetPubKey().(*multisig.LegacyAminoPubKey)

pk, ok := k.PubKey.GetCachedValue().(*multisig.LegacyAminoPubKey)
if !ok {
return nil, errors.New("unable to cast any to multisig.LegacyAminoPubKey")
}

return pk, nil
}

// GetPubKey fetches a public key of the record
func (k *Record) GetPubKey() (cryptotypes.PubKey, error) {
pk, ok := k.PubKey.GetCachedValue().(cryptotypes.PubKey)
Expand Down
45 changes: 41 additions & 4 deletions crypto/keyring/record_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,35 @@ import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
)

type RecordTestSuite struct {
suite.Suite

appName string
cdc codec.Codec
priv cryptotypes.PrivKey
pub cryptotypes.PubKey
appName string
cdc codec.Codec
priv cryptotypes.PrivKey
pub cryptotypes.PubKey
multiPub *multisig.LegacyAminoPubKey
}

func (s *RecordTestSuite) SetupSuite() {
s.appName = "cosmos"
s.cdc = getCodec()
s.priv = cryptotypes.PrivKey(ed25519.GenPrivKey())
s.pub = s.priv.PubKey()

multiKey := secp256k1.GenPrivKeyFromSecret([]byte("myMultiKey"))
pk := multisig.NewLegacyAminoPubKey(
1,
[]cryptotypes.PubKey{multiKey.PubKey()},
)
s.multiPub = pk

}

func (s *RecordTestSuite) TestOfflineRecordMarshaling() {
Expand All @@ -47,6 +58,32 @@ func (s *RecordTestSuite) TestOfflineRecordMarshaling() {
s.Require().True(s.pub.Equals(pk2))
}

func (s *RecordTestSuite) TestMultiRecordMarshaling() {
dir := s.T().TempDir()
mockIn := strings.NewReader("")

kb, err := New(s.appName, BackendTest, dir, mockIn, s.cdc)
s.Require().NoError(err)

k, err := NewMultiRecord("testrecord", s.multiPub)
s.Require().NoError(err)

ks, ok := kb.(keystore)
s.Require().True(ok)

bz, err := ks.cdc.Marshal(k)
s.Require().NoError(err)

k2, err := ks.protoUnmarshalRecord(bz)
s.Require().NoError(err)
s.Require().Equal(k.Name, k2.Name)
s.Require().True(k.PubKey.Equal(k2.PubKey))

pub2, err := k2.GetMultisigPubKey()
s.Require().NoError(err)
s.Require().True(s.multiPub.Equals(pub2))
}

func (s *RecordTestSuite) TestLocalRecordMarshaling() {
dir := s.T().TempDir()
mockIn := strings.NewReader("")
Expand Down
44 changes: 36 additions & 8 deletions x/auth/client/cli/tx_sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,14 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
return err
}

txFactory := tx.NewFactoryCLI(clientCtx, cmd.Flags())
txCfg := clientCtx.TxConfig
txBuilder, err := txCfg.WrapTxBuilder(newTx)
if err != nil {
return err
}

printSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly)
multisig, _ := cmd.Flags().GetString(flagMultisig)
multisigkey, _ := cmd.Flags().GetString(flagMultisig)
if err != nil {
return err
}
Expand All @@ -243,15 +242,44 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
}

overwrite, _ := f.GetBool(flagOverwrite)
if multisig != "" {
multisigAddr, err := sdk.AccAddressFromBech32(multisig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this line got deleted. @likhita-809 is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, because at the line L251, we are getting both name and address of the multisig irrespective of multisig flag value(be it name or address).

if multisigkey != "" {
var (
multisigName string
multisigAddr sdk.AccAddress
)
// Bech32 decode error, maybe it's a name, we try to fetch from keyring
multisigAddr, multisigName, _, err = client.GetFromFields(clientCtx, txF.Keybase(), multisigkey)
if err != nil {
// Bech32 decode error, maybe it's a name, we try to fetch from keyring
multisigAddr, _, _, err = client.GetFromFields(clientCtx, txFactory.Keybase(), multisig)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
return fmt.Errorf("error getting account from keybase: %w", err)
}
multisigRecord, err := clientCtx.Keyring.Key(multisigName)
if err != nil {
return err
}
multisigPub, err := multisigRecord.GetMultisigPubKey()
if err != nil {
return err
}

fromRecord, err := clientCtx.Keyring.Key(fromName)
if err != nil {
return err
}
fromPubKey, err := fromRecord.GetPubKey()
if err != nil {
return err
}

var found bool
for _, pubkey := range multisigPub.GetPubKeys() {
if pubkey.Equals(fromPubKey) {
found = true
}
}
if !found {
return fmt.Errorf("from key is not a part of multisig key")
}

err = authclient.SignTxWithSignerAddress(
txF, clientCtx, multisigAddr, fromName, txBuilder, clientCtx.Offline, overwrite)
if err != nil {
Expand Down
21 changes: 18 additions & 3 deletions x/auth/client/testutil/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ func (s *IntegrationTestSuite) SetupSuite() {
pub2, err := account2.GetPubKey()
s.Require().NoError(err)

// Create a dummy account for testing purpose
_, _, err = kb.NewMnemonic("dummyAccount", keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
s.Require().NoError(err)

multi := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pub1, pub2})
_, err = kb.SaveMultisig("multi", multi)
s.Require().NoError(err)
Expand Down Expand Up @@ -938,6 +942,10 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {
multisigRecord, err := val1.ClientCtx.Keyring.Key("multi")
s.Require().NoError(err)

// Generate dummy account which is not a part of multisig.
dummyAcc, err := val1.ClientCtx.Keyring.Key("dummyAccount")
s.Require().NoError(err)

addr, err := multisigRecord.GetAddress()
s.Require().NoError(err)
resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, addr)
Expand Down Expand Up @@ -995,14 +1003,21 @@ func (s *IntegrationTestSuite) TestCLIMultisignSortSignatures() {

sign1File := testutil.WriteToNewTempFile(s.T(), account1Signature.String())

// Sign with account1
// Sign with account2
addr2, err := account2.GetAddress()
s.Require().NoError(err)
account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String())
s.Require().NoError(err)

sign2File := testutil.WriteToNewTempFile(s.T(), account2Signature.String())

// Sign with dummy account
dummyAddr, err := dummyAcc.GetAddress()
s.Require().NoError(err)
_, err = TxSignExec(val1.ClientCtx, dummyAddr, multiGeneratedTxFile.Name(), "--multisig", addr.String())
s.Require().Error(err)
s.Require().Contains(err.Error(), "from key is not a part of multisig key")

multiSigWith2Signatures, err := TxMultiSignExec(val1.ClientCtx, multisigRecord.Name, multiGeneratedTxFile.Name(), sign1File.Name(), sign2File.Name())
s.Require().NoError(err)

Expand Down Expand Up @@ -1057,7 +1072,7 @@ func (s *IntegrationTestSuite) TestSignWithMultisig() {
// as the main point of this test is to test the `--multisig` flag with an address
// that is not in the keyring.
_, err = TxSignExec(val1.ClientCtx, addr1, multiGeneratedTx2File.Name(), "--multisig", multisigAddr.String())
s.Require().Contains(err.Error(), "tx intended signer does not match the given signer")
s.Require().Contains(err.Error(), "error getting account from keybase")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we changed this test seems a bit suspicious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test, we are passing multisig flag value as its address.
As you can see here https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/client/cli/tx_sign.go#L247, since it is an address the error check will be skipped. But if multisig flag is a name, then we'll encounter error getting account from keybase error.

}

func (s *IntegrationTestSuite) TestCLIMultisign() {
Expand Down Expand Up @@ -1124,7 +1139,7 @@ func (s *IntegrationTestSuite) TestCLIMultisign() {

addr2, err := account2.GetAddress()
s.Require().NoError(err)
// Sign with account1
// Sign with account2
account2Signature, err := TxSignExec(val1.ClientCtx, addr2, multiGeneratedTxFile.Name(), "--multisig", addr.String())
s.Require().NoError(err)

Expand Down