From 7e1af3bf741862deb91f048a92e91d3b31301f90 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Sun, 17 Nov 2024 18:38:29 +0100 Subject: [PATCH] crypto/internal/fips: handle the one possible PCT failure Since ECDSA private keys are irredeemably malleable, an application could construct one where the public key doesn't match the private key. They'd be very much on their own, but crashing the program feels a bit harsh. Add this one to the list of issues caused by exposing the ECDSA (and RSA) key values as big.Ints. For #69536 Change-Id: Iaa65c73d7145e74f860ca097fa9641448442fbf9 Reviewed-on: https://go-review.googlesource.com/c/go/+/628855 Auto-Submit: Filippo Valsorda Reviewed-by: Russ Cox Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI --- src/crypto/internal/fips/cast.go | 54 +++++++++++++++------ src/crypto/internal/fips/ecdh/ecdh.go | 6 ++- src/crypto/internal/fips/ecdsa/cast.go | 14 +++--- src/crypto/internal/fips/ecdsa/ecdsa.go | 11 ++++- src/crypto/internal/fips/mlkem/mlkem1024.go | 10 +++- src/crypto/internal/fips/mlkem/mlkem768.go | 10 +++- src/crypto/internal/fipstest/cast_test.go | 11 ++--- 7 files changed, 81 insertions(+), 35 deletions(-) diff --git a/src/crypto/internal/fips/cast.go b/src/crypto/internal/fips/cast.go index 6be104a99e..4d056de7b5 100644 --- a/src/crypto/internal/fips/cast.go +++ b/src/crypto/internal/fips/cast.go @@ -16,26 +16,24 @@ import ( //go:linkname fatal crypto/internal/fips.fatal func fatal(string) -// failfipscast is a GODEBUG key allowing simulation of a Cryptographic Algorithm -// Self-Test (CAST) failure, as required during FIPS 140-3 functional testing. -// The value is a substring of the target CAST name. +// failfipscast is a GODEBUG key allowing simulation of a CAST or PCT failure, +// as required during FIPS 140-3 functional testing. The value is the whole name +// of the target CAST or PCT. var failfipscast = godebug.Value("#failfipscast") -// CAST runs the named Cryptographic Algorithm Self-Test or Pairwise Consistency -// Test (if operated in FIPS mode) and aborts the program (stopping the module -// input/output and entering the "error state") if the self-test fails. +// CAST runs the named Cryptographic Algorithm Self-Test (if operated in FIPS +// mode) and aborts the program (stopping the module input/output and entering +// the "error state") if the self-test fails. // // CASTs are mandatory self-checks that must be performed by FIPS 140-3 modules -// before the algorithm is used. See Implementation Guidance 10.3.A. PCTs are -// mandatory for every key pair that is generated/imported, including ephemeral -// keys (which effectively doubles the cost of key establishment). See -// Implementation Guidance 10.3.A Additional Comment 1. +// before the algorithm is used. See Implementation Guidance 10.3.A. // // The name must not contain commas, colons, hashes, or equal signs. // -// When calling this function from init(), also import the calling package from -// crypto/internal/fipstest, while if calling it from key generation/importing, add -// an invocation to fipstest.TestPCTs. +// If a package p calls CAST from its init function, an import of p should also +// be added to crypto/internal/fipstest. If a package p calls CAST on the first +// use of the algorithm, an invocation of that algorithm should be added to +// fipstest.TestConditionals. func CAST(name string, f func() error) { if strings.ContainsAny(name, ",#=:") { panic("fips: invalid self-test name: " + name) @@ -45,8 +43,8 @@ func CAST(name string, f func() error) { } err := f() - if failfipscast != "" && strings.Contains(name, failfipscast) { - err = errors.New("simulated CAST/PCT failure") + if name == failfipscast { + err = errors.New("simulated CAST failure") } if err != nil { fatal("FIPS 140-3 self-test failed: " + name + ": " + err.Error()) @@ -56,3 +54,29 @@ func CAST(name string, f func() error) { println("FIPS 140-3 self-test passed:", name) } } + +// 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. +// +// PCTs are mandatory for every key pair that is generated/imported, 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 { + if strings.ContainsAny(name, ",#=:") { + panic("fips: invalid self-test name: " + name) + } + if !Enabled { + return nil + } + + err := f() + if name == failfipscast { + err = errors.New("simulated PCT failure") + } + return err +} diff --git a/src/crypto/internal/fips/ecdh/ecdh.go b/src/crypto/internal/fips/ecdh/ecdh.go index 66edc8d1f8..032f033dea 100644 --- a/src/crypto/internal/fips/ecdh/ecdh.go +++ b/src/crypto/internal/fips/ecdh/ecdh.go @@ -156,7 +156,7 @@ func checkKeyAndComputePublicKey[P point[P]](key []byte, newPoint func() P, scal // 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. - fips.CAST("ECDH PCT", func() error { + if err := fips.PCT("ECDH PCT", func() error { p1, err := newPoint().ScalarBaseMult(key) if err != nil { return err @@ -165,7 +165,9 @@ func checkKeyAndComputePublicKey[P point[P]](key []byte, newPoint func() P, scal return errors.New("crypto/ecdh: public key does not match private key") } return nil - }) + }); err != nil { + panic(err) + } return publicKey, nil } diff --git a/src/crypto/internal/fips/ecdsa/cast.go b/src/crypto/internal/fips/ecdsa/cast.go index 0fcc626f9c..280516aea7 100644 --- a/src/crypto/internal/fips/ecdsa/cast.go +++ b/src/crypto/internal/fips/ecdsa/cast.go @@ -51,12 +51,14 @@ func testHash() []byte { } func fipsPCT[P Point[P]](c *Curve[P], k *PrivateKey) error { - hash := testHash() - sig, err := Sign(c, sha512.New, k, nil, hash) - if err != nil { - return err - } - return Verify(c, &k.pub, hash, sig) + return fips.PCT("ECDSA PCT", func() error { + hash := testHash() + sig, err := Sign(c, sha512.New, k, nil, hash) + if err != nil { + return err + } + return Verify(c, &k.pub, hash, sig) + }) } var fipsSelfTest = sync.OnceFunc(func() { diff --git a/src/crypto/internal/fips/ecdsa/ecdsa.go b/src/crypto/internal/fips/ecdsa/ecdsa.go index 36f8cb3bb7..a4834307d4 100644 --- a/src/crypto/internal/fips/ecdsa/ecdsa.go +++ b/src/crypto/internal/fips/ecdsa/ecdsa.go @@ -167,7 +167,11 @@ 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)} - fips.CAST("ECDSA PCT", func() error { return fipsPCT(c, priv) }) + 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 } @@ -209,7 +213,10 @@ func GenerateKey[P Point[P]](c *Curve[P], rand io.Reader) (*PrivateKey, error) { }, d: k.Bytes(c.N), } - fips.CAST("ECDSA PCT", func() error { return fipsPCT(c, priv) }) + if err := fipsPCT(c, priv); err != nil { + // This clearly can't happen, but FIPS 140-3 mandates that we check it. + panic(err) + } return priv, nil } diff --git a/src/crypto/internal/fips/mlkem/mlkem1024.go b/src/crypto/internal/fips/mlkem/mlkem1024.go index 5ab94f99e5..30c9f3f0fb 100644 --- a/src/crypto/internal/fips/mlkem/mlkem1024.go +++ b/src/crypto/internal/fips/mlkem/mlkem1024.go @@ -91,7 +91,10 @@ func generateKey1024(dk *DecapsulationKey1024) (*DecapsulationKey1024, error) { var z [32]byte drbg.Read(z[:]) kemKeyGen1024(dk, &d, &z) - fips.CAST("ML-KEM PCT", func() error { return kemPCT1024(dk) }) + if err := fips.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) + } fips.RecordApproved() return dk, nil } @@ -119,7 +122,10 @@ func newKeyFromSeed1024(dk *DecapsulationKey1024, seed []byte) (*DecapsulationKe d := (*[32]byte)(seed[:32]) z := (*[32]byte)(seed[32:]) kemKeyGen1024(dk, d, z) - fips.CAST("ML-KEM PCT", func() error { return kemPCT1024(dk) }) + if err := fips.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) + } fips.RecordApproved() return dk, nil } diff --git a/src/crypto/internal/fips/mlkem/mlkem768.go b/src/crypto/internal/fips/mlkem/mlkem768.go index df49f51b8f..dcab3d8842 100644 --- a/src/crypto/internal/fips/mlkem/mlkem768.go +++ b/src/crypto/internal/fips/mlkem/mlkem768.go @@ -148,7 +148,10 @@ func generateKey(dk *DecapsulationKey768) (*DecapsulationKey768, error) { var z [32]byte drbg.Read(z[:]) kemKeyGen(dk, &d, &z) - fips.CAST("ML-KEM PCT", func() error { return kemPCT(dk) }) + if err := fips.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) + } fips.RecordApproved() return dk, nil } @@ -176,7 +179,10 @@ func newKeyFromSeed(dk *DecapsulationKey768, seed []byte) (*DecapsulationKey768, d := (*[32]byte)(seed[:32]) z := (*[32]byte)(seed[32:]) kemKeyGen(dk, d, z) - fips.CAST("ML-KEM PCT", func() error { return kemPCT(dk) }) + if err := fips.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) + } fips.RecordApproved() return dk, nil } diff --git a/src/crypto/internal/fipstest/cast_test.go b/src/crypto/internal/fipstest/cast_test.go index bbfe1012a0..2b1523b3a3 100644 --- a/src/crypto/internal/fipstest/cast_test.go +++ b/src/crypto/internal/fipstest/cast_test.go @@ -23,7 +23,6 @@ import ( _ "crypto/internal/fips/hmac" "crypto/internal/fips/mlkem" "crypto/internal/fips/sha256" - _ "crypto/internal/fips/sha256" _ "crypto/internal/fips/sha3" _ "crypto/internal/fips/sha512" _ "crypto/internal/fips/tls12" @@ -44,9 +43,9 @@ func findAllCASTs(t *testing.T) map[string]struct{} { fipsDir := strings.TrimSpace(string(out)) t.Logf("FIPS module directory: %s", fipsDir) - // Find all invocations of fips.CAST. + // Find all invocations of fips.CAST or fips.PCT. allCASTs := make(map[string]struct{}) - castRe := regexp.MustCompile(`fips\.CAST\("([^"]+)"`) + castRe := regexp.MustCompile(`fips\.(CAST|PCT)\("([^"]+)"`) if err := fs.WalkDir(os.DirFS(fipsDir), ".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err @@ -59,7 +58,7 @@ func findAllCASTs(t *testing.T) map[string]struct{} { return err } for _, m := range castRe.FindAllSubmatch(data, -1) { - allCASTs[string(m[1])] = struct{}{} + allCASTs[string(m[2])] = struct{}{} } return nil }); err != nil { @@ -99,11 +98,11 @@ func TestCASTFailures(t *testing.T) { if err == nil { t.Error(err) } else { - t.Logf("CAST %s failed and caused the program to exit", name) + t.Logf("CAST/PCT %s failed and caused the program to exit or the test to fail", name) t.Logf("%s", out) } if strings.Contains(string(out), "completed successfully") { - t.Errorf("CAST %s failure did not stop the program", name) + t.Errorf("CAST/PCT %s failure did not stop the program", name) } }) } -- 2.48.1