From 6b3cd693416b041885613c3481f7d18a44c92410 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Thu, 14 Dec 2023 22:13:29 +0100 Subject: [PATCH] crypto/tls: align FIPS-only mode with BoringSSL policy This enables TLS 1.3, disables P-521, and disables non-ECDHE suites. Fixes #64717 Updates #62372 Change-Id: I3a65b239ef0198bbdbe5e55e0810e7128f90a091 Reviewed-on: https://go-review.googlesource.com/c/go/+/549975 Reviewed-by: Roland Shoemaker LUCI-TryBot-Result: Go LUCI Reviewed-by: Than McIntosh --- src/crypto/internal/boring/aes.go | 29 +++++++--- src/crypto/internal/boring/notboring.go | 1 + src/crypto/tls/boring.go | 26 +++++---- src/crypto/tls/boring_test.go | 69 +++++++++++++++++------- src/crypto/tls/cipher_suites.go | 8 ++- src/crypto/tls/handshake_client.go | 4 +- src/crypto/tls/handshake_client_tls13.go | 4 -- src/crypto/tls/handshake_server_test.go | 28 ++++++---- src/crypto/tls/handshake_server_tls13.go | 7 ++- src/crypto/tls/notboring.go | 2 + src/crypto/x509/boring.go | 4 +- 11 files changed, 125 insertions(+), 57 deletions(-) diff --git a/src/crypto/internal/boring/aes.go b/src/crypto/internal/boring/aes.go index 8819f576f4f4c..d18ed5cdc5c25 100644 --- a/src/crypto/internal/boring/aes.go +++ b/src/crypto/internal/boring/aes.go @@ -228,26 +228,41 @@ func (c *aesCipher) NewGCM(nonceSize, tagSize int) (cipher.AEAD, error) { if tagSize != gcmTagSize { return cipher.NewGCMWithTagSize(&noGCM{c}, tagSize) } - return c.newGCM(false) + return c.newGCM(0) } +const ( + VersionTLS12 = 0x0303 + VersionTLS13 = 0x0304 +) + func NewGCMTLS(c cipher.Block) (cipher.AEAD, error) { - return c.(*aesCipher).newGCM(true) + return c.(*aesCipher).newGCM(VersionTLS12) +} + +func NewGCMTLS13(c cipher.Block) (cipher.AEAD, error) { + return c.(*aesCipher).newGCM(VersionTLS13) } -func (c *aesCipher) newGCM(tls bool) (cipher.AEAD, error) { +func (c *aesCipher) newGCM(tlsVersion uint16) (cipher.AEAD, error) { var aead *C.GO_EVP_AEAD switch len(c.key) * 8 { case 128: - if tls { + switch tlsVersion { + case VersionTLS12: aead = C._goboringcrypto_EVP_aead_aes_128_gcm_tls12() - } else { + case VersionTLS13: + aead = C._goboringcrypto_EVP_aead_aes_128_gcm_tls13() + default: aead = C._goboringcrypto_EVP_aead_aes_128_gcm() } case 256: - if tls { + switch tlsVersion { + case VersionTLS12: aead = C._goboringcrypto_EVP_aead_aes_256_gcm_tls12() - } else { + case VersionTLS13: + aead = C._goboringcrypto_EVP_aead_aes_256_gcm_tls13() + default: aead = C._goboringcrypto_EVP_aead_aes_256_gcm() } default: diff --git a/src/crypto/internal/boring/notboring.go b/src/crypto/internal/boring/notboring.go index 361dec9672ff5..02bc468a0de8e 100644 --- a/src/crypto/internal/boring/notboring.go +++ b/src/crypto/internal/boring/notboring.go @@ -50,6 +50,7 @@ func NewHMAC(h func() hash.Hash, key []byte) hash.Hash { panic("boringcrypto: no func NewAESCipher(key []byte) (cipher.Block, error) { panic("boringcrypto: not available") } func NewGCMTLS(cipher.Block) (cipher.AEAD, error) { panic("boringcrypto: not available") } +func NewGCMTLS13(cipher.Block) (cipher.AEAD, error) { panic("boringcrypto: not available") } type PublicKeyECDSA struct{ _ int } type PrivateKeyECDSA struct{ _ int } diff --git a/src/crypto/tls/boring.go b/src/crypto/tls/boring.go index 1827f764589b5..aad96b1c74778 100644 --- a/src/crypto/tls/boring.go +++ b/src/crypto/tls/boring.go @@ -6,9 +6,10 @@ package tls -import ( - "crypto/internal/boring/fipstls" -) +import "crypto/internal/boring/fipstls" + +// The FIPS-only policies enforced here currently match BoringSSL's +// ssl_policy_fips_202205. // needFIPS returns fipstls.Required(); it avoids a new import in common.go. func needFIPS() bool { @@ -17,19 +18,19 @@ func needFIPS() bool { // fipsMinVersion replaces c.minVersion in FIPS-only mode. func fipsMinVersion(c *Config) uint16 { - // FIPS requires TLS 1.2. + // FIPS requires TLS 1.2 or TLS 1.3. return VersionTLS12 } // fipsMaxVersion replaces c.maxVersion in FIPS-only mode. func fipsMaxVersion(c *Config) uint16 { - // FIPS requires TLS 1.2. - return VersionTLS12 + // FIPS requires TLS 1.2 or TLS 1.3. + return VersionTLS13 } // default defaultFIPSCurvePreferences is the FIPS-allowed curves, // in preference order (most preferable first). -var defaultFIPSCurvePreferences = []CurveID{CurveP256, CurveP384, CurveP521} +var defaultFIPSCurvePreferences = []CurveID{CurveP256, CurveP384} // fipsCurvePreferences replaces c.curvePreferences in FIPS-only mode. func fipsCurvePreferences(c *Config) []CurveID { @@ -54,8 +55,6 @@ var defaultCipherSuitesFIPS = []uint16{ TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, - TLS_RSA_WITH_AES_128_GCM_SHA256, - TLS_RSA_WITH_AES_256_GCM_SHA384, } // fipsCipherSuites replaces c.cipherSuites in FIPS-only mode. @@ -75,8 +74,14 @@ func fipsCipherSuites(c *Config) []uint16 { return list } +// defaultCipherSuitesTLS13FIPS are the FIPS-allowed cipher suites for TLS 1.3. +var defaultCipherSuitesTLS13FIPS = []uint16{ + TLS_AES_128_GCM_SHA256, + TLS_AES_256_GCM_SHA384, +} + // fipsSupportedSignatureAlgorithms currently are a subset of -// defaultSupportedSignatureAlgorithms without Ed25519 and SHA-1. +// defaultSupportedSignatureAlgorithms without Ed25519, SHA-1, and P-521. var fipsSupportedSignatureAlgorithms = []SignatureScheme{ PSSWithSHA256, PSSWithSHA384, @@ -86,7 +91,6 @@ var fipsSupportedSignatureAlgorithms = []SignatureScheme{ PKCS1WithSHA384, ECDSAWithP384AndSHA384, PKCS1WithSHA512, - ECDSAWithP521AndSHA512, } // supportedSignatureAlgorithms returns the supported signature algorithms. diff --git a/src/crypto/tls/boring_test.go b/src/crypto/tls/boring_test.go index 085ff5713ec52..a192a657b4d79 100644 --- a/src/crypto/tls/boring_test.go +++ b/src/crypto/tls/boring_test.go @@ -25,6 +25,31 @@ import ( "time" ) +func allCipherSuitesIncludingTLS13() []uint16 { + s := allCipherSuites() + for _, suite := range cipherSuitesTLS13 { + s = append(s, suite.id) + } + return s +} + +func isTLS13CipherSuite(id uint16) bool { + for _, suite := range cipherSuitesTLS13 { + if id == suite.id { + return true + } + } + return false +} + +func generateKeyShare(group CurveID) keyShare { + key, err := generateECDHEKey(rand.Reader, group) + if err != nil { + panic(err) + } + return keyShare{group: group, data: key.PublicKey().Bytes()} +} + func TestBoringServerProtocolVersion(t *testing.T) { test := func(name string, v uint16, msg string) { t.Run(name, func(t *testing.T) { @@ -33,8 +58,11 @@ func TestBoringServerProtocolVersion(t *testing.T) { clientHello := &clientHelloMsg{ vers: v, random: make([]byte, 32), - cipherSuites: allCipherSuites(), + cipherSuites: allCipherSuitesIncludingTLS13(), compressionMethods: []uint8{compressionNone}, + supportedCurves: defaultCurvePreferences, + keyShares: []keyShare{generateKeyShare(CurveP256)}, + supportedPoints: []uint8{pointFormatUncompressed}, supportedVersions: []uint16{v}, } testClientHelloFailure(t, serverConfig, clientHello, msg) @@ -48,25 +76,25 @@ func TestBoringServerProtocolVersion(t *testing.T) { fipstls.Force() defer fipstls.Abandon() - test("VersionSSL30", VersionSSL30, "client offered only unsupported versions") - test("VersionTLS10", VersionTLS10, "client offered only unsupported versions") - test("VersionTLS11", VersionTLS11, "client offered only unsupported versions") - test("VersionTLS12", VersionTLS12, "") - test("VersionTLS13", VersionTLS13, "client offered only unsupported versions") + test("VersionSSL30/fipstls", VersionSSL30, "client offered only unsupported versions") + test("VersionTLS10/fipstls", VersionTLS10, "client offered only unsupported versions") + test("VersionTLS11/fipstls", VersionTLS11, "client offered only unsupported versions") + test("VersionTLS12/fipstls", VersionTLS12, "") + test("VersionTLS13/fipstls", VersionTLS13, "") } func isBoringVersion(v uint16) bool { - return v == VersionTLS12 + return v == VersionTLS12 || v == VersionTLS13 } func isBoringCipherSuite(id uint16) bool { switch id { - case TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + case TLS_AES_128_GCM_SHA256, + TLS_AES_256_GCM_SHA384, + TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, - TLS_RSA_WITH_AES_128_GCM_SHA256, - TLS_RSA_WITH_AES_256_GCM_SHA384: + TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384: return true } return false @@ -74,7 +102,7 @@ func isBoringCipherSuite(id uint16) bool { func isBoringCurve(id CurveID) bool { switch id { - case CurveP256, CurveP384, CurveP521: + case CurveP256, CurveP384: return true } return false @@ -86,7 +114,7 @@ func isECDSA(id uint16) bool { return suite.flags&suiteECSign == suiteECSign } } - panic(fmt.Sprintf("unknown cipher suite %#x", id)) + return false // TLS 1.3 cipher suites are not tied to the signature algorithm. } func isBoringSignatureScheme(alg SignatureScheme) bool { @@ -98,7 +126,6 @@ func isBoringSignatureScheme(alg SignatureScheme) bool { PKCS1WithSHA384, ECDSAWithP384AndSHA384, PKCS1WithSHA512, - ECDSAWithP521AndSHA512, PSSWithSHA256, PSSWithSHA384, PSSWithSHA512: @@ -109,10 +136,9 @@ func isBoringSignatureScheme(alg SignatureScheme) bool { func TestBoringServerCipherSuites(t *testing.T) { serverConfig := testConfig.Clone() - serverConfig.CipherSuites = allCipherSuites() serverConfig.Certificates = make([]Certificate, 1) - for _, id := range allCipherSuites() { + for _, id := range allCipherSuitesIncludingTLS13() { if isECDSA(id) { serverConfig.Certificates[0].Certificate = [][]byte{testECDSACertificate} serverConfig.Certificates[0].PrivateKey = testECDSAPrivateKey @@ -121,14 +147,19 @@ func TestBoringServerCipherSuites(t *testing.T) { serverConfig.Certificates[0].PrivateKey = testRSAPrivateKey } serverConfig.BuildNameToCertificate() - t.Run(fmt.Sprintf("suite=%#x", id), func(t *testing.T) { + t.Run(fmt.Sprintf("suite=%s", CipherSuiteName(id)), func(t *testing.T) { clientHello := &clientHelloMsg{ vers: VersionTLS12, random: make([]byte, 32), cipherSuites: []uint16{id}, compressionMethods: []uint8{compressionNone}, supportedCurves: defaultCurvePreferences, + keyShares: []keyShare{generateKeyShare(CurveP256)}, supportedPoints: []uint8{pointFormatUncompressed}, + supportedVersions: []uint16{VersionTLS12}, + } + if isTLS13CipherSuite(id) { + clientHello.supportedVersions = []uint16{VersionTLS13} } testClientHello(t, serverConfig, clientHello) @@ -160,7 +191,9 @@ func TestBoringServerCurves(t *testing.T) { cipherSuites: []uint16{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}, compressionMethods: []uint8{compressionNone}, supportedCurves: []CurveID{curveid}, + keyShares: []keyShare{generateKeyShare(curveid)}, supportedPoints: []uint8{pointFormatUncompressed}, + supportedVersions: []uint16{VersionTLS12}, } testClientHello(t, serverConfig, clientHello) @@ -279,7 +312,7 @@ func TestBoringClientHello(t *testing.T) { } if !isBoringVersion(hello.vers) { - t.Errorf("client vers=%#x, want %#x (TLS 1.2)", hello.vers, VersionTLS12) + t.Errorf("client vers=%#x", hello.vers) } for _, v := range hello.supportedVersions { if !isBoringVersion(v) { diff --git a/src/crypto/tls/cipher_suites.go b/src/crypto/tls/cipher_suites.go index 6f5bc37197a4f..636689beb4dce 100644 --- a/src/crypto/tls/cipher_suites.go +++ b/src/crypto/tls/cipher_suites.go @@ -556,7 +556,13 @@ func aeadAESGCMTLS13(key, nonceMask []byte) aead { if err != nil { panic(err) } - aead, err := cipher.NewGCM(aes) + var aead cipher.AEAD + if boring.Enabled { + aead, err = boring.NewGCMTLS13(aes) + } else { + boring.Unreachable() + aead, err = cipher.NewGCM(aes) + } if err != nil { panic(err) } diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go index f016e01b4b518..89004c2898962 100644 --- a/src/crypto/tls/handshake_client.go +++ b/src/crypto/tls/handshake_client.go @@ -139,7 +139,9 @@ func (c *Conn) makeClientHello() (*clientHelloMsg, *ecdh.PrivateKey, error) { if len(hello.supportedVersions) == 1 { hello.cipherSuites = nil } - if hasAESGCMHardwareSupport { + if needFIPS() { + hello.cipherSuites = append(hello.cipherSuites, defaultCipherSuitesTLS13FIPS...) + } else if hasAESGCMHardwareSupport { hello.cipherSuites = append(hello.cipherSuites, defaultCipherSuitesTLS13...) } else { hello.cipherSuites = append(hello.cipherSuites, defaultCipherSuitesTLS13NoAES...) diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go index 2f59f6888c5d8..a84cede1b0b51 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -41,10 +41,6 @@ type clientHandshakeStateTLS13 struct { func (hs *clientHandshakeStateTLS13) handshake() error { c := hs.c - if needFIPS() { - return errors.New("tls: internal error: TLS 1.3 reached in FIPS mode") - } - // The server must not select TLS 1.3 in a renegotiation. See RFC 8446, // sections 4.1.2 and 4.1.3. if c.handshakes > 0 { diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go index 15db760716c3d..c0a86a49841d6 100644 --- a/src/crypto/tls/handshake_server_test.go +++ b/src/crypto/tls/handshake_server_test.go @@ -27,6 +27,7 @@ import ( ) func testClientHello(t *testing.T, serverConfig *Config, m handshakeMessage) { + t.Helper() testClientHelloFailure(t, serverConfig, m, "") } @@ -52,23 +53,32 @@ func testClientHelloFailure(t *testing.T, serverConfig *Config, m handshakeMessa ctx := context.Background() conn := Server(s, serverConfig) ch, err := conn.readClientHello(ctx) - hs := serverHandshakeState{ - c: conn, - ctx: ctx, - clientHello: ch, - } - if err == nil { + if err == nil && conn.vers == VersionTLS13 { + hs := serverHandshakeStateTLS13{ + c: conn, + ctx: ctx, + clientHello: ch, + } err = hs.processClientHello() - } - if err == nil { - err = hs.pickCipherSuite() + } else if err == nil { + hs := serverHandshakeState{ + c: conn, + ctx: ctx, + clientHello: ch, + } + err = hs.processClientHello() + if err == nil { + err = hs.pickCipherSuite() + } } s.Close() if len(expectedSubStr) == 0 { if err != nil && err != io.EOF { + t.Helper() t.Errorf("Got error: %s; expected to succeed", err) } } else if err == nil || !strings.Contains(err.Error(), expectedSubStr) { + t.Helper() t.Errorf("Got error: %v; expected to match substring '%s'", err, expectedSubStr) } } diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index 21d798de37db0..b68ff9db4c6d4 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -45,10 +45,6 @@ type serverHandshakeStateTLS13 struct { func (hs *serverHandshakeStateTLS13) handshake() error { c := hs.c - if needFIPS() { - return errors.New("tls: internal error: TLS 1.3 reached in FIPS mode") - } - // For an overview of the TLS 1.3 handshake, see RFC 8446, Section 2. if err := hs.processClientHello(); err != nil { return err @@ -163,6 +159,9 @@ func (hs *serverHandshakeStateTLS13) processClientHello() error { if !hasAESGCMHardwareSupport || !aesgcmPreferred(hs.clientHello.cipherSuites) { preferenceList = defaultCipherSuitesTLS13NoAES } + if needFIPS() { + preferenceList = defaultCipherSuitesTLS13FIPS + } for _, suiteID := range preferenceList { hs.suite = mutualCipherSuiteTLS13(hs.clientHello.cipherSuites, suiteID) if hs.suite != nil { diff --git a/src/crypto/tls/notboring.go b/src/crypto/tls/notboring.go index 7d85b39c59319..edccb44d87a55 100644 --- a/src/crypto/tls/notboring.go +++ b/src/crypto/tls/notboring.go @@ -18,3 +18,5 @@ func fipsCurvePreferences(c *Config) []CurveID { panic("fipsCurvePreferences") } func fipsCipherSuites(c *Config) []uint16 { panic("fipsCipherSuites") } var fipsSupportedSignatureAlgorithms []SignatureScheme + +var defaultCipherSuitesTLS13FIPS []uint16 diff --git a/src/crypto/x509/boring.go b/src/crypto/x509/boring.go index 095b58c31590d..e6237e96bb3b1 100644 --- a/src/crypto/x509/boring.go +++ b/src/crypto/x509/boring.go @@ -22,7 +22,7 @@ func boringAllowCert(c *Certificate) bool { } // The key must be RSA 2048, RSA 3072, RSA 4096, - // or ECDSA P-256, P-384, P-521. + // or ECDSA P-256 or P-384. switch k := c.PublicKey.(type) { default: return false @@ -31,7 +31,7 @@ func boringAllowCert(c *Certificate) bool { return false } case *ecdsa.PublicKey: - if k.Curve != elliptic.P256() && k.Curve != elliptic.P384() && k.Curve != elliptic.P521() { + if k.Curve != elliptic.P256() && k.Curve != elliptic.P384() { return false } }