EdDSA Verification Checks #430

gnarula · 2020-07-30T15:47:41Z

This PR builds on #427 with the following modifications:

The IsCanonical and HasSmallOrder checks are moved to Point and Scalar implementations of edwards25519
Tests use the constants defined in edwards25519
Ensured bitwise operations are on unsigned types / bytes
Added eddsa.VerifyWithChecks(pub, msg, sig []byte) which checks constraints as imposed by RFC8032 and group order checks as suggested in https://eprint.iacr.org/2020/823.pdf and implemented in libsodium

… and integrating it in Point / Scalar structure

ineiti

I would replace all assert with require, as most of the time it doesn't make sense to continue the test, e.g., if the msg := random.Bits... fails, no reason to continue the test.

Also, there are some assert.Nil which should be require.NoError.

I thought that HasSmallOrder is defined on the point and doesn't need to take a slice of bytes, no?

Also, IsCanonical could be defined only in the eddsa package, as it doesn't make sense in the edwards25519 package, given that Points and Scalars always are canonical.

ineiti · 2020-07-31T07:26:33Z

sign/eddsa/eddsa_test.go

 // Test the property of a EdDSA signature
 func TestEdDSASigningRandom(t *testing.T) {
-	suite := edwards25519.NewBlakeSHA256Ed25519()
+	//suite := edwards25519.NewBlakeSHA256Ed25519()


Suggested change

//suite := edwards25519.NewBlakeSHA256Ed25519()

ineiti · 2020-07-31T07:28:44Z

sign/eddsa/eddsa_test.go

+	msg := make([]byte, 32)
+	_, err := rand.Read(msg)
+	assert.NoError(t, err)
+


Suggested change

msg := make([]byte, 32)

_, err := rand.Read(msg)

assert.NoError(t, err)

msg := random.Bits(256, true, random.New())

ineiti · 2020-07-31T07:29:15Z

sign/eddsa/eddsa_test.go

+	msg := make([]byte, 32)
+	_, err := rand.Read(msg)
+	assert.NoError(t, err)
+


Suggested change

msg := make([]byte, 32)

_, err := rand.Read(msg)

assert.NoError(t, err)

msg := random.Bits(256, true, random.New())

ineiti · 2020-07-31T07:29:45Z

sign/eddsa/eddsa_test.go

+}
+
+// Test non-canonical keys
+// TODO check with p+2


Can this check be easily added?

ineiti · 2020-07-31T07:30:05Z

sign/eddsa/eddsa_test.go

+	err = ed.Public.UnmarshalBinary(publicKey)
+	assert.Nil(t, err)


Suggested change

err = ed.Public.UnmarshalBinary(publicKey)

assert.Nil(t, err)

require.NoError(ed.Public.UnmarshalBinary(publicKey))

ineiti · 2020-07-31T07:30:24Z

sign/eddsa/eddsa_test.go

+	assert.Nil(t, err)
+
+	err = Verify(ed.Public, msg2, sig2)
+	assert.Equal(t, fmt.Errorf("signature is not canonical"), err)


Suggested change

assert.Equal(t, fmt.Errorf("signature is not canonical"), err)

require.Equal(t, fmt.Errorf("signature is not canonical"), err)

ineiti · 2020-07-31T07:31:00Z

sign/eddsa/eddsa_test.go

+	msg := make([]byte, 32)
+	_, err := rand.Read(msg)
+	assert.NoError(t, err)
+


Suggested change

msg := make([]byte, 32)

_, err := rand.Read(msg)

assert.NoError(t, err)

msg := random.Bits(256, true, random.New())

ineiti · 2020-07-31T07:31:16Z

sign/eddsa/eddsa_test.go

+	assert.NoError(t, err)
+
+	sig, err := ed.Sign(msg)
+	assert.Nil(t, err)


Suggested change

assert.Nil(t, err)

require.NoError(t, err)

ineiti · 2020-07-31T07:31:30Z

sign/eddsa/eddsa_test.go

+
+	sig, err := ed.Sign(msg)
+	assert.Nil(t, err)
+	assert.Nil(t, Verify(ed.Public, msg, sig))


Suggested change

assert.Nil(t, Verify(ed.Public, msg, sig))

