Skip to content

Commit

Permalink
fix: cgosecp256k1 verification (cosmos#11298)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
tac0turtle authored Mar 10, 2022
1 parent efdedf3 commit 361c837
Show file tree
Hide file tree
Showing 23 changed files with 461 additions and 90 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/authz) [\#11252](https://github.com/cosmos/cosmos-sdk/pull/11252) Allow insufficient funds error for authz simulation
* (cli) [\#11313](https://github.com/cosmos/cosmos-sdk/pull/11313) Fixes `--gas auto` when executing CLI transactions in `--generate-only` mode
* (cli) [\#11337](https://github.com/cosmos/cosmos-sdk/pull/11337) Fixes `show-adress` cli cmd
* (crypto) [\#11298](https://github.com/cosmos/cosmos-sdk/pull/11298) Fix cgo secp signature verification and update libscep256k1 library.

### State Machine Breaking

Expand Down
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ 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 := ,
Expand Down
4 changes: 2 additions & 2 deletions crypto/keys/secp256k1/internal/secp256k1/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# secp256k1

This package is copied from https://github.com/ethereum/go-ethereum/tree/729bf365b5f17325be9107b63b233da54100eec6/crypto/secp256k1
This package is copied from https://github.com/ethereum/go-ethereum/tree/8fddf27a989e246659fd018ea9be37b2b4f55326/crypto/secp256k1

Unlike the rest of go-ethereum it is MIT 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.
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.
60 changes: 15 additions & 45 deletions crypto/keys/secp256k1/internal/secp256k1/curve.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,13 @@
// (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:gocritic
package secp256k1

import (
"crypto/elliptic"
"math/big"
"unsafe"
)

/*
#include "libsecp256k1/include/secp256k1.h"
extern int secp256k1_ext_scalar_mul(const secp256k1_context* ctx,
const unsigned char *point,
const unsigned char *scalar);
*/
import "C"

const (
// number of bits in a big.Word
wordBits = 32 << (uint64(^big.Word(0)) >> 63)
Expand Down Expand Up @@ -119,6 +109,10 @@ func (BitCurve *BitCurve) IsOnCurve(x, y *big.Int) bool {
// 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) {
if z.Sign() == 0 {
return new(big.Int), new(big.Int)
}

zinv := new(big.Int).ModInverse(z, BitCurve.P)
zinvsq := new(big.Int).Mul(zinv, zinv)

Expand All @@ -132,7 +126,18 @@ 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) {
// 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 {
return x2, y2
}
if x2.Sign() == 0 && y2.Sign() == 0 {
return x1, y1
}
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.addJacobian(x1, y1, z, x2, y2, z))
}

Expand Down Expand Up @@ -241,41 +246,6 @@ func (BitCurve *BitCurve) doubleJacobian(x, y, z *big.Int) (*big.Int, *big.Int,
return x3, y3, z3
}

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.
if len(scalar) > 32 {
panic("can't handle scalars > 256 bits")
}
// NOTE: potential timing issue
padded := make([]byte, 32)
copy(padded[32-len(scalar):], scalar)
scalar = padded

// Do the multiplication in C, updating point.
point := make([]byte, 64)
readBits(Bx, point[:32])
readBits(By, point[32:])

pointPtr := (*C.uchar)(unsafe.Pointer(&point[0]))
scalarPtr := (*C.uchar)(unsafe.Pointer(&scalar[0]))
res := C.secp256k1_ext_scalar_mul(context, pointPtr, scalarPtr)

// Unpack the result and clear temporaries.
x := new(big.Int).SetBytes(point[:32])
y := new(big.Int).SetBytes(point[32:])
for i := range point {
point[i] = 0
}
for i := range padded {
scalar[i] = 0
}
if res != 1 {
return nil, nil
}
return x, y
}

// ScalarBaseMult returns k*G, where G is the base point of the group and k is
// an integer in big-endian form.
func (BitCurve *BitCurve) ScalarBaseMult(k []byte) (*big.Int, *big.Int) {
Expand Down
21 changes: 21 additions & 0 deletions crypto/keys/secp256k1/internal/secp256k1/dummy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//go:build dummy
// +build dummy

// This file is part of a workaround for `go mod vendor` which won't vendor
// C files if there's no Go file in the same directory.
// This would prevent the crypto/secp256k1/libsecp256k1/include/secp256k1.h file to be vendored.
//
// This Go file imports the c directory where there is another dummy.go file which
// is the second part of this workaround.
//
// These two files combined make it so `go mod vendor` behaves correctly.
//
// See this issue for reference: https://github.com/golang/go/issues/26366

package secp256k1

import (
_ "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"
)
59 changes: 29 additions & 30 deletions crypto/keys/secp256k1/internal/secp256k1/libsecp256k1/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package contrib
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package libsecp256k1
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package include
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Note:
- To avoid unnecessary loads and make use of available registers, two
'passes' have every time been interleaved, with the odd passes accumulating c' and d'
which will be added to c and d respectively in the the even passes
which will be added to c and d respectively in the even passes
*/

Expand All @@ -23,7 +23,7 @@ Note:
.eabi_attribute 10, 0 @ Tag_FP_arch = none
.eabi_attribute 24, 1 @ Tag_ABI_align_needed = 8-byte
.eabi_attribute 25, 1 @ Tag_ABI_align_preserved = 8-byte, except leaf SP
.eabi_attribute 30, 2 @ Tag_ABI_optimization_goals = Agressive Speed
.eabi_attribute 30, 2 @ Tag_ABI_optimization_goals = Aggressive Speed
.eabi_attribute 34, 1 @ Tag_CPU_unaligned_access = v6
.text

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package src
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package module
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package ecdh
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build dummy

// Package c contains only a C file.
//
// This Go file is part of a workaround for `go mod vendor`.
// Please see the file crypto/secp256k1/dummy.go for more information.
package recovery
3 changes: 3 additions & 0 deletions crypto/keys/secp256k1/internal/secp256k1/panic_cb.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// Use of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.

//go:build !gofuzz && cgo
// +build !gofuzz,cgo

package secp256k1

import "C"
Expand Down
57 changes: 57 additions & 0 deletions crypto/keys/secp256k1/internal/secp256k1/scalar_mult_cgo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright 2015 Jeffrey Wilcke, Felix Lange, Gustav Simonsson. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.

//go:build !gofuzz && cgo
// +build !gofuzz,cgo

package secp256k1

import (
"math/big"
"unsafe"
)

/*
#include "libsecp256k1/include/secp256k1.h"
extern int secp256k1_ext_scalar_mul(const secp256k1_context* ctx, const unsigned char *point, const unsigned char *scalar);
*/
import "C"

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.
if len(scalar) > 32 {
panic("can't handle scalars > 256 bits")
}
// NOTE: potential timing issue
padded := make([]byte, 32)
copy(padded[32-len(scalar):], scalar)
scalar = padded

// Do the multiplication in C, updating point.
point := make([]byte, 64)
readBits(Bx, point[:32])
readBits(By, point[32:])

pointPtr := (*C.uchar)(unsafe.Pointer(&point[0]))
scalarPtr := (*C.uchar)(unsafe.Pointer(&scalar[0]))
res := C.secp256k1_ext_scalar_mul(context, pointPtr, scalarPtr)

// Unpack the result and clear temporaries.
x := new(big.Int).SetBytes(point[:32])
y := new(big.Int).SetBytes(point[32:])
for i := range point {
point[i] = 0
}
for i := range padded {
scalar[i] = 0
}
if res != 1 {
return nil, nil
}
return x, y
}
14 changes: 14 additions & 0 deletions crypto/keys/secp256k1/internal/secp256k1/scalar_mult_nocgo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2015 Jeffrey Wilcke, Felix Lange, Gustav Simonsson. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be found in
// the LICENSE file.

//go:build gofuzz || !cgo
// +build gofuzz !cgo

package secp256k1

import "math/big"

func (BitCurve *BitCurve) ScalarMult(Bx, By *big.Int, scalar []byte) (*big.Int, *big.Int) {
panic("ScalarMult is not available when secp256k1 is built without cgo")
}
Loading

0 comments on commit 361c837

Please sign in to comment.