diff --git a/UPGRADING.md b/UPGRADING.md index 19469820..ebabbccb 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -97,6 +97,73 @@ example letters and digits: req.BindingMessage = "W4SCT" ``` +### `X5CAttesterChain` limits the RSA keys of `x5c` certificates (server) + +**Affects:** a server whose `Dependencies.AttesterTrust` is +`server.X5CAttesterChain`, when a wallet's attestation `x5c` carries a +certificate — the signing certificate or an intermediate — whose +public key is RSA with a modulus over 4096 bits or a public exponent +other than 65537. Anchors you configure (`TrustAnchors`, `Anchors`) +aren't limited. + +**Why:** the `x5c` certificates are the caller's, and `crypto/x509` +bounds neither an RSA modulus nor its exponent. Path building spent an +RSA operation on whatever key an unauthenticated request carried, so a +31,744-bit modulus with exponent 2^31-1 cost the server about 60 ms of +CPU per request. v0.63.0 already limited every other RSA key a caller +supplies (a DPoP `jwk`, an attestation's `cnf.jwk`, a client's JWK Set) +to 4096 bits and exponent 65537; `x5c` now follows the same rule, +checked before any signature. + +**What fails:** nothing at startup. Such an attestation is refused +with `invalid_client`. There is no option to keep the old behaviour. + +**What to change:** have the attester's CA issue its signing and +intermediate certificates for 2048-, 3072- or 4096-bit RSA keys with +exponent 65537, or for EC keys: + +```sh +openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:4096 -out attester-ca.pem +``` + +### `IssueAccessToken` and `IssueRefreshToken` refuse an edited binding or attestation expiry (server) + +**Affects:** an embedder serving its own grant +(`Config.AdditionalGrantTypes`) that changes, after the server returned +them, the `Thumbprint` of a `server.TokenBinding` from +`VerifyTokenRequestBinding` before passing it to `IssueAccessToken`, or +the `AttestationExpiresAt` of a `server.AttestedClient` from +`AuthenticateAttestedClient` before passing it to `IssueAccessToken` or +`IssueRefreshToken`. + +**Why:** both documented that only what the server verified is +accepted, but the check covered only the client the value was verified +for. An edited `Thumbprint` bound the access token to a key that never +proved possession, and an extended `AttestationExpiresAt` let a +`Client` kept from an earlier request pass the attestation expiry +check. Each now keeps its own copy of what was verified. + +**What fails:** nothing at startup. Such a call is refused with a +`server_error` `*server.Error`. + +**What to change:** pass both exactly as the server returned them, for +the current request: + +```go +attested, err := srv.AuthenticateAttestedClient(ctx, req.AttestedClientAuthentication()) +if err != nil { + return err +} +binding, err := srv.VerifyTokenRequestBinding(ctx, attested.Client, req) +if err != nil { + return err +} +result, err := srv.IssueAccessToken(ctx, server.IssueAccessTokenRequest{ + GrantType: grantType, Client: attested, Binding: binding, + Subject: subject, Scope: scope, +}) +``` + ## v0.68.0 ### `DefaultAttesterLeafPolicy` refuses more signing roles (server) @@ -171,6 +238,12 @@ anchor, including cross-signed copies: x5c := [][]byte{leaf.Raw, intermediate.Raw} ``` +*Relaxed in v0.69.0:* a configured anchor in `x5c` that isn't +self-signed is accepted when a verified chain also ends at an anchor +`x5c` doesn't carry, as `[leaf, intermediate]` does with both the root +and the intermediate configured as anchors. It's refused only when +every verified chain ends at an anchor the wallet sent. + ## v0.66.0 ### `X5CAttesterChain` refuses other roles' signing certificates (server) @@ -274,6 +347,12 @@ anchor: x5c := [][]byte{leaf.Raw, intermediate.Raw} ``` +*Relaxed in v0.69.0:* a configured anchor in `x5c` that isn't +self-signed is accepted when a verified chain also ends at an anchor +`x5c` doesn't carry, as `[leaf, intermediate]` does with both the root +and the intermediate configured as anchors. It's refused only when +every verified chain ends at an anchor the wallet sent. + ## v0.65.0 ### `Limits.MaxAccessTokenClaimsBytes` is required (server) diff --git a/docs/guides/native-wallet.md b/docs/guides/native-wallet.md index d914b199..9df5ece6 100644 --- a/docs/guides/native-wallet.md +++ b/docs/guides/native-wallet.md @@ -36,17 +36,27 @@ A native client may use a private-use scheme in reverse-domain form, written with a single slash (`com.example.wallet:/callback`), loopback `http` to `127.0.0.1` or `[::1]` (never `localhost`, RFC 8252 §8.3), matched on any port, or a claimed `https` URI, in production as well; -`NewRegisteredClient` refuses any other form. The server also +`NewRegisteredClient` refuses any other form. The attestation is the +client's authentication method, not an extra signal beside another +one: the server doesn't support the attestation-based client +authentication draft's use alongside `private_key_jwt` (draft-12 +§7.6), and refuses a request carrying both an attestation and a +`client_assertion` with `invalid_request`. The server also needs `Config.AttestationBasedClientAuthentication` and an `AttesterTrust`: `server.RegisteredAttesterKeys` with the attesters' own keys, by issuer (a `keys.AttesterKeySource` such as `keys.StaticAttesterKeys` — never the clients' keys), or -`server.X5CAttesterChain` to verify the attestation's `x5c` chain. That -`x5c` carries the signing certificate and its intermediates only, as -HAIP 1.0 requires: an attestation whose `x5c` also includes a trust -anchor (any self-signed certificate, or a certificate with a -configured anchor's subject and public key, such as a cross-signed -copy of it) is `invalid_client`. With +`server.X5CAttesterChain` to verify the attestation's `x5c` chain. +HAIP 1.0 §4.4.1 has the wallet send the signing certificate and its +intermediates only, leaving out the trust anchor. The server refuses +an `x5c` that includes any self-signed certificate, or whose every +verified chain ends at an anchor `x5c` carries itself (a certificate +with a configured anchor's subject and public key, such as a +cross-signed copy of it), with `invalid_client`. An intermediate the +server also lists as an anchor is accepted when the chain verifies on +to another anchor, since the wallet can't know which of its CAs the +server lists. Each RSA key in `x5c` must be at most 4096 bits, with +exponent 65537. With a trust list shared by several attesters, use its `AttesterIssuerBoundToAnchor` binding, so each anchor vouches only for the attesters it's bound to. Its `LeafPolicy`, by default diff --git a/internal/claimvalues/claimvalues.go b/internal/claimvalues/claimvalues.go index f0a5f319..cac3adfd 100644 --- a/internal/claimvalues/claimvalues.go +++ b/internal/claimvalues/claimvalues.go @@ -12,7 +12,11 @@ package claimvalues import ( "bytes" "encoding/json" - "reflect" + "io" + "maps" + "math/big" + "slices" + "strings" ) // Acceptable is, for a claim requested with "value", "values" or both, @@ -53,14 +57,91 @@ func FromRequest(request json.RawMessage) ([]json.RawMessage, bool) { } // Equal reports whether a and b are the same JSON value: equal once -// decoded, so "1" equals "1.0" and member order doesn't matter. Text -// that doesn't decode equals nothing. +// decoded, so "1" equals "1.0" and member order doesn't matter. Numbers +// compare exactly, by the value their text denotes, not as float64, +// which would make integers above 2^53 that differ equal. Text that +// doesn't decode equals nothing. func Equal(a, b json.RawMessage) bool { - var av, bv any - if json.Unmarshal(a, &av) != nil || json.Unmarshal(b, &bv) != nil { - return false + av, aok := decode(a) + bv, bok := decode(b) + return aok && bok && equalValues(av, bv) +} + +// decode decodes raw as one JSON value, keeping numbers as their text. +func decode(raw json.RawMessage) (any, bool) { + d := json.NewDecoder(bytes.NewReader(raw)) + d.UseNumber() + var v any + if d.Decode(&v) != nil { + return nil, false + } + // Like json.Unmarshal, refuse anything after the value. + if _, err := d.Token(); err != io.EOF { + return nil, false + } + return v, true +} + +// equalValues compares two values decode returned. +func equalValues(a, b any) bool { + switch a := a.(type) { + case json.Number: + b, ok := b.(json.Number) + return ok && equalNumbers(a, b) + case []any: + b, ok := b.([]any) + return ok && slices.EqualFunc(a, b, equalValues) + case map[string]any: + b, ok := b.(map[string]any) + return ok && maps.EqualFunc(a, b, equalValues) + default: + // string, bool or nil. + return a == b + } +} + +// equalNumbers reports whether two JSON number texts denote the same +// value, exactly: each is reduced to a sign, its significant digits and +// a decimal exponent, so "100", "1e2" and "100.0" are equal. The +// exponent is a big.Int, so neither overflow nor a huge exponent's +// expansion (as big.Rat or big.Float would compute) is possible. +func equalNumbers(a, b json.Number) bool { + an, aok := canonicalNumber(string(a)) + bn, bok := canonicalNumber(string(b)) + return aok && bok && an.negative == bn.negative && an.digits == bn.digits && an.exponent.Cmp(bn.exponent) == 0 +} + +// number is a JSON number as ±digits × 10^exponent, with digits +// free of leading and trailing zeros; zero has no digits, no sign and +// exponent 0. +type number struct { + negative bool + digits string + exponent *big.Int +} + +// canonicalNumber reduces text, a JSON number (RFC 8259 §6) as +// encoding/json validated it, to a number. +func canonicalNumber(text string) (number, bool) { + negative := strings.HasPrefix(text, "-") + text = strings.TrimPrefix(text, "-") + mantissa, exp, hasExp := strings.Cut(strings.ToLower(text), "e") + exponent := new(big.Int) + if hasExp { + if _, ok := exponent.SetString(strings.TrimPrefix(exp, "+"), 10); !ok { + return number{}, false + } + } + integer, fraction, _ := strings.Cut(mantissa, ".") + digits := integer + fraction + exponent.Sub(exponent, big.NewInt(int64(len(fraction)))) + trimmed := strings.TrimRight(digits, "0") + exponent.Add(exponent, big.NewInt(int64(len(digits)-len(trimmed)))) + digits = strings.TrimLeft(trimmed, "0") + if digits == "" { + return number{exponent: new(big.Int)}, true } - return reflect.DeepEqual(av, bv) + return number{negative: negative, digits: digits, exponent: exponent}, true } // Allows reports whether name may be released with value under a. diff --git a/internal/claimvalues/claimvalues_test.go b/internal/claimvalues/claimvalues_test.go index b60ca8f3..f2660265 100644 --- a/internal/claimvalues/claimvalues_test.go +++ b/internal/claimvalues/claimvalues_test.go @@ -38,3 +38,49 @@ func TestFromRequest(t *testing.T) { } } } + +// TestEqualNumbersExactly covers OIDC Core §5.5.1's equality comparison +// for numbers: by the value the text denotes, exactly, so integers +// above 2^53 that float64 would round together stay distinct, while +// different spellings of one value are equal. +func TestEqualNumbersExactly(t *testing.T) { + for _, tc := range []struct { + a, b string + want bool + }{ + {`9007199254740993`, `9007199254740992`, false}, + {`9007199254740993`, `9007199254740993`, true}, + {`[9007199254740993]`, `[9007199254740992]`, false}, + {`{"n":9007199254740993}`, `{"n":9007199254740992}`, false}, + {`0.1000000000000000000001`, `0.1`, false}, + {`1`, `1.0`, true}, + {`100`, `1e2`, true}, + {`100`, `1E+2`, true}, + {`0.01`, `1e-2`, true}, + {`1.50`, `15e-1`, true}, + {`-1`, `1`, false}, + {`0`, `-0.0`, true}, + {`0`, `0e5`, true}, + {`1e400`, `1e400`, true}, + {`1e400`, `1e401`, false}, + {`1e99999999999999999999`, `10e99999999999999999998`, true}, + {`1`, `"1"`, false}, + {`"A"`, `"A"`, true}, + {`{"x":[1,{"y":2}]}`, `{"x":[1.0,{"y":2e0}]}`, true}, + {`[1,2]`, `[1,2,3]`, false}, + {`null`, `null`, true}, + {`1 2`, `1`, false}, + {`1x`, `1`, false}, + } { + if got := Equal(json.RawMessage(tc.a), json.RawMessage(tc.b)); got != tc.want { + t.Errorf("Equal(%s, %s) = %v, want %v", tc.a, tc.b, got, tc.want) + } + if got := Equal(json.RawMessage(tc.b), json.RawMessage(tc.a)); got != tc.want { + t.Errorf("Equal(%s, %s) = %v, want %v", tc.b, tc.a, got, tc.want) + } + } + a := Acceptable{"account_no": {json.RawMessage(`9007199254740993`)}} + if a.Allows("account_no", json.RawMessage(`9007199254740992`)) { + t.Error("Allows released 9007199254740992 for a request of value 9007199254740993") + } +} diff --git a/internal/jose/callerkey.go b/internal/jose/callerkey.go index aa37b89a..51ece51f 100644 --- a/internal/jose/callerkey.go +++ b/internal/jose/callerkey.go @@ -57,3 +57,16 @@ func checkCallerKeyMaterial(pub crypto.PublicKey) error { } return nil } + +// ValidateCallerCertificateKey checks pub, the public key of a +// certificate a caller supplied — one in an attestation's "x5c" chain, +// say — against the key-material rules ParseJWK applies to a caller's +// key, whatever algorithm the certificate is used with: an RSA modulus +// of at most 4096 bits (maxCallerRSAModulusBits) and exponent 65537. +// Run it before the certificate's key verifies anything, path building +// included: crypto/x509 accepts any modulus size and exponents up to +// 2^31-1, so an unchecked certificate lets the caller choose how much +// CPU each signature check costs. +func ValidateCallerCertificateKey(pub crypto.PublicKey) error { + return checkCallerKeyMaterial(pub) +} diff --git a/server/attested_client.go b/server/attested_client.go index 0ce61de6..9c63a4aa 100644 --- a/server/attested_client.go +++ b/server/attested_client.go @@ -48,7 +48,9 @@ type AttestedClient struct { Client storage.RegisteredClient // AttestationExpiresAt is when the verified Client Attestation - // expires. + // expires. IssueRefreshToken and IssueAccessToken refuse a Client + // whose AttestationExpiresAt was changed since: they judge the + // attestation's expiry by their own copy of it. AttestationExpiresAt time.Time // instanceKey is the RFC 7638 thumbprint of the Client Instance Key @@ -57,8 +59,11 @@ type AttestedClient struct { // this server authenticated carries one. authenticatedID is the // client it was verified for: IssueRefreshToken refuses a Client // changed since, so the key can't be attached to another client. - instanceKey string - authenticatedID fapi.ClientID + // verifiedExpiresAt is AttestationExpiresAt as verified, which they + // compare it with and judge expiry by. + instanceKey string + authenticatedID fapi.ClientID + verifiedExpiresAt time.Time } // AuthenticateAttestedClient authenticates a client with a Client @@ -108,7 +113,7 @@ func (s *Server) AuthenticateAttestedClient(ctx context.Context, req AttestedCli if req.malformed { err = newError(ErrorInvalidRequest, 400, "the request's parameters are duplicated, too many, or too large", nil) } else if req.otherClientAuth { - err = newError(ErrorInvalidRequest, 400, "the request uses more than one client authentication method", nil) + err = newError(ErrorInvalidRequest, 400, "a client attestation cannot be combined with another client authentication method", nil) } else if err == nil && attestation == "" { err = newError(ErrorInvalidClient, 401, "client authentication is required", nil) } @@ -122,5 +127,5 @@ func (s *Server) AuthenticateAttestedClient(ctx context.Context, req AttestedCli return AttestedClient{}, err } s.audit(ctx, AuditEventAuthenticateAttestedClient, client.ID(), AuditOutcomeSuccess, "") - return AttestedClient{Client: client, AttestationExpiresAt: verified.ExpiresAt, instanceKey: verified.InstanceKey, authenticatedID: client.ID()}, nil + return AttestedClient{Client: client, AttestationExpiresAt: verified.ExpiresAt, instanceKey: verified.InstanceKey, authenticatedID: client.ID(), verifiedExpiresAt: verified.ExpiresAt}, nil } diff --git a/server/attester_chain_fuzz_test.go b/server/attester_chain_fuzz_test.go index fafef382..668cbdf1 100644 --- a/server/attester_chain_fuzz_test.go +++ b/server/attester_chain_fuzz_test.go @@ -105,7 +105,7 @@ func attesterChainRefusedOracle(chain []*x509.Certificate) bool { } // FuzzAttesterLeafPolicy checks DefaultAttesterLeafPolicy and -// checkX5CExcludesAnchors on chains of certificates whose extended key +// checkX5CReachesUnsentAnchor on chains of certificates whose extended key // usages, and shapes, come from the fuzzer. Beyond never panicking on // an empty or odd chain: // @@ -113,9 +113,9 @@ func attesterChainRefusedOracle(chain []*x509.Certificate) bool { // documented rule (attesterChainRefusedOracle) refuses; // - approvedAttesterChains keeps exactly the chains the policy // accepts, in order, and fails closed when it keeps none; -// - checkX5CExcludesAnchors refuses exactly when an x5c certificate -// below the leaf has the subject and public key of the last -// certificate of some chain. +// - checkX5CReachesUnsentAnchor refuses exactly when every chain's +// last certificate has the subject and public key of an x5c +// certificate below the leaf. func FuzzAttesterLeafPolicy(f *testing.F) { f.Add([]byte{}) f.Add([]byte{1, 0, 0, 0}) @@ -179,16 +179,16 @@ func checkX5CAnchorRule(t *testing.T, extra []*x509.Certificate, chains [][]*x50 // A certificate is identified, for path building, by its subject // and public key together. type identity struct{ subject, key string } - anchors := map[identity]bool{} - for _, c := range nonEmpty { - a := c[len(c)-1] - anchors[identity{string(a.RawSubject), string(a.RawSubjectPublicKeyInfo)}] = true + sent := map[identity]bool{} + for _, e := range extra { + sent[identity{string(e.RawSubject), string(e.RawSubjectPublicKeyInfo)}] = true } - want := slices.ContainsFunc(extra, func(e *x509.Certificate) bool { - return anchors[identity{string(e.RawSubject), string(e.RawSubjectPublicKeyInfo)}] + want := !slices.ContainsFunc(nonEmpty, func(c []*x509.Certificate) bool { + a := c[len(c)-1] + return !sent[identity{string(a.RawSubject), string(a.RawSubjectPublicKeyInfo)}] }) - if err := checkX5CExcludesAnchors(extra, nonEmpty); (err != nil) != want { - t.Fatalf("checkX5CExcludesAnchors = %v, want refusal %v", err, want) + if err := checkX5CReachesUnsentAnchor(extra, nonEmpty); (err != nil) != want { + t.Fatalf("checkX5CReachesUnsentAnchor = %v, want refusal %v", err, want) } } @@ -288,7 +288,8 @@ func newAttesterPool(f *testing.F) attesterPool { // - no self-signed certificate, and nothing that isn't a certificate, // anywhere in x5c; // - chains that each start at that certificate and end at one of the -// configured anchors, none of which x5c carries below the leaf. +// configured anchors, at least one of which x5c doesn't carry +// below the leaf. func FuzzVerifyAttesterChain(f *testing.F) { pool := newAttesterPool(f) f.Add([]byte{poolLeaf, poolIntermediate}, uint8(1<