CVE-2023-24533: Incorrect Calculation (CWE-682)
value: Multiplication of certain unreduced P-256 scalars produce incorrect results. There are no protocols known at this time that can be attacked due to this.
CWE description: The product performs a calculation that generates incorrect or unintended results that are later used in security-critical decisions or resource management.
CVSS v3 base score: 7.5
Published: 2023-03-08T20:15Z
Repository: https://github.com/FiloSottile/nistec
Language: Go
Files changed: nistec_test.go, p256_asm.go, p256_ordinv.go
Commit date: 2023-02-24 23:20:10 +0100
Commit message: crypto/internal/nistec: reduce P-256 scalar

Unlike the rest of nistec, the P-256 assembly doesn't use complete
addition formulas, meaning that p256PointAdd[Affine]Asm won't return the
correct value if the two inputs are equal.

This was (undocumentedly) ignored in the scalar multiplication loops
because as long as the input point is not the identity and the scalar is
lower than the order of the group, the addition inputs can't be the same.

As part of the math/big rewrite, we went however from always reducing
the scalar to only checking its length, under the incorrect assumption
that the scalar multiplication loop didn't require reduction.

Added a reduction, and while at it added it in P256OrdInverse, too, to
enforce a universal reduction invariant on p256OrdElement values.

Note that if the input point is the infinity, the code currently still
relies on undefined behavior, but that's easily tested to behave
acceptably, and will be addressed in a future CL.

Fixes #58647
Fixes CVE-2023-24532

(Filed with the "safe APIs like complete addition formulas are good" dept.)

Change-Id: I7b2c75238440e6852be2710fad66ff1fdc4e2b24

