crypto/internal/fips140: remove key import PCTs, make keygen PCTs fatal

CMVP clarified with the September 2nd changes to IG 10.3.A that PCTs
don't need to run on imported keys.

However, PCT failure must enter the error state (which for us is fatal).

Thankfully, now that PCTs only run on key generation, we can be assured
they will never fail.

This change should only affect FIPS 140-3 mode.

While at it, make the CAST/PCT testing more robust, checking
TestConditional is terminated by a fatal error (and not by t.Fatal).

Fixes #74947
Updates #69536

Change-Id: I6a6a696439e1560c10f3cce2cb208fd40c5bc641
Reviewed-on: https://go-review.googlesource.com/c/go/+/701517
Reviewed-by: Daniel McCarney <[email protected]>
Reviewed-by: Junyang Shao <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
TryBot-Bypass: Filippo Valsorda <[email protected]>

Author: FiloSottile

src/crypto/internal/fips140/cast.go

@@ -56,27 +56,34 @@ func CAST(name string, f func() error) {
 }
 
 // PCT runs the named Pairwise Consistency Test (if operated in FIPS mode) and
-// returns any errors. If an error is returned, the key must not be used.
+// aborts the program (stopping the module input/output and entering the "error
+// state") if the test fails.
 //
-// PCTs are mandatory for every key pair that is generated/imported, including
+// PCTs are mandatory for every generated (but not imported) key pair, including
 // ephemeral keys (which effectively doubles the cost of key establishment). See
 // Implementation Guidance 10.3.A Additional Comment 1.
 //
 // The name must not contain commas, colons, hashes, or equal signs.
 //
 // If a package p calls PCT during key generation, an invocation of that
 // function should be added to fipstest.TestConditionals.
-func PCT(name string, f func() error) error {
+func PCT(name string, f func() error) {
 	if strings.ContainsAny(name, ",#=:") {
 		panic("fips: invalid self-test name: " + name)
 	}
 	if !Enabled {
-		return nil
+		return
 	}
 
 	err := f()
 	if name == failfipscast {
 		err = errors.New("simulated PCT failure")
 	}
-	return err
+	if err != nil {
+		fatal("FIPS 140-3 self-test failed: " + name + ": " + err.Error())
+		panic("unreachable")
+	}
+	if debug {
+		println("FIPS 140-3 PCT passed:", name)
+	}
 }

src/crypto/internal/fips140/ecdh/ecdh.go

@@ -161,6 +161,27 @@ func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) {
 		if err != nil {
 			continue
 		}
+
+		// A "Pairwise Consistency Test" makes no sense if we just generated the
+		// public key from an ephemeral private key. Moreover, there is no way to
+		// check it aside from redoing the exact same computation again. SP 800-56A
+		// Rev. 3, Section 5.6.2.1.4 acknowledges that, and doesn't require it.
+		// However, ISO 19790:2012, Section 7.10.3.3 has a blanket requirement for a
+		// PCT for all generated keys (AS10.35) and FIPS 140-3 IG 10.3.A, Additional
+		// Comment 1 goes out of its way to say that "the PCT shall be performed
+		// consistent [...], even if the underlying standard does not require a
+		// PCT". So we do it. And make ECDH nearly 50% slower (only) in FIPS mode.
+		fips140.PCT("ECDH PCT", func() error {
+			p1, err := c.newPoint().ScalarBaseMult(privateKey.d)
+			if err != nil {
+				return err
+			}
+			if !bytes.Equal(p1.Bytes(), privateKey.pub.q) {
+				return errors.New("crypto/ecdh: public key does not match private key")
+			}
+			return nil
+		})
+
 		return privateKey, nil
 	}
 }
@@ -188,28 +209,6 @@ func NewPrivateKey[P Point[P]](c *Curve[P], key []byte) (*PrivateKey, error) {
 		panic("crypto/ecdh: internal error: public key is the identity element")
 	}
 
