Merge branch 'master' into fedekunze/keys-add-ledger

CHANGELOG.md

@@ -41,6 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * [\#9533](https://github.com/cosmos/cosmos-sdk/pull/9533) Added a new gRPC method, `DenomOwners`, in `x/bank` to query for all account holders of a specific denomination.
 * (bank) [\#9618](https://github.com/cosmos/cosmos-sdk/pull/9618) Update bank.Metadata: add URI and URIHash attributes.
+* [\#9750](https://github.com/cosmos/cosmos-sdk/pull/9750) Emit events for tx signature and sequence, so clients can now query txs by signature (`tx.signature='<base64_sig>'`) or by address and sequence combo (`tx.acc_seq='<addr>/<seq>'`).
 
 ### API Breaking Changes
 
@@ -88,6 +89,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/genutil) [\#9638](https://github.com/cosmos/cosmos-sdk/pull/9638) Added missing validator key save when recovering from mnemonic
 * (server) [#9704](https://github.com/cosmos/cosmos-sdk/pull/9704) Start GRPCWebServer in goroutine, avoid blocking other services from starting.
 * [\#9762](https://github.com/cosmos/cosmos-sdk/pull/9762) The init command uses the chain-id from the client config if --chain-id is not provided
+* [\#9793](https://github.com/cosmos/cosmos-sdk/pull/9793) Fixed ECDSA/secp256r1 transaction malleability.
 
 ### State Machine Breaking
 

CONTRIBUTING.md

@@ -65,10 +65,7 @@ Other notes:
 
 ## Architecture Decision Records (ADR)
 
-When proposing an architecture decision for the SDK, please create an [ADR](./docs/architecture/README.md)
-so further discussions can be made. We are following this process so all involved parties are in
-agreement before any party begins coding the proposed implementation. If you would like to see some examples
-of how these are written refer to the current [ADRs](https://github.com/cosmos/cosmos-sdk/tree/master/docs/architecture).
+When proposing an architecture decision for the SDK, please start by opening an [issue](https://github.com/cosmos/cosmos-sdk/issues/new/choose) or a [discussion](https://github.com/cosmos/cosmos-sdk/discussions/new) with a summary of the proposal. Once the proposal has been discussed and there is rough alignment on a high-level approach to the design, the [ADR creation process](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/PROCESS.md) can begin. We are following this process to ensure all involved parties are in agreement before any party begins coding the proposed implementation. If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/cosmos-sdk/tree/master/docs/architecture).
 
 ## Pull Requests
 

crypto/keys/internal/ecdsa/privkey.go

@@ -9,7 +9,50 @@ import (
 	"math/big"
 )
 
-// GenPrivKey generates a new secp256r1 private key. It uses operating system randomness.
+// p256Order returns the curve order for the secp256r1 curve
+// NOTE: this is specific to the secp256r1/P256 curve,
+// and not taken from the domain params for the key itself
+// (which would be a more generic approach for all EC).
+var p256Order = elliptic.P256().Params().N
+
+// p256HalfOrder returns half the curve order
+// a bit shift of 1 to the right (Rsh) is equivalent
+// to division by 2, only faster.
+var p256HalfOrder = new(big.Int).Rsh(p256Order, 1)
+
+// IsSNormalized returns true for the integer sigS if sigS falls in
+// lower half of the curve order
+func IsSNormalized(sigS *big.Int) bool {
+	return sigS.Cmp(p256HalfOrder) != 1
+}
+
+// NormalizeS will invert the s value if not already in the lower half
+// of curve order value
+func NormalizeS(sigS *big.Int) *big.Int {
+
+	if IsSNormalized(sigS) {
+		return sigS
+	}
+
+	return new(big.Int).Sub(p256Order, sigS)
+}
+
+// signatureRaw will serialize signature to R || S.
+// R, S are padded to 32 bytes respectively.
+// code roughly copied from secp256k1_nocgo.go
+func signatureRaw(r *big.Int, s *big.Int) []byte {
+
+	rBytes := r.Bytes()
+	sBytes := s.Bytes()
+	sigBytes := make([]byte, 64)
+	// 0 pad the byte arrays from the left if they aren't big enough.
+	copy(sigBytes[32-len(rBytes):32], rBytes)
+	copy(sigBytes[64-len(sBytes):64], sBytes)
+	return sigBytes
+}
+
+// GenPrivKey generates a new secp256r1 private key. It uses operating
+// system randomness.
 func GenPrivKey(curve elliptic.Curve) (PrivKey, error) {
 	key, err := ecdsa.GenerateKey(curve, rand.Reader)
 	if err != nil {
@@ -38,10 +81,25 @@ func (sk *PrivKey) Bytes() []byte {
 	return bz
 }
 
-// Sign hashes and signs the message usign ECDSA. Implements SDK PrivKey interface.
+// Sign hashes and signs the message using ECDSA. Implements SDK
+// PrivKey interface.
+// NOTE: this now calls the ecdsa Sign function
+// (not method!) directly as the s value of the signature is needed to
+// low-s normalize the signature value
+// See issue: https://github.com/cosmos/cosmos-sdk/issues/9723
+// It then raw encodes the signature as two fixed width 32-byte values
+// concatenated, reusing the code copied from secp256k1_nocgo.go
 func (sk *PrivKey) Sign(msg []byte) ([]byte, error) {
+
 	digest := sha256.Sum256(msg)
-	return sk.PrivateKey.Sign(rand.Reader, digest[:], nil)
+	r, s, err := ecdsa.Sign(rand.Reader, &sk.PrivateKey, digest[:])
+
+	if err != nil {
+		return nil, err
+	}
+
+	normS := NormalizeS(s)
+	return signatureRaw(r, normS), nil
 }
 
 // String returns a string representation of the public key based on the curveName.

crypto/keys/internal/ecdsa/privkey_internal_test.go

@@ -1,9 +1,12 @@
 package ecdsa
 
 import (
-	"testing"
-
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/sha256"
 	"github.com/tendermint/tendermint/crypto"
+	"math/big"
+	"testing"
 
 	"github.com/stretchr/testify/suite"
 )
@@ -60,6 +63,37 @@ func (suite *SKSuite) TestSign() {
 		require.False(suite.pk.VerifySignature(msg, sigCpy))
 	}
 
+	// mutate the signature by scalar neg'ing the s value
+	// to give a high-s signature, valid ECDSA but should
+	// be invalid with Cosmos signatures.
+	// code mostly copied from privkey/pubkey.go
+
+	// extract the r, s values from sig
+	r := new(big.Int).SetBytes(sig[:32])
+	low_s := new(big.Int).SetBytes(sig[32:64])
+
+	// test that NormalizeS simply returns an already
+	// normalized s
+	require.Equal(NormalizeS(low_s), low_s)
+
+	// flip the s value into high order of curve P256
+	// leave r untouched!
+	high_s := new(big.Int).Mod(new(big.Int).Neg(low_s), elliptic.P256().Params().N)
+
+	require.False(suite.pk.VerifySignature(msg, signatureRaw(r,high_s)))
+
+	// Valid signature using low_s, but too long
+	sigCpy = make([]byte, len(sig)+2)
+	copy(sigCpy, sig)
+	sigCpy[65] = byte('A')
+
+	require.False(suite.pk.VerifySignature(msg, sigCpy))
+
+	// check whether msg can be verified with same key, and high_s
+	// value using "regular" ecdsa signature
+	hash := sha256.Sum256([]byte(msg))
+	require.True(ecdsa.Verify(&suite.pk.PublicKey, hash[:], r, high_s))
+
 	// Mutate the message
 	msg[1] ^= byte(2)
 	require.False(suite.pk.VerifySignature(msg, sig))

crypto/keys/internal/ecdsa/pubkey.go

@@ -4,7 +4,6 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/sha256"
-	"encoding/asn1"
 	"fmt"
 	"math/big"
 
@@ -14,6 +13,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/errors"
 )
 
+// signatureFromBytes function roughly copied from secp256k1_nocgo.go
+// Read Signature struct from R || S. Caller needs to ensure that
+// len(sigStr) == 64.
+func signatureFromBytes(sigStr []byte) *signature {
+	return &signature{
+		R: new(big.Int).SetBytes(sigStr[:32]),
+		S: new(big.Int).SetBytes(sigStr[32:64]),
+	}
+}
+
 // signature holds the r and s values of an ECDSA signature.
 type signature struct {
 	R, S *big.Int
@@ -46,9 +55,23 @@ func (pk *PubKey) Bytes() []byte {
 }
 
 // VerifySignature checks if sig is a valid ECDSA signature for msg.
+// This includes checking for low-s normalized signatures
+// where the s integer component of the signature is in the
+// lower half of the curve order
+// 7/21/21 - expects raw encoded signature (fixed-width 64-bytes, R || S)
 func (pk *PubKey) VerifySignature(msg []byte, sig []byte) bool {
-	s := new(signature)
-	if _, err := asn1.Unmarshal(sig, s); err != nil || s == nil {
+
+	// check length for raw signature
+	// which is two 32-byte padded big.Ints
+	// concatenated
+	// NOT DER!
+
+	if len(sig) != 64 {
+		return false
+	}
+
+	s := signatureFromBytes(sig)
+	if !IsSNormalized(s.S) {
 		return false
 	}
 

docs/basics/tx-lifecycle.md

@@ -95,7 +95,7 @@ For example, the [`auth`](https://github.com/cosmos/cosmos-sdk/tree/master/x/aut
 
 ### Gas
 
-The [`Context`](../core/context.md), which keeps a `GasMeter` that will track how much gas has been used during the execution of `Tx`, is initialized. The user-provided amount of gas for `Tx` is known as `GasWanted`. If `GasConsumed`, the amount of gas consumed so during execution, ever exceeds `GasWanted`, the execution will stop and the changes made to the cached copy of the state won't be committed. Otherwise, `CheckTx` sets `GasUsed` equal to `GasConsumed` and returns it in the result. After calculating the gas and fee values, validator-nodes check that the user-specified `gas-prices` is less than their locally defined `min-gas-prices`.
+The [`Context`](../core/context.md), which keeps a `GasMeter` that will track how much gas has been used during the execution of `Tx`, is initialized. The user-provided amount of gas for `Tx` is known as `GasWanted`. If `GasConsumed`, the amount of gas consumed so during execution, ever exceeds `GasWanted`, the execution will stop and the changes made to the cached copy of the state won't be committed. Otherwise, `CheckTx` sets `GasUsed` equal to `GasConsumed` and returns it in the result. After calculating the gas and fee values, validator-nodes check that the user-specified `gas-prices` is greater than their locally defined `min-gas-prices`.
 
 ### Discard or Addition to Mempool
 

docs/core/baseapp.md

@@ -389,7 +389,7 @@ The [`Query` ABCI message](https://tendermint.com/docs/app-dev/abci-spec.html#qu
 Each Tendermint `query` comes with a `path`, which is a `string` which denotes what to query. If the `path` matches a gRPC fully-qualified service method, then `BaseApp` will defer the query to the `grpcQueryRouter` and let it handle it like explained [above](#grpc-query-router). Otherwise, the `path` represents a query that is not (yet) handled by the gRPC router. `BaseApp` splits the `path` string with the `/` delimiter. By convention, the first element of the splitted string (`splitted[0]`) contains the category of `query` (`app`, `p2p`, `store` or `custom` ). The `BaseApp` implementation of the `Query(req abci.RequestQuery)` method is a simple dispatcher serving these 4 main categories of queries:
 
 - Application-related queries like querying the application's version, which are served via the `handleQueryApp` method.
-- Direct queries to the multistore, which are served by the `handlerQueryStore` method. These direct queryeis are different from custom queries which go through `app.queryRouter`, and are mainly used by third-party service provider like block explorers.
+- Direct queries to the multistore, which are served by the `handlerQueryStore` method. These direct queries are different from custom queries which go through `app.queryRouter`, and are mainly used by third-party service provider like block explorers.
 - P2P queries, which are served via the `handleQueryP2P` method. These queries return either `app.addrPeerFilter` or `app.ipPeerFilter` that contain the list of peers filtered by address or IP respectively. These lists are first initialized via `options` in `BaseApp`'s [constructor](#constructor).
 - Custom queries, which encompass legacy queries (before the introduction of gRPC queries), are served via the `handleQueryCustom` method. The `handleQueryCustom` branches the multistore before using the `queryRoute` obtained from `app.queryRouter` to map the query to the appropriate module's [legacy `querier`](../building-modules/query-services.md#legacy-queriers).
 

proto/cosmos/group/v1beta1/query.proto

@@ -16,7 +16,7 @@ service Query {
 
   // GroupAccountInfo queries group account info based on group account address.
   rpc GroupAccountInfo(QueryGroupAccountInfoRequest) returns (QueryGroupAccountInfoResponse);
-  
+
   // GroupMembers queries members of a group
   rpc GroupMembers(QueryGroupMembersRequest) returns (QueryGroupMembersResponse);
 
@@ -25,16 +25,16 @@ service Query {
 
   // GroupAccountsByGroup queries group accounts by group id.
   rpc GroupAccountsByGroup(QueryGroupAccountsByGroupRequest) returns (QueryGroupAccountsByGroupResponse);
-  
+
   // GroupsByAdmin queries group accounts by admin address.
   rpc GroupAccountsByAdmin(QueryGroupAccountsByAdminRequest) returns (QueryGroupAccountsByAdminResponse);
-  
+
   // Proposal queries a proposal based on proposal id.
   rpc Proposal(QueryProposalRequest) returns (QueryProposalResponse);
 
   // ProposalsByGroupAccount queries proposals based on group account address.
   rpc ProposalsByGroupAccount(QueryProposalsByGroupAccountRequest) returns (QueryProposalsByGroupAccountResponse);
-  
+
   // VoteByProposalVoter queries a vote by proposal id and voter.
   rpc VoteByProposalVoter(QueryVoteByProposalVoterRequest) returns (QueryVoteByProposalVoterResponse);
 
@@ -47,16 +47,16 @@ service Query {
 
 // QueryGroupInfoRequest is the Query/GroupInfo request type.
 message QueryGroupInfoRequest {
-  
+
   // group_id is the unique ID of the group.
   uint64 group_id = 1;
 }
 
 // QueryGroupInfoResponse is the Query/GroupInfo response type.
 message QueryGroupInfoResponse {
 
-    // info is the GroupInfo for the group.
-    GroupInfo info = 1;
+  // info is the GroupInfo for the group.
+  GroupInfo info = 1;
 }
 
 // QueryGroupAccountInfoRequest is the Query/GroupAccountInfo request type.
@@ -69,8 +69,8 @@ message QueryGroupAccountInfoRequest {
 // QueryGroupAccountInfoResponse is the Query/GroupAccountInfo response type.
 message QueryGroupAccountInfoResponse {
 
-    // info is the GroupAccountInfo for the group account.
-    GroupAccountInfo info = 1;
+  // info is the GroupAccountInfo for the group account.
+  GroupAccountInfo info = 1;
 }
 
 // QueryGroupMembersRequest is the Query/GroupMembersRequest request type.
@@ -115,7 +115,7 @@ message QueryGroupsByAdminResponse {
 
 // QueryGroupAccountsByGroupRequest is the Query/GroupAccountsByGroup request type.
 message QueryGroupAccountsByGroupRequest {
-  
+
   // group_id is the unique ID of the group account's group.
   uint64 group_id = 1;
 
@@ -135,7 +135,7 @@ message QueryGroupAccountsByGroupResponse {
 
 // QueryGroupAccountsByAdminRequest is the Query/GroupAccountsByAdmin request type.
 message QueryGroupAccountsByAdminRequest {
-  
+
   // admin is the admin address of the group account.
   string admin = 1;
 
@@ -192,7 +192,7 @@ message QueryVoteByProposalVoterRequest {
 
   // proposal_id is the unique ID of a proposal.
   uint64 proposal_id = 1;
-  
+
   // voter is a proposal voter account address.
   string voter = 2;
 }
@@ -209,7 +209,7 @@ message QueryVotesByProposalRequest {
 
   // proposal_id is the unique ID of a proposal.
   uint64 proposal_id = 1;
-  
+
   // pagination defines an optional pagination for the request.
   cosmos.base.query.v1beta1.PageRequest pagination = 2;
 }