crypto/secp256k1: add checking z sign in affineFromJacobian

[hbakhtiyor]

No description provided.

[karalabe]

What is the purpose of this PR? I don't think we're willing to accept modifications to our crypto libraries without very strong arguments.

[hbakhtiyor]

checking for infinity
zinv := new(big.Int).ModInverse(z, BitCurve.P) we got nil of zinv
and zinvsq := new(big.Int).Mul(zinv, zinv) throws error on Mul func

[hbakhtiyor]

https://github.com/hbakhtiyor/schnorr, here's my usecase

[hbakhtiyor]

@karalabe any updates?

[fjl]

I will figure out if this change can be included as soon as I have a free afternoon. It's been a while since I've looked into EC math. Please be patient :).

[MariusVanDerWijden]

The same fix was proposed in #19934 again. In order to keep our backlog clean I closed the other PR.

The other PR received the following comment from @CoreyLin

The purpose of adding if z.Sign() == 0 here is to prevent that z and BitCurve.P are not relatively prime which would lead to zinv == nil , right? If so, besides z.Sign() == 0 , there are other conditions/situations which may cause z and BitCurve.P to be not relatively prime. In other words, z.Sign() == 0 is not the only reason to cause z and BitCurve.P to be not relatively prime.

Maybe you can furthur check source code of GCD method of big.Int as following:

// GCD sets z to the greatest common divisor of a and b, which both must
// be > 0, and returns z.
// If x or y are not nil, GCD sets their value such that z = a*x + b*y.
// If either a or b is <= 0, GCD sets z = x = y = 0.
func (z *Int) GCD(x, y, a, b *Int) *Int {
	if a.Sign() <= 0 || b.Sign() <= 0 {
		z.SetInt64(0)
		if x != nil {
			x.SetInt64(0)
		}
		if y != nil {
			y.SetInt64(0)
		}
		return z
	}

	return z.lehmerGCD(x, y, a, b)
}

Here z is passed in as a , and BitCurve.P is passed in as b. As long as a and b are not relatively prime, the return value would not be equal to 1.

[MariusVanDerWijden]

Okay so the z == 0 is hit whenever we Add two points with the same x1/x2 coordinate.
In this case h in line 149 becomes 0, therefore z3 in line 195 becomes zero thus hitting the bug in affineFromJacobian.

Tests (produces sigsegv on master, nothing on this branch):

func TestAffine(t *testing.T) {
	x := new(big.Int)
	y := new(big.Int)
	z := new(big.Int)
	theCurve.affineFromJacobian(x, y, z)
}

func TestAdd(t *testing.T) {
	x := new(big.Int)
	y := new(big.Int)
	z := new(big.Int)
	w := new(big.Int)
	theCurve.Add(x, y, z, w)
}

Elliptic uses the same check in their affineFromJacobian code: https://golang.org/src/crypto/elliptic/elliptic.go l.95