-	// A "Pairwise Consistency Test" makes no sense if we just generated the
-	// public key from an ephemeral private key. Moreover, there is no way to
-	// check it aside from redoing the exact same computation again. SP 800-56A
-	// Rev. 3, Section 5.6.2.1.4 acknowledges that, and doesn't require it.
-	// However, ISO 19790:2012, Section 7.10.3.3 has a blanket requirement for a
-	// PCT for all generated keys (AS10.35) and FIPS 140-3 IG 10.3.A, Additional
-	// Comment 1 goes out of its way to say that "the PCT shall be performed
-	// consistent [...], even if the underlying standard does not require a
-	// PCT". So we do it. And make ECDH nearly 50% slower (only) in FIPS mode.
-	if err := fips140.PCT("ECDH PCT", func() error {
-		p1, err := c.newPoint().ScalarBaseMult(key)
-		if err != nil {
-			return err
-		}
-		if !bytes.Equal(p1.Bytes(), publicKey) {
-			return errors.New("crypto/ecdh: public key does not match private key")
-		}
-		return nil
-	}); err != nil {
-		panic(err)
-	}
-
 	k := &PrivateKey{d: bytes.Clone(key), pub: PublicKey{curve: c.curve, q: publicKey}}
 	return k, nil
 }

src/crypto/internal/fips140/ecdsa/cast.go

@@ -51,8 +51,8 @@ func testHash() []byte {
 	}
 }
 
