From 90de3b3399bbd535f7656506bf08c867b896c1e2 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Thu, 4 Sep 2025 22:19:18 -0400 Subject: [PATCH] [release-branch.go1.25] 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). Updates #75524 Updates #74947 Updates #69536 Change-Id: I6a6a696439e1560c10f3cce2cb208fd40c5bc641 Reviewed-on: https://go-review.googlesource.com/c/go/+/706718 TryBot-Bypass: Filippo Valsorda Reviewed-by: Roland Shoemaker Reviewed-by: Junyang Shao --- src/crypto/internal/fips140/cast.go | 17 +++- src/crypto/internal/fips140/ecdh/ecdh.go | 43 +++++---- src/crypto/internal/fips140/ecdsa/cast.go | 4 +- src/crypto/internal/fips140/ecdsa/ecdsa.go | 10 +- src/crypto/internal/fips140/ed25519/cast.go | 4 +- .../internal/fips140/ed25519/ed25519.go | 15 +-- .../internal/fips140/mlkem/mlkem1024.go | 9 +- src/crypto/internal/fips140/mlkem/mlkem768.go | 9 +- src/crypto/internal/fips140/rsa/keygen.go | 23 ++++- src/crypto/internal/fips140/rsa/rsa.go | 20 ---- src/crypto/internal/fips140test/cast_test.go | 92 ++++++++++--------- 11 files changed, 111 insertions(+), 135 deletions(-) diff --git a/src/crypto/internal/fips140/cast.go b/src/crypto/internal/fips140/cast.go index 66e21d8a90..3968dcadd4 100644 --- a/src/crypto/internal/fips140/cast.go +++ b/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) + } } diff --git a/src/crypto/internal/fips140/ecdh/ecdh.go b/src/crypto/internal/fips140/ecdh/ecdh.go index bf71c75a92..967032aab2 100644 --- a/src/crypto/internal/fips140/ecdh/ecdh.go +++ b/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 } diff --git a/src/crypto/internal/fips140/ecdsa/cast.go b/src/crypto/internal/fips140/ecdsa/cast.go index 219b7211e7..6bc9fd1f46 100644 --- a/src/crypto/internal/fips140/ecdsa/cast.go +++ b/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) diff --git a/src/crypto/internal/fips140/ecdsa/ecdsa.go b/src/crypto/internal/fips140/ecdsa/ecdsa.go index 47c1b24421..81179de4f4 100644 --- a/src/crypto/internal/fips140/ecdsa/ecdsa.go +++ b/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 } diff --git a/src/crypto/internal/fips140/ed25519/cast.go b/src/crypto/internal/fips140/ed25519/cast.go index a680c2514b..2a3426bd42 100644 --- a/src/crypto/internal/fips140/ed25519/cast.go +++ b/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) }) } diff --git a/src/crypto/internal/fips140/ed25519/ed25519.go b/src/crypto/internal/fips140/ed25519/ed25519.go index bbdc5b4a8b..8beda341d9 100644 --- a/src/crypto/internal/fips140/ed25519/ed25519.go +++ b/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 } diff --git a/src/crypto/internal/fips140/mlkem/mlkem1024.go b/src/crypto/internal/fips140/mlkem/mlkem1024.go index 034bf3b5d6..1419cf20fa 100644 --- a/src/crypto/internal/fips140/mlkem/mlkem1024.go +++ b/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 } diff --git a/src/crypto/internal/fips140/mlkem/mlkem768.go b/src/crypto/internal/fips140/mlkem/mlkem768.go index 77043830d4..298660e4e9 100644 --- a/src/crypto/internal/fips140/mlkem/mlkem768.go +++ b/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 } diff --git a/src/crypto/internal/fips140/rsa/keygen.go b/src/crypto/internal/fips140/rsa/keygen.go index 7c0272239f..00b325d24b 100644 --- a/src/crypto/internal/fips140/rsa/keygen.go +++ b/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 } } diff --git a/src/crypto/internal/fips140/rsa/rsa.go b/src/crypto/internal/fips140/rsa/rsa.go index 0bbf701050..764338940a 100644 --- a/src/crypto/internal/fips140/rsa/rsa.go +++ b/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 } diff --git a/src/crypto/internal/fips140test/cast_test.go b/src/crypto/internal/fips140test/cast_test.go index 1818b583d5..14ab72d171 100644 --- a/src/crypto/internal/fips140test/cast_test.go +++ b/src/crypto/internal/fips140test/cast_test.go @@ -5,9 +5,9 @@ package fipstest import ( + "crypto" + "crypto/internal/fips140" "crypto/rand" - "crypto/x509" - "encoding/pem" "fmt" "internal/testenv" "io/fs" @@ -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,60 +105,65 @@ 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) { + moduleStatus(t) + testenv.MustHaveExec(t) + if err := fips140.Supported(); err != nil { + t.Skipf("test requires FIPS 140 mode: %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) { moduleStatus(t) testenv.MustHaveExec(t) + if err := fips140.Supported(); err != nil { + t.Skipf("test requires FIPS 140 mode: %v", err) + } for _, name := range allCASTs { t.Run(name, func(t *testing.T) { @@ -169,7 +172,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)) @@ -180,6 +182,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) } -- 2.52.0