From ae2b6ff21510610de5a26789aa4501d13485c3a5 Mon Sep 17 00:00:00 2001 From: Gaurav Narula Date: Wed, 12 Aug 2020 14:05:46 +0200 Subject: [PATCH 1/3] Schnorr Verification Checks Added `schnorr.VerifyWithChecks` which checks if the scalars and points are canonical and ensures the points do not have a small order for Edwards25519 Group. Builds on top of #432 and closes #431 Co-authored-by: David Cerezo --- sign/schnorr/schnorr.go | 32 +++++++++++++++++++++++++++++--- sign/schnorr/schnorr_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/sign/schnorr/schnorr.go b/sign/schnorr/schnorr.go index def76095b..9d0dae6b8 100644 --- a/sign/schnorr/schnorr.go +++ b/sign/schnorr/schnorr.go @@ -18,6 +18,8 @@ import ( "fmt" "go.dedis.ch/kyber/v3" + "go.dedis.ch/kyber/v3/group/edwards25519" + "go.dedis.ch/kyber/v3/sign/eddsa" ) // Suite represents the set of functionalities needed by the package schnorr. @@ -57,9 +59,16 @@ func Sign(s Suite, private kyber.Scalar, msg []byte) ([]byte, error) { return b.Bytes(), nil } -// Verify verifies a given Schnorr signature. It returns nil iff the -// given signature is valid. -func Verify(g kyber.Group, public kyber.Point, msg, sig []byte) error { +// VerifyWithChecks uses a public key buffer, a message and a signature. +// It will return nil if sig is a valid signature for msg created by +// key public, or an error otherwise. Compared to `Verify`, it performs +// additional checks around the canonicality and ensures the public key +// does not have a small order when using `edwards25519` group. +func VerifyWithChecks(g kyber.Group, pub, msg, sig []byte) error { + if _, ok := g.(*edwards25519.SuiteEd25519); ok { + return eddsa.VerifyWithChecks(pub, msg, sig) + } + R := g.Point() s := g.Scalar() pointSize := R.MarshalSize() @@ -74,6 +83,12 @@ func Verify(g kyber.Group, public kyber.Point, msg, sig []byte) error { if err := s.UnmarshalBinary(sig[pointSize:]); err != nil { return err } + + public := g.Point() + err := public.UnmarshalBinary(pub) + if err != nil { + return fmt.Errorf("schnorr: error unmarshalling public key") + } // recompute hash(public || R || msg) h, err := hash(g, public, R, msg) if err != nil { @@ -91,6 +106,17 @@ func Verify(g kyber.Group, public kyber.Point, msg, sig []byte) error { } return nil + +} + +// Verify verifies a given Schnorr signature. It returns nil iff the +// given signature is valid. +func Verify(g kyber.Group, public kyber.Point, msg, sig []byte) error { + PBuf, err := public.MarshalBinary() + if err != nil { + return fmt.Errorf("error unmarshalling public key: %s", err) + } + return VerifyWithChecks(g, PBuf, msg, sig) } func hash(g kyber.Group, public, r kyber.Point, msg []byte) (kyber.Scalar, error) { diff --git a/sign/schnorr/schnorr_test.go b/sign/schnorr/schnorr_test.go index 43d9855ba..e669c2ce1 100644 --- a/sign/schnorr/schnorr_test.go +++ b/sign/schnorr/schnorr_test.go @@ -96,3 +96,32 @@ func TestQuickSchnorrSignature(t *testing.T) { t.Error(err) } } + +func TestSchnorrMalleability(t *testing.T) { + /* l = 2^252+27742317777372353535851937790883648493, prime order of the base point */ + var L []uint16 = []uint16{0xed, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, + 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10} + var c uint16 = 0 + + msg := []byte("Hello Schnorr") + suite := edwards25519.NewBlakeSHA256Ed25519() + kp := key.NewKeyPair(suite) + + s, err := Sign(suite, kp.Private, msg) + assert.NoErrorf(t, err, "Couldn't sign msg: %s: %v", msg, err) + + err = Verify(suite, kp.Public, msg, s) + assert.NoErrorf(t, err, "Couldn't verify signature (schnorr.Verify): \n%+v\nfor msg:'%s'. Error:\n%v", s, msg, err) + + // Add l to signature + for i := 0; i < 32; i++ { + c += uint16(s[32+i]) + L[i] + s[32+i] = byte(c) + c >>= 8 + } + assert.Error(t, eddsa.Verify(kp.Public, msg, s)) + + err = Verify(suite, kp.Public, msg, s) + assert.Error(t, err, "schnorr signature malleable") +} From 5556ed0b0d5efa4ca378f79409b388e9216e0b52 Mon Sep 17 00:00:00 2001 From: Gaurav Narula Date: Wed, 12 Aug 2020 16:10:47 +0200 Subject: [PATCH 2/3] Nicolas' comments --- group/edwards25519/point.go | 27 +++++++++ group/edwards25519/point_test.go | 39 +++++++++++++ group/edwards25519/scalar.go | 34 +++++++++++ group/edwards25519/scalar_test.go | 22 +++++++ sign/eddsa/eddsa.go | 95 ++++++------------------------- sign/eddsa/eddsa_test.go | 59 ------------------- sign/schnorr/schnorr.go | 30 ++++++++-- 7 files changed, 165 insertions(+), 141 deletions(-) diff --git a/group/edwards25519/point.go b/group/edwards25519/point.go index 27009191d..e04f0d3a1 100644 --- a/group/edwards25519/point.go +++ b/group/edwards25519/point.go @@ -263,3 +263,30 @@ func (P *point) HasSmallOrder() bool { return (k>>8)&1 > 0 } + +// IsCanonical determines whether the group element is canonical +// +// Checks whether group element s is less than p, according to RFC8032§5.1.3.1 +// https://tools.ietf.org/html/rfc8032#section-5.1.3 +// +// Taken from +// https://github.com/jedisct1/libsodium/blob/4744636721d2e420f8bbe2d563f31b1f5e682229/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c#L1113 +// +// The meethod accepts a buffer instead of calling `MarshalBianry` on the receiver +// because that always returns a value modulo `prime`. +func (P *point) IsCanonical(s []byte) bool { + if len(s) != 32 { + return false + } + + c := (s[31] & 0x7f) ^ 0x7f + for i := 30; i > 0; i-- { + c |= s[i] ^ 0xff + } + + // subtraction might underflow + c = byte((uint16(c) - 1) >> 8) + d := byte((0xed - 1 - uint16(s[0])) >> 8) + + return 1-(c&d&1) == 1 +} diff --git a/group/edwards25519/point_test.go b/group/edwards25519/point_test.go index 9d2a26803..4a5b958d5 100644 --- a/group/edwards25519/point_test.go +++ b/group/edwards25519/point_test.go @@ -23,3 +23,42 @@ func TestPoint_HasSmallOrder(t *testing.T) { require.True(t, p.HasSmallOrder(), fmt.Sprintf("%s should be considered to have a small order", hex.EncodeToString(key))) } } + +// Test_PointIsCanonical ensures that elements >= p are considered +// non canonical +func Test_PointIsCanonical(t *testing.T) { + + // buffer stores the candidate points (in little endian) that we'll test + // against, starting with `prime` + buffer := prime.Bytes() + for i, j := 0, len(buffer)-1; i < j; i, j = i+1, j-1 { + buffer[i], buffer[j] = buffer[j], buffer[i] + } + + // Iterate over the 19*2 finite field elements + point := point{} + actualNonCanonicalCount := 0 + expectedNonCanonicalCount := 24 + for i := 0; i < 19; i++ { + buffer[0] = byte(237 + i) + buffer[31] = byte(127) + + // Check if it's a valid point on the curve that's + // not canonical + err := point.UnmarshalBinary(buffer) + if err == nil && !point.IsCanonical(buffer) { + actualNonCanonicalCount++ + } + + // flip bit + buffer[31] |= 128 + + // Check if it's a valid point on the curve that's + // not canonical + err = point.UnmarshalBinary(buffer) + if err == nil && !point.IsCanonical(buffer) { + actualNonCanonicalCount++ + } + } + require.Equal(t, expectedNonCanonicalCount, actualNonCanonicalCount, "Incorrect number of non canonical points detected") +} diff --git a/group/edwards25519/scalar.go b/group/edwards25519/scalar.go index 30caa0253..ac26a68c5 100644 --- a/group/edwards25519/scalar.go +++ b/group/edwards25519/scalar.go @@ -2230,3 +2230,37 @@ func scReduce(out *[32]byte, s *[64]byte) { out[30] = byte(s11 >> 9) out[31] = byte(s11 >> 17) } + +// IsCanonical whether scalar s is in the range 0<=s= 0; i-- { + // subtraction might lead to an underflow which needs + // to be accounted for in the right shift + c |= byte((uint16(sb[i])-uint16(L[i]))>>8) & n + n &= byte((uint16(sb[i]) ^ uint16(L[i]) - 1) >> 8) + } + + return c != 0 +} diff --git a/group/edwards25519/scalar_test.go b/group/edwards25519/scalar_test.go index 0e8459c9a..5c50864a6 100644 --- a/group/edwards25519/scalar_test.go +++ b/group/edwards25519/scalar_test.go @@ -2,6 +2,7 @@ package edwards25519 import ( "fmt" + "math/big" "testing" "github.com/stretchr/testify/require" @@ -457,3 +458,24 @@ func scSubFact(s, a, c *[32]byte) { scReduceLimbs(limbs) } + +// Test_ScalarIsCanonical ensures that scalars >= primeOrder are +// considered non canonical. +func Test_ScalarIsCanonical(t *testing.T) { + candidate := big.NewInt(-2) + candidate.Add(candidate, primeOrder) + + candidateBuf := candidate.Bytes() + for i, j := 0, len(candidateBuf)-1; i < j; i, j = i+1, j-1 { + candidateBuf[i], candidateBuf[j] = candidateBuf[j], candidateBuf[i] + } + + expected := []bool{true, true, false, false} + scalar := scalar{} + + // We check in range [L-2, L+4) + for i := 0; i < 4; i++ { + require.Equal(t, expected[i], scalar.IsCanonical(candidateBuf), fmt.Sprintf("`lMinus2 + %d` does not pass canonicality test", i)) + candidateBuf[0]++ + } +} diff --git a/sign/eddsa/eddsa.go b/sign/eddsa/eddsa.go index e7bfbf135..0e8c9633f 100644 --- a/sign/eddsa/eddsa.go +++ b/sign/eddsa/eddsa.go @@ -7,7 +7,6 @@ import ( "crypto/sha512" "errors" "fmt" - "math/big" "go.dedis.ch/kyber/v3" "go.dedis.ch/kyber/v3/group/edwards25519" @@ -15,11 +14,6 @@ import ( var group = new(edwards25519.Curve) -// TODO: maybe export prime and primeOrder from edwards25519/const or allow it to be -// retrieved from the curve? -var prime, _ = new(big.Int).SetString("57896044618658097711785492504343953926634992332820282019728792003956564819949", 10) -var primeOrder, _ = new(big.Int).SetString("7237005577332262213973186563042994240857116359379907606001950938285454250989", 10) - // EdDSA is a structure holding the data necessary to make a series of // EdDSA signatures. type EdDSA struct { @@ -32,15 +26,6 @@ type EdDSA struct { prefix []byte } -// edDSAPoint is used to verify signatures -// with checks around canonicality and group order -type edDSAPoint interface { - kyber.Point - // HasSmallOrder checks if the given buffer (in little endian) - // represents a point with a small order - HasSmallOrder() bool -} - // NewEdDSA will return a freshly generated key pair to use for generating // EdDSA signatures. func NewEdDSA(stream cipher.Stream) *EdDSA { @@ -143,21 +128,28 @@ func VerifyWithChecks(pub, msg, sig []byte) error { if len(sig) != 64 { return fmt.Errorf("signature length invalid, expect 64 but got %v", len(sig)) } - if !scalarIsCanonical(sig[32:]) { + + type scalarCanCheckCanonical interface { + IsCanonical(b []byte) bool + } + + if !group.Scalar().(scalarCanCheckCanonical).IsCanonical(sig[32:]) { return fmt.Errorf("signature is not canonical") } - if !pointIsCanonical(pub) { - return fmt.Errorf("public key is not canonical") + + type pointCanCheckCanonicalAndSmallOrder interface { + HasSmallOrder() bool + IsCanonical(b []byte) bool } - if !pointIsCanonical(sig[:32]) { + R := group.Point() + if !R.(pointCanCheckCanonicalAndSmallOrder).IsCanonical(sig[:32]) { return fmt.Errorf("R is not canonical") } - R := group.Point() if err := R.UnmarshalBinary(sig[:32]); err != nil { return fmt.Errorf("got R invalid point: %s", err) } - if R.(edDSAPoint).HasSmallOrder() { + if R.(pointCanCheckCanonicalAndSmallOrder).HasSmallOrder() { return fmt.Errorf("R has small order") } @@ -167,10 +159,13 @@ func VerifyWithChecks(pub, msg, sig []byte) error { } public := group.Point() + if !public.(pointCanCheckCanonicalAndSmallOrder).IsCanonical(pub) { + return fmt.Errorf("public key is not canonical") + } if err := public.UnmarshalBinary(pub); err != nil { return fmt.Errorf("invalid public key: %s", err) } - if public.(edDSAPoint).HasSmallOrder() { + if public.(pointCanCheckCanonicalAndSmallOrder).HasSmallOrder() { return fmt.Errorf("public key has small order") } @@ -201,59 +196,3 @@ func Verify(public kyber.Point, msg, sig []byte) error { } return VerifyWithChecks(PBuf, msg, sig) } - -// scalarIsCanonical whether scalar s is in the range 0<=s= 0; i-- { - // subtraction might lead to an underflow which needs - // to be accounted for in the right shift - c |= byte((uint16(sb[i])-uint16(L[i]))>>8) & n - n &= byte((uint16(sb[i]) ^ uint16(L[i]) - 1) >> 8) - } - - return c != 0 -} - -// pointIsCanonical determines whether the group element is canonical -// -// Checks whether group element s is less than p, according to RFC8032§5.1.3.1 -// https://tools.ietf.org/html/rfc8032#section-5.1.3 -// -// Taken from -// https://github.com/jedisct1/libsodium/blob/4744636721d2e420f8bbe2d563f31b1f5e682229/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c#L1113 -func pointIsCanonical(s []byte) bool { - if len(s) != 32 { - return false - } - - c := (s[31] & 0x7f) ^ 0x7f - for i := 30; i > 0; i-- { - c |= s[i] ^ 0xff - } - - // subtraction might underflow - c = byte((uint16(c) - 1) >> 8) - d := byte((0xed - 1 - uint16(s[0])) >> 8) - - return 1-(c&d&1) == 1 -} diff --git a/sign/eddsa/eddsa_test.go b/sign/eddsa/eddsa_test.go index 126139e76..2f7b88334 100644 --- a/sign/eddsa/eddsa_test.go +++ b/sign/eddsa/eddsa_test.go @@ -6,8 +6,6 @@ import ( "compress/gzip" "crypto/cipher" "encoding/hex" - "fmt" - "math/big" "math/rand" "os" "strings" @@ -346,60 +344,3 @@ func TestGolden(t *testing.T) { t.Fatalf("error reading test data: %s", err) } } - -// Test_pointIsCanonical ensures that elements >= p are considered -// non canonical -func Test_pointIsCanonical(t *testing.T) { - - // buffer stores the candidate points (in little endian) that we'll test - // against, starting with `prime` - buffer := prime.Bytes() - for i, j := 0, len(buffer)-1; i < j; i, j = i+1, j-1 { - buffer[i], buffer[j] = buffer[j], buffer[i] - } - - // Iterate over the 19*2 finite field elements - point := group.Point() - actualNonCanonicalCount := 0 - expectedNonCanonicalCount := 24 - for i := 0; i < 19; i++ { - buffer[0] = byte(237 + i) - buffer[31] = byte(127) - - // Check if it's a valid point on the curve that's - // not canonical - err := point.UnmarshalBinary(buffer) - if err == nil && !pointIsCanonical(buffer) { - actualNonCanonicalCount++ - } - - // flip bit - buffer[31] |= 128 - - // Check if it's a valid point on the curve that's - // not canonical - err = point.UnmarshalBinary(buffer) - if err == nil && !pointIsCanonical(buffer) { - actualNonCanonicalCount++ - } - } - require.Equal(t, expectedNonCanonicalCount, actualNonCanonicalCount, "Incorrect number of non canonical points detected") -} - -func Test_scalarIsCanonical(t *testing.T) { - candidate := big.NewInt(-2) - candidate.Add(candidate, primeOrder) - - candidateBuf := candidate.Bytes() - for i, j := 0, len(candidateBuf)-1; i < j; i, j = i+1, j-1 { - candidateBuf[i], candidateBuf[j] = candidateBuf[j], candidateBuf[i] - } - - expected := []bool{true, true, false, false} - - // We check in range [L-2, L+4) - for i := 0; i < 4; i++ { - require.Equal(t, expected[i], scalarIsCanonical(candidateBuf), fmt.Sprintf("`lMinus2 + %d` does not pass canonicality test", i)) - candidateBuf[0]++ - } -} diff --git a/sign/schnorr/schnorr.go b/sign/schnorr/schnorr.go index 9d0dae6b8..969cb9b01 100644 --- a/sign/schnorr/schnorr.go +++ b/sign/schnorr/schnorr.go @@ -18,8 +18,6 @@ import ( "fmt" "go.dedis.ch/kyber/v3" - "go.dedis.ch/kyber/v3/group/edwards25519" - "go.dedis.ch/kyber/v3/sign/eddsa" ) // Suite represents the set of functionalities needed by the package schnorr. @@ -65,8 +63,13 @@ func Sign(s Suite, private kyber.Scalar, msg []byte) ([]byte, error) { // additional checks around the canonicality and ensures the public key // does not have a small order when using `edwards25519` group. func VerifyWithChecks(g kyber.Group, pub, msg, sig []byte) error { - if _, ok := g.(*edwards25519.SuiteEd25519); ok { - return eddsa.VerifyWithChecks(pub, msg, sig) + type scalarCanCheckCanonical interface { + IsCanonical(b []byte) bool + } + + type pointCanCheckCanonicalAndSmallOrder interface { + HasSmallOrder() bool + IsCanonical(b []byte) bool } R := g.Point() @@ -80,6 +83,17 @@ func VerifyWithChecks(g kyber.Group, pub, msg, sig []byte) error { if err := R.UnmarshalBinary(sig[:pointSize]); err != nil { return err } + if p, ok := R.(pointCanCheckCanonicalAndSmallOrder); ok { + if !p.IsCanonical(sig[:pointSize]) { + return fmt.Errorf("R is not canonical") + } + if p.HasSmallOrder() { + return fmt.Errorf("R has small order") + } + } + if s, ok := g.Scalar().(scalarCanCheckCanonical); ok && !s.IsCanonical(sig[pointSize:]) { + return fmt.Errorf("signature is not canonical") + } if err := s.UnmarshalBinary(sig[pointSize:]); err != nil { return err } @@ -89,6 +103,14 @@ func VerifyWithChecks(g kyber.Group, pub, msg, sig []byte) error { if err != nil { return fmt.Errorf("schnorr: error unmarshalling public key") } + if p, ok := public.(pointCanCheckCanonicalAndSmallOrder); ok { + if !p.IsCanonical(pub) { + return fmt.Errorf("public key is not canonical") + } + if p.HasSmallOrder() { + return fmt.Errorf("public key has small order") + } + } // recompute hash(public || R || msg) h, err := hash(g, public, R, msg) if err != nil { From 6a0ad452aa1b959a41df5c16327999c0ecc49e54 Mon Sep 17 00:00:00 2001 From: Gaurav Narula Date: Thu, 13 Aug 2020 12:14:07 +0200 Subject: [PATCH 3/3] Jeff's Comments Co-authored-by: Jeff R. Allen --- group/edwards25519/point.go | 2 +- group/edwards25519/scalar.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/group/edwards25519/point.go b/group/edwards25519/point.go index e04f0d3a1..191450ee0 100644 --- a/group/edwards25519/point.go +++ b/group/edwards25519/point.go @@ -272,7 +272,7 @@ func (P *point) HasSmallOrder() bool { // Taken from // https://github.com/jedisct1/libsodium/blob/4744636721d2e420f8bbe2d563f31b1f5e682229/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c#L1113 // -// The meethod accepts a buffer instead of calling `MarshalBianry` on the receiver +// The method accepts a buffer instead of calling `MarshalBinary` on the receiver // because that always returns a value modulo `prime`. func (P *point) IsCanonical(s []byte) bool { if len(s) != 32 { diff --git a/group/edwards25519/scalar.go b/group/edwards25519/scalar.go index ac26a68c5..cd5195294 100644 --- a/group/edwards25519/scalar.go +++ b/group/edwards25519/scalar.go @@ -2231,13 +2231,13 @@ func scReduce(out *[32]byte, s *[64]byte) { out[31] = byte(s11 >> 17) } -// IsCanonical whether scalar s is in the range 0<=s