require.NoError(t, Verify(ed.Public, msg, sig))

ineiti · 2020-07-31T07:32:01Z

sign/eddsa/eddsa_test.go

+	}
+
+	err = Verify(ed.Public, msg, sig)
+	assert.Equal(t, fmt.Errorf("signature is not canonical"), err)


Suggested change

assert.Equal(t, fmt.Errorf("signature is not canonical"), err)

require.Equal(t, fmt.Errorf("signature is not canonical"), err)

gnarula · 2020-07-31T08:14:08Z

I would replace all assert with require, as most of the time it doesn't make sense to continue the test, e.g., if the msg := random.Bits... fails, no reason to continue the test.

Also, there are some assert.Nil which should be require.NoError.

Yeah, I'm yet to work my way through eddsa_test. Realised it quite late yesterday after I created the PR and hence the TODO commit/messages in haste :)

I thought that HasSmallOrder is defined on the point and doesn't need to take a slice of bytes, no?

I used the byte slice because point.MarshalBinary() returns values modulo prime and we have prime and prime+1 in weak keys.

Also, IsCanonical could be defined only in the eddsa package, as it doesn't make sense in the edwards25519 package, given that Points and Scalars always are canonical.

👍

ineiti · 2020-07-31T08:24:56Z

I thought that HasSmallOrder is defined on the point and doesn't need to take a slice of bytes, no?

I used the byte slice because point.MarshalBinary() returns values modulo prime and we have prime and prime+1 in weak keys.

Yes - I still don't really understand what this check does with all the XORs :( This is where I would've liked to have an input from Simone, to know what makes most sense. From an engineering point of view HasSmallOrder([]byte) is a static method, which doesn't belong in a structure, but should be kept in the package. But as it only applies to ed25519.points, it is cleaner to be defined on the structure, but then it shouldn't be a static method, so it should use the structure.
But then, as you write, we will never have the prime and prime+1 as weak keys.

Is it correct to replace prime and prime+1 with 0 and 1? As we're always counting modulo prime?

gnarula · 2020-07-31T09:02:17Z

I still don't really understand what this check does with all the XORs

If I understand it correctly, the XORing ensures that c[i] = 0 iff the given slice is the same as the weakKeys[i].

But then, as you write, we will never have the prime and prime+1 as weak keys.

We might run into that at https://github.com/dedis/kyber/pull/430/files#diff-bd9209ce75feb92351db6c68bd8010d7R154

ineiti · 2020-07-31T09:47:22Z

sign/eddsa/eddsa.go

 	if len(sig) != 64 {
 		return fmt.Errorf("signature length invalid, expect 64 but got %v", len(sig))
 	}
