edwards25519.FromBytes: do not negate the point

edwards25519.FromBytes is an exported method and should thus live up to
its name. In particular, X == ToBytes(FromBytes(X)) should hold. Up to
now, FromBytes silently negate the point it was deserializing, and
ed25519.Verify was missing a negation -- these two defects cancelled
each other out.

ed25519.go

@@ -103,6 +103,8 @@ func Verify(publicKey *[PublicKeySize]byte, message []byte, sig *[SignatureSize]
 	if !A.FromBytes(publicKey) {
 		return false
 	}
+	edwards25519.FeNeg(&A.X, &A.X)
+	edwards25519.FeNeg(&A.T, &A.T)
 
 	h := sha512.New()
 	h.Write(sig[:32])

edwards25519/edwards25519.go

@@ -754,7 +754,7 @@ func (p *ExtendedGroupElement) FromBytes(s *[32]byte) bool {
 		}
 	}
 
-	if FeIsNegative(&p.X) == (s[31] >> 7) {
+	if FeIsNegative(&p.X) != (s[31] >> 7) {
 		FeNeg(&p.X, &p.X)
 	}
 

marshal_test.go

@@ -0,0 +1,58 @@
+package ed25519
+
+import (
+	"crypto/rand"
+	"testing"
+
+	"github.com/agl/ed25519/edwards25519"
+)
+
+func TestUnmarshalMarshal(t *testing.T) {
+	pk, _, _ := GenerateKey(rand.Reader)
+
+	var A edwards25519.ExtendedGroupElement
+	ret := A.FromBytes(pk)
+
+	var pk2 [32]byte
+	A.ToBytes(&pk2)
+
+	if *pk != pk2 {
+		_ = ret
+		t.Errorf("FromBytes(%v)->ToBytes not idempotent:\n%x\nbytes:\n\t%x\n\t%x\ndelta: %x\n", ret, A, *pk, pk2, int(pk[31])-int(pk2[31]))
+	}
+}
+
+func TestUnmarshalMarshalTwice(t *testing.T) {
+	pk, _, _ := GenerateKey(rand.Reader)
+
+	var A edwards25519.ExtendedGroupElement
+	A.FromBytes(pk)
+
+	var pk2 [32]byte
+	A.ToBytes(&pk2)
+
+	var B edwards25519.ExtendedGroupElement
+	ret := B.FromBytes(&pk2)
+
+	var pk3 [32]byte
+	B.ToBytes(&pk3)
+
+	if *pk != pk3 {
+		t.Errorf("FromBytes(%v)->ToBytes not idempotent:\n%x\nbytes:\n\t%x\n\t%x\ndelta: %x\n", ret, A, *pk, pk3, int(pk[31])-int(pk2[31]))
+	}
+}
+
+func TestUnmarshalMarshalNegative(t *testing.T) {
+	pk, _, _ := GenerateKey(rand.Reader)
+
+	var A edwards25519.ExtendedGroupElement
+	ret := A.FromBytes(pk)
+
+	var pk2 [32]byte
+	A.ToBytes(&pk2)
+	pk2[31] ^= 0x80
+
+	if *pk == pk2 {
+		t.Errorf("flipping sign did not change public key:\n%x\nbytes:\n\t%x\n\t%x\ndelta: %x\n", ret, A, *pk, pk2, int(pk[31])-int(pk2[31]))
+	}
+}