[release-branch.go1.24] 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 #75523
Updates #69536

Change-Id: I6a6a696439e1560c10f3cce2cb208fd40c5bc641
Reviewed-on: https://go-review.googlesource.com/c/go/+/701438
Reviewed-by: Junyang Shao <shaojunyang@google.com>
TryBot-Bypass: Filippo Valsorda <filippo@golang.org>
Commit-Queue: Junyang Shao <shaojunyang@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>

src/crypto/internal/fips140/cast.go

@@ -56,9 +56,10 @@ 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.
 //
@@ -66,17 +67,23 @@ func CAST(name string, f func() error) {
 //
 // 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

@@ -166,11 +166,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
 }
 
@@ -203,10 +198,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

@@ -118,10 +118,7 @@ func generateKey1024(dk *DecapsulationKey1024) (*DecapsulationKey1024, error) {
 	var z [32]byte
 	drbg.Read(z[:])
 	kemKeyGen1024(dk, &d, &z)
-	if err := fips140.PCT("ML-KEM PCT", func() error { return kemPCT1024(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 kemPCT1024(dk) })
 	fips140.RecordApproved()
 	return dk, nil
 }
@@ -149,10 +146,6 @@ func newKeyFromSeed1024(dk *DecapsulationKey1024, seed []byte) (*DecapsulationKe
 	d := (*[32]byte)(seed[:32])
 	z := (*[32]byte)(seed[32:])
 	kemKeyGen1024(dk, d, z)
-	if err := fips140.PCT("ML-KEM PCT", func() error { return kemPCT1024(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

@@ -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/rsa/keygen.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/rsa.go

@@ -310,26 +310,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
 }
 

src/crypto/internal/fips140test/cast_test.go

@@ -5,9 +5,8 @@
 package fipstest
 
 import (
+	"crypto"
 	"crypto/rand"
-	"crypto/x509"
-	"encoding/pem"
 	"fmt"
 	"internal/testenv"
 	"io/fs"
@@ -17,6 +16,7 @@ import (
 	"strings"
 	"testing"
 
+	"crypto/internal/fips140"
 	// Import packages that define CASTs to test them.
 	_ "crypto/internal/fips140/aes"
 	_ "crypto/internal/fips140/aes/gcm"
@@ -50,8 +50,6 @@ var allCASTs = []string{
 	"KAS-ECC-SSC P-256",
 	"ML-KEM PCT",
 	"ML-KEM PCT",
-	"ML-KEM PCT",
-	"ML-KEM PCT",
 	"ML-KEM-768",
 	"PBKDF2",
 	"RSA sign and verify PCT",
@@ -107,59 +105,63 @@ func TestAllCASTs(t *testing.T) {
 // TestConditionals causes the conditional CASTs and PCTs to be invoked.
 func TestConditionals(t *testing.T) {
 	mlkem.GenerateKey768()
-	k, err := ecdh.GenerateKey(ecdh.P256(), rand.Reader)
+	kDH, err := ecdh.GenerateKey(ecdh.P256(), rand.Reader)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
+	} else {
+		ecdh.ECDH(ecdh.P256(), kDH, kDH.PublicKey())
 	}
-	ecdh.ECDH(ecdh.P256(), k, k.PublicKey())
 	kDSA, err := ecdsa.GenerateKey(ecdsa.P256(), rand.Reader)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
+	} else {
+		ecdsa.SignDeterministic(ecdsa.P256(), sha256.New, kDSA, make([]byte, 32))
 	}
-	ecdsa.SignDeterministic(ecdsa.P256(), sha256.New, kDSA, make([]byte, 32))
 	k25519, err := ed25519.GenerateKey()
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
+	} else {
+		ed25519.Sign(k25519, make([]byte, 32))
 	}
-	ed25519.Sign(k25519, make([]byte, 32))
-	rsa.VerifyPKCS1v15(&rsa.PublicKey{}, "", nil, nil)
-	// Parse an RSA key to hit the PCT rather than generating one (which is slow).
-	block, _ := pem.Decode([]byte(strings.ReplaceAll(
-		`-----BEGIN RSA TESTING KEY-----
-MIIEowIBAAKCAQEAsPnoGUOnrpiSqt4XynxA+HRP7S+BSObI6qJ7fQAVSPtRkqso
-tWxQYLEYzNEx5ZSHTGypibVsJylvCfuToDTfMul8b/CZjP2Ob0LdpYrNH6l5hvFE
-89FU1nZQF15oVLOpUgA7wGiHuEVawrGfey92UE68mOyUVXGweJIVDdxqdMoPvNNU
-l86BU02vlBiESxOuox+dWmuVV7vfYZ79Toh/LUK43YvJh+rhv4nKuF7iHjVjBd9s
-B6iDjj70HFldzOQ9r8SRI+9NirupPTkF5AKNe6kUhKJ1luB7S27ZkvB3tSTT3P59
-3VVJvnzOjaA1z6Cz+4+eRvcysqhrRgFlwI9TEwIDAQABAoIBAEEYiyDP29vCzx/+
-dS3LqnI5BjUuJhXUnc6AWX/PCgVAO+8A+gZRgvct7PtZb0sM6P9ZcLrweomlGezI
-FrL0/6xQaa8bBr/ve/a8155OgcjFo6fZEw3Dz7ra5fbSiPmu4/b/kvrg+Br1l77J
-aun6uUAs1f5B9wW+vbR7tzbT/mxaUeDiBzKpe15GwcvbJtdIVMa2YErtRjc1/5B2
-BGVXyvlJv0SIlcIEMsHgnAFOp1ZgQ08aDzvilLq8XVMOahAhP1O2A3X8hKdXPyrx
-IVWE9bS9ptTo+eF6eNl+d7htpKGEZHUxinoQpWEBTv+iOoHsVunkEJ3vjLP3lyI/
-fY0NQ1ECgYEA3RBXAjgvIys2gfU3keImF8e/TprLge1I2vbWmV2j6rZCg5r/AS0u
-pii5CvJ5/T5vfJPNgPBy8B/yRDs+6PJO1GmnlhOkG9JAIPkv0RBZvR0PMBtbp6nT
-Y3yo1lwamBVBfY6rc0sLTzosZh2aGoLzrHNMQFMGaauORzBFpY5lU50CgYEAzPHl
-u5DI6Xgep1vr8QvCUuEesCOgJg8Yh1UqVoY/SmQh6MYAv1I9bLGwrb3WW/7kqIoD
-fj0aQV5buVZI2loMomtU9KY5SFIsPV+JuUpy7/+VE01ZQM5FdY8wiYCQiVZYju9X
-Wz5LxMNoz+gT7pwlLCsC4N+R8aoBk404aF1gum8CgYAJ7VTq7Zj4TFV7Soa/T1eE
-k9y8a+kdoYk3BASpCHJ29M5R2KEA7YV9wrBklHTz8VzSTFTbKHEQ5W5csAhoL5Fo
-qoHzFFi3Qx7MHESQb9qHyolHEMNx6QdsHUn7rlEnaTTyrXh3ifQtD6C0yTmFXUIS
-CW9wKApOrnyKJ9nI0HcuZQKBgQCMtoV6e9VGX4AEfpuHvAAnMYQFgeBiYTkBKltQ
-XwozhH63uMMomUmtSG87Sz1TmrXadjAhy8gsG6I0pWaN7QgBuFnzQ/HOkwTm+qKw
-AsrZt4zeXNwsH7QXHEJCFnCmqw9QzEoZTrNtHJHpNboBuVnYcoueZEJrP8OnUG3r
-UjmopwKBgAqB2KYYMUqAOvYcBnEfLDmyZv9BTVNHbR2lKkMYqv5LlvDaBxVfilE0
-2riO4p6BaAdvzXjKeRrGNEKoHNBpOSfYCOM16NjL8hIZB1CaV3WbT5oY+jp7Mzd5
-7d56RZOE+ERK2uz/7JX9VSsM/LbH9pJibd4e8mikDS9ntciqOH/3
------END RSA TESTING KEY-----`, "TESTING KEY", "PRIVATE KEY")))
-	if _, err := x509.ParsePKCS1PrivateKey(block.Bytes); err != nil {
-		t.Fatal(err)
+	kRSA, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Error(err)
+	} else {
+		rsa.SignPKCS1v15(kRSA, crypto.SHA256.String(), make([]byte, 32))
 	}
 	t.Log("completed successfully")
 }
 
+func TestCASTPasses(t *testing.T) {
+	testenv.MustHaveExec(t)
+	if err := fips140.Supported(); err != nil {
+		t.Skipf("FIPS140 not supported: %v", err)
+	}
+
+	cmd := testenv.Command(t, testenv.Executable(t), "-test.run=^TestConditionals$", "-test.v")
+	cmd.Env = append(cmd.Env, "GODEBUG=fips140=debug")
+	out, err := cmd.CombinedOutput()
+	t.Logf("%s", out)
+	if err != nil || !strings.Contains(string(out), "completed successfully") {
+		t.Errorf("TestConditionals did not complete successfully")
+	}
+
+	for _, name := range allCASTs {
+		t.Run(name, func(t *testing.T) {
+			if !strings.Contains(string(out), fmt.Sprintf("passed: %s\n", name)) {
+				t.Errorf("CAST/PCT %s success was not logged", name)
+			} else {
+				t.Logf("CAST/PCT succeeded: %s", name)
+			}
+		})
+	}
+}
+
 func TestCASTFailures(t *testing.T) {
 	testenv.MustHaveExec(t)
+	if err := fips140.Supported(); err != nil {
+		t.Skipf("FIPS140 not supported: %v", err)
+	}
 
 	for _, name := range allCASTs {
 		t.Run(name, func(t *testing.T) {
@@ -168,7 +170,6 @@ func TestCASTFailures(t *testing.T) {
 			if !testing.Verbose() {
 				t.Parallel()
 			}
-			t.Logf("CAST/PCT succeeded: %s", name)
 			t.Logf("Testing CAST/PCT failure...")
 			cmd := testenv.Command(t, testenv.Executable(t), "-test.run=TestConditionals", "-test.v")
 			cmd.Env = append(cmd.Env, fmt.Sprintf("GODEBUG=failfipscast=%s,fips140=on", name))
@@ -179,6 +180,8 @@ func TestCASTFailures(t *testing.T) {
 			}
 			if strings.Contains(string(out), "completed successfully") {
 				t.Errorf("CAST/PCT %s failure did not stop the program", name)
+			} else if !strings.Contains(string(out), "self-test failed: "+name) {
+				t.Errorf("CAST/PCT %s failure did not log the expected message", name)
 			} else {
 				t.Logf("CAST/PCT %s failed as expected and caused the program to exit", name)
 			}