fix: cgosecp256k1 verification (backport cosmos#11298) (cosmos#11360)

* fix: cgosecp256k1 verification (cosmos#11298) ## Description Closes: cosmos#10747 - update secp256k1 cgo fork, - debug verify bytes ``` benchmark old ns/op new ns/op delta BenchmarkKeyGeneration-10 407 413 +1.35% BenchmarkSigning-10 95099 36754 -61.35% BenchmarkVerification-10 215551 48053 -77.71% benchmark old allocs new allocs delta BenchmarkKeyGeneration-10 2 2 +0.00% BenchmarkSigning-10 83 4 -95.18% BenchmarkVerification-10 74 1 -98.65% benchmark old bytes new bytes delta BenchmarkKeyGeneration-10 96 96 +0.00% BenchmarkSigning-10 5283 196 -96.29% BenchmarkVerification-10 3537 32 -99.10% ``` --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit 361c837) # Conflicts: # CHANGELOG.md # crypto/keys/secp256k1/internal/secp256k1/README.md * fix conflicts Co-authored-by: Marko <marbar3778@yahoo.com>
agoric-labs · Sep 28, 2024 · 4ed9f8d · 4ed9f8d
1 parent 430369e
commit 4ed9f8d
Show file tree

Hide file tree

Showing 18 changed files with 213 additions and 88 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -52,6 +52,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (store) [\#11177](https://github.com/cosmos/cosmos-sdk/pull/11177) Update the prune `everything` strategy to store the last two heights.
 * (store) [\#11117](https://github.com/cosmos/cosmos-sdk/pull/11117) Fix data race in store trace component
 * (x/authz) [\#11252](https://github.com/cosmos/cosmos-sdk/pull/11252) Allow insufficient funds error for authz simulation
+* (crypto) [\#11298](https://github.com/cosmos/cosmos-sdk/pull/11298) Fix cgo secp signature verification and update libscep256k1 library. 
 
 ### Improvements
 

diff --git a/Makefile b/Makefile
@@ -15,7 +15,167 @@ PR_TARGET_BRANCH = master
 DOCKER := $(shell which docker)
 DOCKER_BUF := $(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace bufbuild/buf
 
-.DEFAULT_GOAL := help
+export GO111MODULE = on
+
+# process build tags
+
+build_tags = netgo
+ifeq ($(LEDGER_ENABLED),true)
+  ifeq ($(OS),Windows_NT)
+    GCCEXE = $(shell where gcc.exe 2> NUL)
+    ifeq ($(GCCEXE),)
+      $(error gcc.exe not installed for ledger support, please install or set LEDGER_ENABLED=false)
+    else
+      build_tags += ledger
+    endif
+  else
+    UNAME_S = $(shell uname -s)
+    ifeq ($(UNAME_S),OpenBSD)
+      $(warning OpenBSD detected, disabling ledger support (https://github.com/cosmos/cosmos-sdk/issues/1988))
+    else
+      GCC = $(shell command -v gcc 2> /dev/null)
+      ifeq ($(GCC),)
+        $(error gcc not installed for ledger support, please install or set LEDGER_ENABLED=false)
+      else
+        build_tags += ledger
+      endif
+    endif
+  endif
+endif
+
+ifeq (cleveldb,$(findstring cleveldb,$(COSMOS_BUILD_OPTIONS)))
+  build_tags += gcc
+endif
+
+ifeq (secp,$(findstring secp,$(COSMOS_BUILD_OPTIONS)))
+  build_tags += libsecp256k1_sdk
+endif
+
+whitespace :=
+whitespace += $(whitespace)
+comma := ,
+build_tags_comma_sep := $(subst $(whitespace),$(comma),$(build_tags))
+
+# process linker flags
+
+ldflags = -X github.com/cosmos/cosmos-sdk/version.Name=sim \
+		  -X github.com/cosmos/cosmos-sdk/version.AppName=simd \
+		  -X github.com/cosmos/cosmos-sdk/version.Version=$(VERSION) \
+		  -X github.com/cosmos/cosmos-sdk/version.Commit=$(COMMIT) \
+		  -X "github.com/cosmos/cosmos-sdk/version.BuildTags=$(build_tags_comma_sep)" \
+			-X github.com/tendermint/tendermint/version.TMCoreSemVer=$(TMVERSION)
+
+# DB backend selection
+ifeq (cleveldb,$(findstring cleveldb,$(COSMOS_BUILD_OPTIONS)))
+  ldflags += -X github.com/cosmos/cosmos-sdk/types.DBBackend=cleveldb
+endif
+ifeq (badgerdb,$(findstring badgerdb,$(COSMOS_BUILD_OPTIONS)))
+  ldflags += -X github.com/cosmos/cosmos-sdk/types.DBBackend=badgerdb
+  BUILD_TAGS += badgerdb
+endif
+# handle rocksdb
+ifeq (rocksdb,$(findstring rocksdb,$(COSMOS_BUILD_OPTIONS)))
+  CGO_ENABLED=1
+  BUILD_TAGS += rocksdb
+  ldflags += -X github.com/cosmos/cosmos-sdk/types.DBBackend=rocksdb
+endif
+# handle boltdb
+ifeq (boltdb,$(findstring boltdb,$(COSMOS_BUILD_OPTIONS)))
+  BUILD_TAGS += boltdb
+  ldflags += -X github.com/cosmos/cosmos-sdk/types.DBBackend=boltdb
+endif
+
+ifeq (,$(findstring nostrip,$(COSMOS_BUILD_OPTIONS)))
+  ldflags += -w -s
+endif
+ldflags += $(LDFLAGS)
+ldflags := $(strip $(ldflags))
+
+build_tags += $(BUILD_TAGS)
+build_tags := $(strip $(build_tags))
+
+BUILD_FLAGS := -tags "$(build_tags)" -ldflags '$(ldflags)'
+# check for nostrip option
+ifeq (,$(findstring nostrip,$(COSMOS_BUILD_OPTIONS)))
+  BUILD_FLAGS += -trimpath
+endif
+
+all: tools build lint test
+
+# The below include contains the tools and runsim targets.
+include contrib/devtools/Makefile
+
+###############################################################################
+###                                  Build                                  ###
+###############################################################################
+
+BUILD_TARGETS := build install
+
+build: BUILD_ARGS=-o $(BUILDDIR)/
+build-linux:
+	GOOS=linux GOARCH=amd64 LEDGER_ENABLED=false $(MAKE) build
+
+$(BUILD_TARGETS): go.sum $(BUILDDIR)/
+	go $@ -mod=readonly $(BUILD_FLAGS) $(BUILD_ARGS) ./...
+
+$(BUILDDIR)/:
+	mkdir -p $(BUILDDIR)/
+
+build-simd-all: go.sum
+	$(DOCKER) rm latest-build || true
+	$(DOCKER) run --volume=$(CURDIR):/sources:ro \
+        --env TARGET_PLATFORMS='linux/amd64 darwin/amd64 linux/arm64 windows/amd64' \
+        --env APP=simd \
+        --env VERSION=$(VERSION) \
+        --env COMMIT=$(COMMIT) \
+        --env LEDGER_ENABLED=$(LEDGER_ENABLED) \
+        --name latest-build cosmossdk/rbuilder:latest
+	$(DOCKER) cp -a latest-build:/home/builder/artifacts/ $(CURDIR)/
+
+build-simd-linux: go.sum $(BUILDDIR)/
+	$(DOCKER) rm latest-build || true
+	$(DOCKER) run --volume=$(CURDIR):/sources:ro \
+        --env TARGET_PLATFORMS='linux/amd64' \
+        --env APP=simd \
+        --env VERSION=$(VERSION) \
+        --env COMMIT=$(COMMIT) \
+        --env LEDGER_ENABLED=false \
+        --name latest-build cosmossdk/rbuilder:latest
+	$(DOCKER) cp -a latest-build:/home/builder/artifacts/ $(CURDIR)/
+	cp artifacts/simd-*-linux-amd64 $(BUILDDIR)/simd
+
+cosmovisor:
+	$(MAKE) -C cosmovisor cosmovisor
+
+.PHONY: build build-linux build-simd-all build-simd-linux cosmovisor
+
+mockgen_cmd=go run github.com/golang/mock/mockgen
+
+mocks: $(MOCKS_DIR)
+	$(mockgen_cmd) -source=client/account_retriever.go -package mocks -destination tests/mocks/account_retriever.go
+	$(mockgen_cmd) -package mocks -destination tests/mocks/tendermint_tm_db_DB.go github.com/tendermint/tm-db DB
+	$(mockgen_cmd) -source=types/module/module.go -package mocks -destination tests/mocks/types_module_module.go
+	$(mockgen_cmd) -source=types/invariant.go -package mocks -destination tests/mocks/types_invariant.go
+	$(mockgen_cmd) -source=types/router.go -package mocks -destination tests/mocks/types_router.go
+	$(mockgen_cmd) -package mocks -destination tests/mocks/grpc_server.go github.com/gogo/protobuf/grpc Server
+	$(mockgen_cmd) -package mocks -destination tests/mocks/tendermint_tendermint_libs_log_DB.go github.com/tendermint/tendermint/libs/log Logger
+.PHONY: mocks
+
+$(MOCKS_DIR):
+	mkdir -p $(MOCKS_DIR)
+
+distclean: clean tools-clean
+clean:
+	rm -rf \
+    $(BUILDDIR)/ \
+    artifacts/ \
+    tmp-swagger-gen/
+
+.PHONY: distclean clean
+
+###############################################################################
+###                          Tools & Dependencies                           ###
+###############################################################################
 
 #? go.sum: Run go mod tidy and ensure dependencies have not been modified.
 go.sum: go.mod

diff --git a/crypto/keys/secp256k1/internal/secp256k1/README.md b/crypto/keys/secp256k1/internal/secp256k1/README.md
@@ -3,17 +3,3 @@
  This package is copied from https://github.com/ethereum/go-ethereum/tree/8fddf27a989e246659fd018ea9be37b2b4f55326/crypto/secp256k1
 
  Unlike the rest of go-ethereum it is [3-clause BSD](https://opensource.org/licenses/BSD-3-Clause) licensed so compatible with our Apache2.0 license. We opt to copy in here rather than depend on go-ethereum to avoid issues with vendoring of the GPL parts of that repository by downstream.
-
-## Duplicate Symbols
-
-If a project is importing [go-ethereum](https://github.com/ethereum/go-ethereum) and the Cosmos SDK, cgo secp256k1 will only work on linux operating systems due to duplicated symbols. If you are testing on a mac, we recommend using a docker container or something similar. 
-
-To avoid duplicate symbol errors `ldflags` must be set to allow for multiple definitions. 
-
-#### Gcc
-
- + `go build -tags libsecp256k1_sdk  -ldflags=all="-extldflags=-Wl,--allow-multiple-definition"`
-
-#### Clang
-
- + `go build -tags libsecp256k1_sdk -ldflags=all="-extldflags=-zmuldefs"`
diff --git a/crypto/keys/secp256k1/internal/secp256k1/curve.go b/crypto/keys/secp256k1/internal/secp256k1/curve.go
@@ -29,7 +29,7 @@
 // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-// nolint // this nolint lets us use this file in its original and unmodified form.
+
 package secp256k1
 
 import (
@@ -108,12 +108,12 @@ func (bitCurve *BitCurve) IsOnCurve(x, y *big.Int) bool {
 // TODO: double check if the function is okay
 // affineFromJacobian reverses the Jacobian transform. See the comment at the
 // top of the file.
-func (bitCurve *BitCurve) affineFromJacobian(x, y, z *big.Int) (xOut, yOut *big.Int) {
+func (BitCurve *BitCurve) affineFromJacobian(x, y, z *big.Int) (xOut, yOut *big.Int) {
 	if z.Sign() == 0 {
 		return new(big.Int), new(big.Int)
 	}
 
-	zinv := new(big.Int).ModInverse(z, bitCurve.P)
+	zinv := new(big.Int).ModInverse(z, BitCurve.P)
 	zinvsq := new(big.Int).Mul(zinv, zinv)
 
 	xOut = new(big.Int).Mul(x, zinvsq)
@@ -125,7 +125,7 @@ func (bitCurve *BitCurve) affineFromJacobian(x, y, z *big.Int) (xOut, yOut *big.
 }
 
 // Add returns the sum of (x1,y1) and (x2,y2)
-func (bitCurve *BitCurve) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int) {
+func (BitCurve *BitCurve) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int) {
 	// If one point is at infinity, return the other point.
 	// Adding the point at infinity to any point will preserve the other point.
 	if x1.Sign() == 0 && y1.Sign() == 0 {
@@ -136,9 +136,9 @@ func (bitCurve *BitCurve) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int) {
 	}
 	z := new(big.Int).SetInt64(1)
 	if x1.Cmp(x2) == 0 && y1.Cmp(y2) == 0 {
-		return bitCurve.affineFromJacobian(bitCurve.doubleJacobian(x1, y1, z))
+		return BitCurve.affineFromJacobian(BitCurve.doubleJacobian(x1, y1, z))
 	}
-	return bitCurve.affineFromJacobian(bitCurve.addJacobian(x1, y1, z, x2, y2, z))
+	return BitCurve.affineFromJacobian(BitCurve.addJacobian(x1, y1, z, x2, y2, z))
 }
 
 // addJacobian takes two points in Jacobian coordinates, (x1, y1, z1) and

diff --git a/crypto/keys/secp256k1/internal/secp256k1/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/dummy.go
@@ -15,7 +15,7 @@
 package secp256k1
 
 import (
-	_ "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/include"
-	_ "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src"
-	_ "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/recovery"
+	_ "github.com/ethereum/go-ethereum/crypto/secp256k1/libsecp256k1/include"
+	_ "github.com/ethereum/go-ethereum/crypto/secp256k1/libsecp256k1/src"
+	_ "github.com/ethereum/go-ethereum/crypto/secp256k1/libsecp256k1/src/modules/recovery"
 )
diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/README.md b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/README.md
@@ -8,7 +8,6 @@ Optimized C library for EC operations on curve secp256k1.
 This library is a work in progress and is being used to research best practices. Use at your own risk.
 
 Features:
-
 * secp256k1 ECDSA signing/verification and key generation.
 * Adding/multiplying private/public keys.
 * Serialization/parsing of private keys, public keys, signatures.
@@ -20,43 +19,43 @@ Implementation details
 ----------------------
 
 * General
-    * No runtime heap allocation.
-    * Extensive testing infrastructure.
-    * Structured to facilitate review and analysis.
-    * Intended to be portable to any system with a C89 compiler and uint64_t support.
-    * Expose only higher level interfaces to minimize the API surface and improve application security. ("Be difficult to use insecurely.")
+  * No runtime heap allocation.
+  * Extensive testing infrastructure.
+  * Structured to facilitate review and analysis.
+  * Intended to be portable to any system with a C89 compiler and uint64_t support.
+  * Expose only higher level interfaces to minimize the API surface and improve application security. ("Be difficult to use insecurely.")
 * Field operations
-    * Optimized implementation of arithmetic modulo the curve's field size (2^256 - 0x1000003D1).
-        * Using 5 52-bit limbs (including hand-optimized assembly for x86_64, by Diederik Huys).
-        * Using 10 26-bit limbs.
-    * Field inverses and square roots using a sliding window over blocks of 1s (by Peter Dettman).
+  * Optimized implementation of arithmetic modulo the curve's field size (2^256 - 0x1000003D1).
+    * Using 5 52-bit limbs (including hand-optimized assembly for x86_64, by Diederik Huys).
+    * Using 10 26-bit limbs.
+  * Field inverses and square roots using a sliding window over blocks of 1s (by Peter Dettman).
 * Scalar operations
-    * Optimized implementation without data-dependent branches of arithmetic modulo the curve's order.
-        * Using 4 64-bit limbs (relying on __int128 support in the compiler).
-        * Using 8 32-bit limbs.
+  * Optimized implementation without data-dependent branches of arithmetic modulo the curve's order.
+    * Using 4 64-bit limbs (relying on __int128 support in the compiler).
+    * Using 8 32-bit limbs.
 * Group operations
-    * Point addition formula specifically simplified for the curve equation (y^2 = x^3 + 7).
-    * Use addition between points in Jacobian and affine coordinates where possible.
-    * Use a unified addition/doubling formula where necessary to avoid data-dependent branches.
-    * Point/x comparison without a field inversion by comparison in the Jacobian coordinate space.
+  * Point addition formula specifically simplified for the curve equation (y^2 = x^3 + 7).
+  * Use addition between points in Jacobian and affine coordinates where possible.
+  * Use a unified addition/doubling formula where necessary to avoid data-dependent branches.
+  * Point/x comparison without a field inversion by comparison in the Jacobian coordinate space.
 * Point multiplication for verification (a*P + b*G).
-    * Use wNAF notation for point multiplicands.
-    * Use a much larger window for multiples of G, using precomputed multiples.
-    * Use Shamir's trick to do the multiplication with the public key and the generator simultaneously.
-    * Optionally (off by default) use secp256k1's efficiently-computable endomorphism to split the P multiplicand into 2 half-sized ones.
+  * Use wNAF notation for point multiplicands.
+  * Use a much larger window for multiples of G, using precomputed multiples.
+  * Use Shamir's trick to do the multiplication with the public key and the generator simultaneously.
+  * Optionally (off by default) use secp256k1's efficiently-computable endomorphism to split the P multiplicand into 2 half-sized ones.
 * Point multiplication for signing
-    * Use a precomputed table of multiples of powers of 16 multiplied with the generator, so general multiplication becomes a series of additions.
-    * Access the table with branch-free conditional moves so memory access is uniform.
-    * No data-dependent branches
-    * The precomputed tables add and eventually subtract points for which no known scalar (private key) is known, preventing even an attacker with control over the private key used to control the data internally.
+  * Use a precomputed table of multiples of powers of 16 multiplied with the generator, so general multiplication becomes a series of additions.
+  * Access the table with branch-free conditional moves so memory access is uniform.
+  * No data-dependent branches
+  * The precomputed tables add and eventually subtract points for which no known scalar (private key) is known, preventing even an attacker with control over the private key used to control the data internally.
 
 Build steps
 -----------
 
 libsecp256k1 is built using autotools:
 
-    ./autogen.sh
-    ./configure
-    make
-    ./tests
-    sudo make install  # optional
+    $ ./autogen.sh
+    $ ./configure
+    $ make
+    $ ./tests
+    $ sudo make install  # optional
diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/contrib/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/contrib/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/include/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/include/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/ecdh/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/ecdh/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/recovery/dummy.go b/crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/src/modules/recovery/dummy.go
@@ -1,4 +1,3 @@
-//go:build dummy
 // +build dummy
 
 // Package c contains only a C file.

diff --git a/crypto/keys/secp256k1/internal/secp256k1/scalar_mult_cgo.go b/crypto/keys/secp256k1/internal/secp256k1/scalar_mult_cgo.go
@@ -5,9 +5,13 @@
 //go:build !gofuzz && cgo
 // +build !gofuzz,cgo
 
-// nolint // this nolint lets us use this file in its original and unmodified form.
 package secp256k1
 
+import (
+	"math/big"
+	"unsafe"
+)
+
 /*
 
 #include "libsecp256k1/include/secp256k1.h"
@@ -17,11 +21,6 @@ extern int secp256k1_ext_scalar_mul(const secp256k1_context* ctx, const unsigned
 */
 import "C"
 
-import (
-	"math/big"
-	"unsafe"
-)
-
 func (BitCurve *BitCurve) ScalarMult(Bx, By *big.Int, scalar []byte) (*big.Int, *big.Int) {
 	// Ensure scalar is exactly 32 bytes. We pad always, even if
 	// scalar is 32 bytes long, to avoid a timing side channel.