Diff: diff --git a/nistec_test.go b/nistec_test.go
index 90f37fe..9be3e69 100644
--- a/nistec_test.go
+++ b/nistec_test.go
@@ -5,10 +5,11 @@
 package nistec_test
 
 import (
 	"bytes"
 	"crypto/elliptic"
+	"fmt"
 	"math/big"
 	"math/rand"
 	"testing"
 
 	"filippo.io/nistec"
@@ -161,10 +162,90 @@ func testEquivalents[P nistPoint[P]](t *testing.T, newPoint func() P, c elliptic
 	if !bytes.Equal(p1.Bytes(), p6.Bytes()) {
 		t.Error("G+G != [N+2]G")
 	}
 }
 
+func TestScalarMult(t *testing.T) {
+	t.Run("P224", func(t *testing.T) {
+		testScalarMult(t, nistec.NewP224Point, elliptic.P224())
+	})
+	t.Run("P256", func(t *testing.T) {
+		testScalarMult(t, nistec.NewP256Point, elliptic.P256())
+	})
+	t.Run("P384", func(t *testing.T) {
+		testScalarMult(t, nistec.NewP384Point, elliptic.P384())
+	})
+	t.Run("P521", func(t *testing.T) {
+		testScalarMult(t, nistec.NewP521Point, elliptic.P521())
+	})
+}
+
+func testScalarMult[P nistPoint[P]](t *testing.T, newPoint func() P, c elliptic.Curve) {
+	G := newPoint().SetGenerator()
+	checkScalar := func(t *testing.T, scalar []byte) {
+		p1, err := newPoint().ScalarBaseMult(scalar)
+		fatalIfErr(t, err)
+		p2, err := newPoint().ScalarMult(G, scalar)
+		fatalIfErr(t, err)
+		if !bytes.Equal(p1.Bytes(), p2.Bytes()) {
+			t.Error("[k]G != ScalarBaseMult(k)")
+		}
+
+		d := new(big.Int).SetBytes(scalar)
+		d.Sub(c.Params().N, d)
+		d.Mod(d, c.Params().N)
+		g1, err := newPoint().ScalarBaseMult(d.FillBytes(make([]byte, len(scalar))))
+		fatalIfErr(t, err)
+		g1.Add(g1, p1)
+		if !bytes.Equal(g1.Bytes(), newPoint().Bytes()) {
+			t.Error("[N - k]G + [k]G != ∞")
+		}
+	}
+
+	byteLen := len(c.Params().N.Bytes())
+	bitLen := c.Params().N.BitLen()
+	t.Run("0", func(t *testing.T) { checkScalar(t, make([]byte, byteLen)) })
+	t.Run("1", func(t *testing.T) {
+		checkScalar(t, big.NewInt(1).FillBytes(make([]byte, byteLen)))
+	})
+	t.Run("N-1", func(t *testing.T) {
+		checkScalar(t, new(big.Int).Sub(c.Params().N, big.NewInt(1)).Bytes())
+	})
+	t.Run("N", func(t *testing.T) { checkScalar(t, c.Params().N.Bytes()) })
+	t.Run("N+1", func(t *testing.T) {
+		checkScalar(t, new(big.Int).Add(c.Params().N, big.NewInt(1)).Bytes())
+	})
+	t.Run("all1s", func(t *testing.T) {
+		s := new(big.Int).Lsh(big.NewInt(1), uint(bitLen))
+		s.Sub(s, big.NewInt(1))
+		checkScalar(t, s.Bytes())
+	})
+	if testing.Short() {
+		return
+	}
+	for i := 0; i < bitLen; i++ {
+		t.Run(fmt.Sprintf("1<<%d", i), func(t *testing.T) {
+			s := new(big.Int).Lsh(big.NewInt(1), uint(i))
+			checkScalar(t, s.FillBytes(make([]byte, byteLen)))
+		})
+	}
+	// Test N+1...N+32 since they risk overlapping with precomputed table values
+	// in the final additions.
+	for i := int64(2); i <= 32; i++ {
+		t.Run(fmt.Sprintf("N+%d", i), func(t *testing.T) {
+			checkScalar(t, new(big.Int).Add(c.Params().N, big.NewInt(i)).Bytes())
+		})
+	}
+}
+
+func fatalIfErr(t *testing.T, err error) {
+	t.Helper()
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 func BenchmarkScalarMult(b *testing.B) {
 	b.Run("P224", func(b *testing.B) {
 		benchmarkScalarMult(b, nistec.NewP224Point().SetGenerator(), 28)
 	})
 	b.Run("P256", func(b *testing.B) {
diff --git a/p256_asm.go b/p256_asm.go
index 2a0b654..b923875 100644
--- a/p256_asm.go
+++ b/p256_asm.go
@@ -362,10 +362,25 @@ func p256PointDoubleAsm(res, in *P256Point)
 
 // p256OrdElement is a P-256 scalar field element in [0, ord(G)-1] in the
 // Montgomery domain (with R 2²⁵⁶) as four uint64 limbs in little-endian order.
 type p256OrdElement [4]uint64
 
+// p256OrdReduce ensures s is in the range [0, ord(G)-1].
+func p256OrdReduce(s *p256OrdElement) {
+	// Since 2 * ord(G) > 2²⁵⁶, we can just conditionally subtract ord(G),
+	// keeping the result if it doesn't underflow.
+	t0, b := bits.Sub64(s[0], 0xf3b9cac2fc632551, 0)
+	t1, b := bits.Sub64(s[1], 0xbce6faada7179e84, b)
+	t2, b := bits.Sub64(s[2], 0xffffffffffffffff, b)
+	t3, b := bits.Sub64(s[3], 0xffffffff00000000, b)
+	tMask := b - 1 // zero if subtraction underflowed
+	s[0] ^= (t0 ^ s[0]) & tMask
+	s[1] ^= (t1 ^ s[1]) & tMask
+	s[2] ^= (t2 ^ s[2]) & tMask
+	s[3] ^= (t3 ^ s[3]) & tMask
+}
+
 // Add sets q = p1 + p2, and returns q. The points may overlap.
 func (q *P256Point) Add(r1, r2 *P256Point) *P256Point {
 	var sum, double P256Point
 	r1IsInfinity := r1.isInfinity()
 	r2IsInfinity := r2.isInfinity()
@@ -391,10 +406,11 @@ func (r *P256Point) ScalarBaseMult(scalar []byte) (*P256Point, error) {
 	if len(scalar) != 32 {
 		return nil, errors.New("invalid scalar length")
 	}
 	scalarReversed := new(p256OrdElement)
 	p256OrdBigToLittle(scalarReversed, (*[32]byte)(scalar))
+	p256OrdReduce(scalarReversed)
 
 	r.p256BaseMult(scalarReversed)
 	return r, nil
 }
 
@@ -405,10 +421,11 @@ func (r *P256Point) ScalarMult(q *P256Point, scalar []byte) (*P256Point, error)
 	if len(scalar) != 32 {
 		return nil, errors.New("invalid scalar length")
 	}
 	scalarReversed := new(p256OrdElement)
 	p256OrdBigToLittle(scalarReversed, (*[32]byte)(scalar))
+	p256OrdReduce(scalarReversed)
 
 	r.Set(q).p256ScalarMult(scalarReversed)
 	return r, nil
 }
 
diff --git a/p256_ordinv.go b/p256_ordinv.go
index f3e388c..b23f3e5 100644
--- a/p256_ordinv.go
+++ b/p256_ordinv.go
@@ -23,10 +23,11 @@ func p256OrdInverse(k []byte) ([]byte, error) {
 		return nil, errors.New("invalid scalar length")
 	}
 
 	x := new(p256OrdElement)
 	p256OrdBigToLittle(x, (*[32]byte)(k))
+	p256OrdReduce(x)
 
 	// Inversion is implemented as exponentiation by n - 2, per Fermat's little theorem.
 	//
 	// The sequence of 38 multiplications and 254 squarings is derived from
 	// https://briansmith.org/ecc-inversion-addition-chains-01#p256_scalar_inversion

Fixed code: 	"fmt"
func TestScalarMult(t *testing.T) {
	t.Run("P224", func(t *testing.T) {
		testScalarMult(t, nistec.NewP224Point, elliptic.P224())
	})
	t.Run("P256", func(t *testing.T) {
		testScalarMult(t, nistec.NewP256Point, elliptic.P256())
	})
	t.Run("P384", func(t *testing.T) {
		testScalarMult(t, nistec.NewP384Point, elliptic.P384())
	})
	t.Run("P521", func(t *testing.T) {
		testScalarMult(t, nistec.NewP521Point, elliptic.P521())
	})
}

func testScalarMult[P nistPoint[P]](t *testing.T, newPoint func() P, c elliptic.Curve) {
	G := newPoint().SetGenerator()
	checkScalar := func(t *testing.T, scalar []byte) {
		p1, err := newPoint().ScalarBaseMult(scalar)
		fatalIfErr(t, err)
		p2, err := newPoint().ScalarMult(G, scalar)
		fatalIfErr(t, err)
		if !bytes.Equal(p1.Bytes(), p2.Bytes()) {
			t.Error("[k]G != ScalarBaseMult(k)")
		}

		d := new(big.Int).SetBytes(scalar)
		d.Sub(c.Params().N, d)
		d.Mod(d, c.Params().N)
		g1, err := newPoint().ScalarBaseMult(d.FillBytes(make([]byte, len(scalar))))
		fatalIfErr(t, err)
		g1.Add(g1, p1)
		if !bytes.Equal(g1.Bytes(), newPoint().Bytes()) {
			t.Error("[N - k]G + [k]G != ∞")
		}
	}

	byteLen := len(c.Params().N.Bytes())
	bitLen := c.Params().N.BitLen()
	t.Run("0", func(t *testing.T) { checkScalar(t, make([]byte, byteLen)) })
	t.Run("1", func(t *testing.T) {
		checkScalar(t, big.NewInt(1).FillBytes(make([]byte, byteLen)))
	})
	t.Run("N-1", func(t *testing.T) {
		checkScalar(t, new(big.Int).Sub(c.Params().N, big.NewInt(1)).Bytes())
	})
	t.Run("N", func(t *testing.T) { checkScalar(t, c.Params().N.Bytes()) })
	t.Run("N+1", func(t *testing.T) {
		checkScalar(t, new(big.Int).Add(c.Params().N, big.NewInt(1)).Bytes())
	})
	t.Run("all1s", func(t *testing.T) {
		s := new(big.Int).Lsh(big.NewInt(1), uint(bitLen))
		s.Sub(s, big.NewInt(1))
		checkScalar(t, s.Bytes())
	})
	if testing.Short() {
		return
	}
	for i := 0; i < bitLen; i++ {
		t.Run(fmt.Sprintf("1<<%d", i), func(t *testing.T) {
			s := new(big.Int).Lsh(big.NewInt(1), uint(i))
			checkScalar(t, s.FillBytes(make([]byte, byteLen)))
		})
	}
	// Test N+1...N+32 since they risk overlapping with precomputed table values
	// in the final additions.
	for i := int64(2); i <= 32; i++ {
		t.Run(fmt.Sprintf("N+%d", i), func(t *testing.T) {
			checkScalar(t, new(big.Int).Add(c.Params().N, big.NewInt(i)).Bytes())
		})
	}
}

func fatalIfErr(t *testing.T, err error) {
	t.Helper()
	if err != nil {
		t.Fatal(err)
	}
}

// p256OrdReduce ensures s is in the range [0, ord(G)-1].
func p256OrdReduce(s *p256OrdElement) {
	// Since 2 * ord(G) > 2²⁵⁶, we can just conditionally subtract ord(G),
	// keeping the result if it doesn't underflow.
	t0, b := bits.Sub64(s[0], 0xf3b9cac2fc632551, 0)
	t1, b := bits.Sub64(s[1], 0xbce6faada7179e84, b)
	t2, b := bits.Sub64(s[2], 0xffffffffffffffff, b)
	t3, b := bits.Sub64(s[3], 0xffffffff00000000, b)
	tMask := b - 1 // zero if subtraction underflowed
	s[0] ^= (t0 ^ s[0]) & tMask
	s[1] ^= (t1 ^ s[1]) & tMask
	s[2] ^= (t2 ^ s[2]) & tMask
	s[3] ^= (t3 ^ s[3]) & tMask
}

	p256OrdReduce(scalarReversed)
	p256OrdReduce(scalarReversed)
	p256OrdReduce(x)