fix!: refuse text sealing keys, and offer only ECDHE AES-GCM under TLS 1.2

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

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)
 	}

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)
 			}

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)
 	}

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
 }

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")
+	}
+}

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,
+}

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
+}

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)

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)
-}

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...)
 }
 

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)