diff --git a/UPGRADING.md b/UPGRADING.md index 4e297478..48f31430 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -159,6 +159,42 @@ entity key is also a protocol key. **What to change:** give `keys.FederationEntitySigning` its own key in your key manager. +### Sealing keys must not be text (client, server) + +**Affects:** a key passed to `client.NewTokenSetSealer`, +`client.NewBackchannelSessionSealer`, `client/sessioncookie` or +`server/interactioncookie` that is made entirely of printable text: a +passphrase, or a hex or base64 key passed without decoding it. Pairwise +keys already refused these. + +**Why:** such a key has far less entropy than 32 random bytes, so +everything sealed under it (token sets, CIBA sessions, session and +interaction cookies) is open to anyone who guesses it. + +**What fails:** the constructor returns an error saying the key is all +printable text. + +**What to change:** generate the key with `crypto/rand`, and decode an +encoded key before passing it. A random key is refused with probability +about 4×10⁻¹⁴. + +### Outbound TLS 1.2 uses only ECDHE AES-GCM suites (fapihttp) + +**Affects:** a `fapihttp.NewClient` client connecting to a server that +offers, under TLS 1.2, only cipher suites outside the four ECDHE AES-GCM +ones (CBC suites, say). TLS 1.3 servers are unaffected. So is a client +built some other way. + +**Why:** FAPI-CIBA §7.9 (as FAPI 1.0 Part 2 §8.5) permits only those +suites under TLS 1.2. + +**What fails:** the TLS handshake with such a server. `HardeningOf` also +no longer counts a transport as hardened if its TLS 1.2 suites were +changed to include any other suite, or cleared back to Go's defaults. + +**What to change:** nothing on your side; the server needs a TLS 1.2 +ECDHE AES-GCM suite, or TLS 1.3. + ### Attestation-based client authentication follows draft-12 (server, client) Not a breaking change: clients built to draft-07 keep working, and so diff --git a/client/essential_acr_test.go b/client/essential_acr_test.go index 12b0191d..6ec3f7d6 100644 --- a/client/essential_acr_test.go +++ b/client/essential_acr_test.go @@ -190,7 +190,7 @@ func assertRequestObjectAsksEssentialGoldOrSilver(t *testing.T, request string) // caller storing it between polls would. func resealBackchannelSession(t *testing.T, c *client.Client, session client.BackchannelAuthenticationSession) client.BackchannelAuthenticationSession { t.Helper() - sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("essential-acr-test-seal-key-0001")}) + sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("essential-acr-test-seal-key-000\x01")}) if err != nil { t.Fatal(err) } diff --git a/client/require_id_token_test.go b/client/require_id_token_test.go index 75fe76c0..bd40382a 100644 --- a/client/require_id_token_test.go +++ b/client/require_id_token_test.go @@ -74,7 +74,7 @@ func TestPollBackchannelAuthenticationRequiresIDTokenForOpenID(t *testing.T) { t.Fatalf("BeginBackchannelAuthentication: %v", err) } // The session survives a restart with its requirement intact. - sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("require-id-token-test-seal-key-1")}) + sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("require-id-token-test-seal-key-\x01")}) if err != nil { t.Fatal(err) } diff --git a/client/token_set_seal_internal_test.go b/client/token_set_seal_internal_test.go index 1b9506e8..8c8d7fc3 100644 --- a/client/token_set_seal_internal_test.go +++ b/client/token_set_seal_internal_test.go @@ -9,7 +9,7 @@ import ( // but isn't a sealed TokenSet's JSON — only something holding the key // could make one: Open still refuses it as unreadable. func TestTokenSetSealerOpenRefusesNonJSON(t *testing.T) { - sealer, err := NewTokenSetSealer(&Client{}, [][]byte{[]byte("token-set-internal-test-seal-key")}) + sealer, err := NewTokenSetSealer(&Client{}, [][]byte{[]byte("token-set-internal-test-seal-ke\x01")}) if err != nil { t.Fatal(err) } diff --git a/fapihttp/hardening.go b/fapihttp/hardening.go index 70444398..98aa386f 100644 --- a/fapihttp/hardening.go +++ b/fapihttp/hardening.go @@ -128,9 +128,10 @@ func registerHardened(t *http.Transport, cfg TransportConfig, dialer, verifier * // - TLSClientConfig still requires TLS 1.2 or later, verifies the // server against the RootCAs it was built with // (TransportConfig.RootCAs, or the system's), with the -// VerifyConnection it was built with (or none), and has no -// VerifyPeerCertificate, ServerName, Time, KeyLogWriter or insecure -// cipher suite set. +// VerifyConnection it was built with (or none), has no +// VerifyPeerCertificate, ServerName, Time or KeyLogWriter set, and +// offers under TLS 1.2 only the ECDHE AES-GCM cipher suites +// NewClient set (FAPI-CIBA §7.9), or some of them. // // Adding a client certificate (TransportConfig.Certificates, or setting // TLSClientConfig.Certificates afterwards) changes none of that. Nor can @@ -206,7 +207,7 @@ func tlsIntact(cfg *tls.Config, record hardenedRecord) bool { if cfg.VerifyPeerCertificate != nil || cfg.ServerName != "" || cfg.Time != nil || cfg.KeyLogWriter != nil { return false } - if cfg.RootCAs != record.rootCAs || hasInsecureCipherSuite(cfg.CipherSuites) { + if cfg.RootCAs != record.rootCAs || !onlyPermittedCipherSuites(cfg.CipherSuites) { return false } return isOwnVerifier(cfg.VerifyConnection, record) @@ -227,13 +228,17 @@ func isOwnVerifier(verify func(tls.ConnectionState) error, record hardenedRecord return errors.As(verify(tls.ConnectionState{ServerName: identityProbeServerName}), &probe) && probe.marker == record.verifier } -// hasInsecureCipherSuite reports whether suites names any of -// tls.InsecureCipherSuites. -func hasInsecureCipherSuite(suites []uint16) bool { - for _, insecure := range tls.InsecureCipherSuites() { - if slices.Contains(suites, insecure.ID) { - return true +// onlyPermittedCipherSuites reports whether suites names at least one +// TLS 1.2 cipher suite and only ones in tls12CipherSuites. An empty list +// is Go's default, which offers CBC suites too. +func onlyPermittedCipherSuites(suites []uint16) bool { + if len(suites) == 0 { + return false + } + for _, suite := range suites { + if !slices.Contains(tls12CipherSuites, suite) { + return false } } - return false + return true } diff --git a/fapihttp/hardening_review_internal_test.go b/fapihttp/hardening_review_internal_test.go index dede4e77..57158fbc 100644 --- a/fapihttp/hardening_review_internal_test.go +++ b/fapihttp/hardening_review_internal_test.go @@ -109,6 +109,10 @@ func TestHardeningOfRefusesTLSWeakenings(t *testing.T) { "fixed clock": func(c *tls.Config) { c.Time = func() time.Time { return time.Unix(0, 0) } }, "fixed server name": func(c *tls.Config) { c.ServerName = "example.com" }, "insecure cipher suite": func(c *tls.Config) { c.CipherSuites = []uint16{tls.InsecureCipherSuites()[0].ID} }, + "CBC cipher suite added": func(c *tls.Config) { + c.CipherSuites = append(c.CipherSuites, tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA) + }, + "default cipher suites": func(c *tls.Config) { c.CipherSuites = nil }, "verify peer certificate": func(c *tls.Config) { c.VerifyPeerCertificate = func([][]byte, [][]*x509.Certificate) error { return nil } }, @@ -315,3 +319,43 @@ func TestNewClientWithoutHTTP2Child(t *testing.T) { } assertSchemesReserved(t, c) } + +// TestNewClientOffersOnlyECDHEGCMUnderTLS12: FAPI-CIBA §7.9 permits only +// the ECDHE AES-GCM cipher suites under TLS 1.2, so a server that +// offers nothing else is reached and one offering only a CBC suite +// isn't. A subset of the permitted suites still counts as hardened. +func TestNewClientOffersOnlyECDHEGCMUnderTLS12(t *testing.T) { + for suite, wantOK := range map[uint16]bool{ + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256: true, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA: false, + } { + ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {})) + ts.TLS = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: []uint16{suite}} + ts.StartTLS() + cfg := RecommendedTransportConfig() + cfg.AllowLoopbackHosts = true + cfg.RootCAs = x509.NewCertPool() + cfg.RootCAs.AddCert(ts.Certificate()) + c, err := NewClient(cfg) + if err != nil { + t.Fatal(err) + } + res, err := c.Get(ts.URL) + if err == nil { + _ = res.Body.Close() + } + if (err == nil) != wantOK { + t.Errorf("%s: Get = %v, want reached %v", tls.CipherSuiteName(suite), err, wantOK) + } + ts.Close() + } + + c, err := NewClient(RecommendedTransportConfig()) + if err != nil { + t.Fatal(err) + } + c.Transport.(*http.Transport).TLSClientConfig.CipherSuites = []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384} + if _, ok := HardeningOf(c); !ok { + t.Error("HardeningOf(a subset of the permitted suites) = false, want true") + } +} diff --git a/fapihttp/transport.go b/fapihttp/transport.go index decc22df..8238ec3d 100644 --- a/fapihttp/transport.go +++ b/fapihttp/transport.go @@ -135,6 +135,7 @@ func NewClient(cfg TransportConfig) (*http.Client, error) { TLSHandshakeTimeout: cfg.TLSHandshakeTimeout, TLSClientConfig: &tls.Config{ MinVersion: tls.VersionTLS12, + CipherSuites: slices.Clone(tls12CipherSuites), VerifyConnection: verifyConnection, RootCAs: cfg.RootCAs, Certificates: cfg.Certificates, @@ -404,3 +405,15 @@ func isZero(b []byte) bool { } return true } + +// tls12CipherSuites are the only TLS 1.2 cipher suites NewClient's +// transport offers: the ECDHE AES-GCM ones FAPI-CIBA §7.9 (as FAPI 1.0 +// Part 2 §8.5) permits under TLS 1.2. Go has no DHE suites, the other +// two that list names. TLS 1.3, whose suites Go doesn't let a +// configuration choose, is unaffected. +var tls12CipherSuites = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, +} diff --git a/internal/sealkeys/sealkeys.go b/internal/sealkeys/sealkeys.go index 271447bd..6511b00c 100644 --- a/internal/sealkeys/sealkeys.go +++ b/internal/sealkeys/sealkeys.go @@ -15,9 +15,12 @@ const Size = 32 // Check refuses keys unless it holds at least one key, each Size bytes, // none of them a single byte repeated (all zeros, say: a placeholder, -// not a random key), and no key twice. what prefixes the errors. It -// can't tell a random key from any other: it only catches the keys -// that are certainly not random. +// not a random key) or made entirely of printable text (a passphrase, +// or a hex or base64 key not yet decoded), and no key twice. what +// prefixes the errors. It can't tell a random key from any other: it +// only catches the keys that are certainly not random. 32 random bytes +// are all printable with probability (98/256)^32, about 4e-14, so a +// real key is never refused. func Check(what string, keys [][]byte) error { if len(keys) == 0 { return fmt.Errorf("%s: at least one key is required", what) @@ -29,6 +32,9 @@ func Check(what string, keys [][]byte) error { if bytes.Count(k, k[:1]) == Size { return fmt.Errorf("%s: key %d is one byte repeated, not a random key", what, i) } + if isText(k) { + return fmt.Errorf("%s: key %d is all printable text (a passphrase, or a hex or base64 key not yet decoded), not %d random bytes: generate it with crypto/rand, and decode an encoded key before passing it", what, i, Size) + } for j := range i { if bytes.Equal(keys[j], k) { return fmt.Errorf("%s: key %d repeats key %d", what, i, j) @@ -37,3 +43,14 @@ func Check(what string, keys [][]byte) error { } return nil } + +// isText reports whether k is made entirely of printable ASCII and +// whitespace. +func isText(k []byte) bool { + for _, c := range k { + if (c < 0x20 || c > 0x7e) && c != '\t' && c != '\n' && c != '\r' { + return false + } + } + return true +} diff --git a/internal/sealkeys/sealkeys_test.go b/internal/sealkeys/sealkeys_test.go index 5f1d92f7..0c10151d 100644 --- a/internal/sealkeys/sealkeys_test.go +++ b/internal/sealkeys/sealkeys_test.go @@ -30,6 +30,8 @@ func TestCheck(t *testing.T) { "all zero": {[][]byte{make([]byte, Size)}, "key 0 is one byte repeated"}, "repeated byte later": {[][]byte{key(1), bytes.Repeat([]byte{0xab}, Size)}, "key 1 is one byte repeated"}, "duplicate": {[][]byte{key(1), key(2), key(1)}, "key 2 repeats key 0"}, + "passphrase": {[][]byte{[]byte("correct horse battery staple!!!!")}, "key 0 is all printable text"}, + "undecoded hex": {[][]byte{key(1), []byte("00112233445566778899aabbccddeeff")}, "key 1 is all printable text"}, } { t.Run(name, func(t *testing.T) { err := Check("test", tc.keys) diff --git a/pairwise/hmac.go b/pairwise/hmac.go index cc4fc49d..d3917929 100644 --- a/pairwise/hmac.go +++ b/pairwise/hmac.go @@ -46,9 +46,6 @@ func NewHMAC(key []byte, opts ...keys.CustodyOption) (*HMACDeriver, error) { if err := sealkeys.Check("pairwise: HMAC", [][]byte{key}); err != nil { return nil, err } - if err := checkNotText("pairwise: HMAC", key); err != nil { - return nil, err - } return NewHMACWith(memoryHMACOps{key: append([]byte(nil), key...)}, opts...) } @@ -159,17 +156,3 @@ func writeLengthPrefixed(w io.Writer, field string) { _, _ = w.Write(length[:]) _, _ = w.Write([]byte(field)) } - -// checkNotText refuses a key made entirely of printable ASCII (and -// whitespace): a passphrase, or a hex or base64 encoding passed without -// decoding, rather than random bytes. 32 random bytes are all in that -// range with probability (98/256)^32, about 4e-14, so a real key is -// never refused. -func checkNotText(what string, key []byte) error { - for _, c := range key { - if (c < 0x20 || c > 0x7e) && c != '\t' && c != '\n' && c != '\r' { - return nil - } - } - return fmt.Errorf("%s: key is all printable text (a passphrase, or a hex or base64 key not yet decoded), not 32 random bytes: generate it with crypto/rand, and decode an encoded key before passing it", what) -} diff --git a/pairwise/reversible.go b/pairwise/reversible.go index ee717e1d..892fb9bd 100644 --- a/pairwise/reversible.go +++ b/pairwise/reversible.go @@ -151,11 +151,6 @@ func NewReversible(macKey, encKey []byte, opts ...keys.CustodyOption) (*Reversib if err := sealkeys.Check("pairwise: reversible", [][]byte{macKey, encKey}); err != nil { return nil, err } - for _, key := range [...][]byte{macKey, encKey} { - if err := checkNotText("pairwise: reversible", key); err != nil { - return nil, err - } - } return newReversible(macKey, encKey, opts...) } diff --git a/server/interactioncookie/interactioncookie_fuzz_test.go b/server/interactioncookie/interactioncookie_fuzz_test.go index 0b1f2ef6..8f2e596f 100644 --- a/server/interactioncookie/interactioncookie_fuzz_test.go +++ b/server/interactioncookie/interactioncookie_fuzz_test.go @@ -14,7 +14,7 @@ import ( // the browser: it never panics, and never fails other than with // ErrNoInteraction. func FuzzRead(f *testing.F) { - key := []byte("interactioncookie-fuzz-test-key!") + key := []byte("interactioncookie-fuzz-test-key\x01") c, err := interactioncookie.New([][]byte{key}, interactioncookie.Options{}) if err != nil { f.Fatal(err)