-func fipsPCT[P Point[P]](c *Curve[P], k *PrivateKey) error {
-	return fips140.PCT("ECDSA PCT", func() error {
+func fipsPCT[P Point[P]](c *Curve[P], k *PrivateKey) {
+	fips140.PCT("ECDSA PCT", func() error {
 		hash := testHash()
 		drbg := newDRBG(sha512.New, k.d, bits2octets(P256(), hash), nil)
 		sig, err := sign(c, k, drbg, hash)

src/crypto/internal/fips140/ecdsa/ecdsa.go

@@ -167,11 +167,6 @@ func NewPrivateKey[P Point[P]](c *Curve[P], D, Q []byte) (*PrivateKey, error) {
 		return nil, err
 	}
 	priv := &PrivateKey{pub: *pub, d: d.Bytes(c.N)}
-	if err := fipsPCT(c, priv); err != nil {
-		// This can happen if the application went out of its way to make an
-		// ecdsa.PrivateKey with a mismatching PublicKey.
-		return nil, err
-	}
 	return priv, nil
 }
 
@@ -204,10 +199,7 @@ func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) {
 		},
 		d: k.Bytes(c.N),
 	}
-	if err := fipsPCT(c, priv); err != nil {
-		// This clearly can't happen, but FIPS 140-3 mandates that we check it.
-		panic(err)
-	}
+	fipsPCT(c, priv)
 	return priv, nil
 }
 

src/crypto/internal/fips140/ed25519/cast.go

@@ -12,8 +12,8 @@ import (
 	"sync"
 )
 
-func fipsPCT(k *PrivateKey) error {
-	return fips140.PCT("Ed25519 sign and verify PCT", func() error {
+func fipsPCT(k *PrivateKey) {
+	fips140.PCT("Ed25519 sign and verify PCT", func() error {
 		return pairwiseTest(k)
 	})
 }

src/crypto/internal/fips140/ed25519/ed25519.go

@@ -69,10 +69,7 @@ func generateKey(priv *PrivateKey) (*PrivateKey, error) {
 	fips140.RecordApproved()
 	drbg.Read(priv.seed[:])
 	precomputePrivateKey(priv)
-	if err := fipsPCT(priv); err != nil {
-		// This clearly can't happen, but FIPS 140-3 requires that we check.
-		panic(err)
-	}
+	fipsPCT(priv)
 	return priv, nil
 }
 
@@ -88,10 +85,6 @@ func newPrivateKeyFromSeed(priv *PrivateKey, seed []byte) (*PrivateKey, error) {
 	}
 	copy(priv.seed[:], seed)
 	precomputePrivateKey(priv)
-	if err := fipsPCT(priv); err != nil {
-		// This clearly can't happen, but FIPS 140-3 requires that we check.
-		panic(err)
-	}
 	return priv, nil
 }
 
@@ -137,12 +130,6 @@ func newPrivateKey(priv *PrivateKey, privBytes []byte) (*PrivateKey, error) {
 
 	copy(priv.prefix[:], h[32:])
 
-	if err := fipsPCT(priv); err != nil {
-		// This can happen if the application messed with the private key
-		// encoding, and the public key doesn't match the seed anymore.
-		return nil, err
-	}
-
 	return priv, nil
 }
 

src/crypto/internal/fips140/mlkem/mlkem1024.go

@@ -177,10 +177,7 @@ func generateKey(dk *DecapsulationKey768) (*DecapsulationKey768, error) {
 	var z [32]byte
 	drbg.Read(z[:])
 	kemKeyGen(dk, &d, &z)
-	if err := fips140.PCT("ML-KEM PCT", func() error { return kemPCT(dk) }); err != nil {
-		// This clearly can't happen, but FIPS 140-3 requires us to check.
-		panic(err)
-	}
+	fips140.PCT("ML-KEM PCT", func() error { return kemPCT(dk) })
 	fips140.RecordApproved()
 	return dk, nil
 }
@@ -208,10 +205,6 @@ func newKeyFromSeed(dk *DecapsulationKey768, seed []byte) (*DecapsulationKey768,
 	d := (*[32]byte)(seed[:32])
 	z := (*[32]byte)(seed[32:])
 	kemKeyGen(dk, d, z)
-	if err := fips140.PCT("ML-KEM PCT", func() error { return kemPCT(dk) }); err != nil {
-		// This clearly can't happen, but FIPS 140-3 requires us to check.
-		panic(err)
-	}
 	fips140.RecordApproved()
 	return dk, nil
 }

src/crypto/internal/fips140/mlkem/mlkem768.go

@@ -105,7 +105,28 @@ func GenerateKey(rand io.Reader, bits int) (*PrivateKey, error) {
 		// negligible chance of failure we can defer the check to the end of key
 		// generation and return an error if it fails. See [checkPrivateKey].
 
-		return newPrivateKey(N, 65537, d, P, Q)
+		k, err := newPrivateKey(N, 65537, d, P, Q)
+		if err != nil {
+			return nil, err
+		}
+
+		if k.fipsApproved {
+			fips140.PCT("RSA sign and verify PCT", func() error {
+				hash := []byte{
+					0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+					0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
+					0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18,
+					0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20,
+				}
+				sig, err := signPKCS1v15(k, "SHA-256", hash)
+				if err != nil {
+					return err
+				}
+				return verifyPKCS1v15(k.PublicKey(), "SHA-256", hash, sig)
+			})
+		}
+
+		return k, nil
 	}
 }
 

src/crypto/internal/fips140/rsa/keygen.go

@@ -320,26 +320,6 @@ func checkPrivateKey(priv *PrivateKey) error {
 		return errors.New("crypto/rsa: d too small")
 	}
 
-	// If the key is still in scope for FIPS mode, perform a Pairwise
-	// Consistency Test.
-	if priv.fipsApproved {
-		if err := fips140.PCT("RSA sign and verify PCT", func() error {
-			hash := []byte{
-				0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
-				0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
-				0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18,
-				0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20,
-			}
-			sig, err := signPKCS1v15(priv, "SHA-256", hash)
-			if err != nil {
-				return err
-			}
-			return verifyPKCS1v15(priv.PublicKey(), "SHA-256", hash, sig)
-		}); err != nil {
-			return err
-		}
-	}
-
 	return nil
 }