+	if (sig[63]&240) > 0 && !group.Scalar().(edDSAScalar).IsCanonical(sig[32:]) {


The (sig[63]&240) > 0 should also go in IsCanonical.

ineiti · 2020-07-31T09:48:02Z

But then, as you write, we will never have the prime and prime+1 as weak keys.

We might run into that at https://github.com/dedis/kyber/pull/430/files#diff-bd9209ce75feb92351db6c68bd8010d7R154

This doesn't make sense: if the keys are supposed to be < prime, then how can you test against prime and prime+1 weak keys?

OK, crypto.stackexchange to the rescue:

https://crypto.stackexchange.com/questions/55632/libsodium-x25519-and-ed25519-small-order-check

So, if I get that right, the second-last paragraph has a nice summary:

The blacklist is the list of all points that will give zero as the ‘shared secret’. It does not list all illegitimate public keys—there are too many to list, of the form 𝐵+𝑄 where 𝐵 is a legitimate public key (of which there are 𝑝1 possibilities) and 𝑄 is on the blacklist. The only way to detect legitimate public keys is to confirm that [𝑝1]𝐵=0.

So if we test for p and p+1 in non-reduced numbers, they will fall on 0 and 1 anyway, which are already tested. Which makes me quite convinced that we can:

from an engineering point of view: define HasSmallOrder on ed25519.point
from a mathematical point of view: only check the first 5 weak keys, as p and p+1 will be caught by the reduction and the check with 0 and 1
from a verification point of view: by UnmarshalBinarying the []byte in eddsa.VerifyWithChecks after we check the IsCanonical

But I don't see yet where we should/could check for the correct subgroup with prime * Point = 0, like the following does: https://github.com/kilic/bls12-381/blob/59713d4652a1851eb2ad0fb980533a5472294fe0/g1.go#L280

That's probably for another PR.

gnarula · 2020-07-31T12:06:06Z

This doesn't make sense: if the keys are supposed to be < prime, then how can you test against prime and prime+1 weak keys?

Aha, I was under the impression that R = sig[:32] could be > prime but I read RFC8032 again and it does indeed mention that verification should fail if R cannot be decoded. Decoding rules require failure for non canonical points. This is what you suggest in the third bullet point as well. 👍 Working on the changes now

Added `edda.VerifyWithChecks` which checks if the scalars and points are canonical and ensures the points do not have a small order. Refer: RFC8032§5.1.7 and https://eprint.iacr.org/2020/823.pdf Builds on top of dedis#427 and closes dedis#426 and dedis#311. Co-authored-by: David Cerezo <david@calctopia.com> Co-authored-by: Linus Gasser <linus.gasser@epfl.ch>

Added `edda.VerifyWithChecks` which checks if the scalars and points are canonical and ensures the points do not have a small order. Refer: RFC8032§5.1.7 and https://eprint.iacr.org/2020/823.pdf Builds on top of #427 and closes #426 and #311. Co-authored-by: David Cerezo <david@calctopia.com> Co-authored-by: Linus Gasser <linus.gasser@epfl.ch>

ineiti and others added 4 commits July 28, 2020 15:31

Starting from https://github.com/Calctopia-OpenSource/kyber#fix_ed25519…

5925f8c

… and integrating it in Point / Scalar structure

work_in_progress

b0c96fe

Add canonicality and order checks in EdDSA verification

4c9685a

Mark EdDSA tests as TODO

c334b22

gnarula requested a review from ineiti July 30, 2020 15:47

ineiti requested changes Jul 31, 2020

View reviewed changes

ineiti reviewed Jul 31, 2020

View reviewed changes

Linus' comments

873ec33

gnarula force-pushed the ed25519_checks branch from f13731d to 873ec33 Compare July 31, 2020 14:16

nikkolasg approved these changes Aug 4, 2020

View reviewed changes

Remove TODO

2288790

gnarula changed the title ~~[WIP] EdDSA Verification Checks~~ EdDSA Verification Checks Aug 6, 2020

gnarula merged commit 88245de into master Aug 6, 2020

gnarula mentioned this pull request Aug 6, 2020

Fix ed25519 verification algorithm #427

Closed

calctopian mentioned this pull request Aug 6, 2020

Fixing schnorr.Verify #432

Closed

ineiti deleted the ed25519_checks branch August 17, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EdDSA Verification Checks #430

EdDSA Verification Checks #430

gnarula commented Jul 30, 2020

ineiti left a comment

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

ineiti Jul 31, 2020

gnarula commented Jul 31, 2020

ineiti commented Jul 31, 2020

gnarula commented Jul 31, 2020 •

edited

Loading

ineiti Jul 31, 2020

ineiti commented Jul 31, 2020

gnarula commented Jul 31, 2020

		err = ed.Public.UnmarshalBinary(publicKey)
		assert.Nil(t, err)

	err = ed.Public.UnmarshalBinary(publicKey)
	assert.Nil(t, err)
	require.NoError(ed.Public.UnmarshalBinary(publicKey))

	assert.Equal(t, fmt.Errorf("signature is not canonical"), err)
	require.Equal(t, fmt.Errorf("signature is not canonical"), err)

	assert.Nil(t, Verify(ed.Public, msg, sig))
	require.NoError(t, Verify(ed.Public, msg, sig))

EdDSA Verification Checks #430

EdDSA Verification Checks #430

Conversation

gnarula commented Jul 30, 2020

ineiti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnarula commented Jul 31, 2020

ineiti commented Jul 31, 2020

gnarula commented Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

ineiti commented Jul 31, 2020

gnarula commented Jul 31, 2020

gnarula commented Jul 31, 2020 •

edited

Loading