diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 2ff37d98..5dc2bda8 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -234,10 +234,24 @@ case server.AuthorizationLocalError: } ``` +**Bind the interaction handle to the browser.** `CompleteAuthorization` +accepts a handle from whoever presents it, so a login UI that carries it +in a form field and signs users in from an existing session cookie is +open to consent CSRF: a malicious client starts its own authorization, +puts its handle in a self-submitting form on its own page, and a +signed-in victim's browser approves it, sending that client a code for +the victim's account. Set the handle (`a.Handle.String()`) in an +HttpOnly, Secure, SameSite cookie when the browser reaches `/authorize`, +read it back with `server.ParseInteractionHandle` when the form is +submitted, and protect that form with your usual CSRF defence. See +`server.InteractionHandle`'s doc comment. + `cmd/conformance-as/authorize.go` is a complete, working version of -exactly this — read it for the full picture, including how it bridges -an `InteractionHandle` across the GET (render the form) and POST -(handle the submission) halves of a real HTTP flow. Its own login form +exactly this — read it for the full picture of the GET (render the +form) and POST (handle the submission) halves of a real HTTP flow. Its +handle bridge is not one to copy: it carries the handle in a hidden form +field, which is only tolerable because it authenticates nobody — use +the cookie binding above instead. Its own login form (`consent_template.go`) is a deliberate non-example: a free-text "type any username" field with no real authentication behind it, because this binary exists to drive OIDF conformance testing, not to demonstrate diff --git a/cmd/conformance-as/authorize.go b/cmd/conformance-as/authorize.go index bf2332a6..851e79b0 100644 --- a/cmd/conformance-as/authorize.go +++ b/cmd/conformance-as/authorize.go @@ -22,6 +22,13 @@ const consentPendingTTL = 5 * time.Minute // genuine HTML consent form — not an auto-approve shortcut — so a real // browser (including an automated conformance-suite browser driver) has // something to load and submit. +// +// Not a model for a production login UI: it carries the interaction +// handle in a hidden form field, with no cookie binding it to the +// browser and no CSRF protection on the POST, which a real deployment +// must have — see server.InteractionHandle's doc comment for the +// consent CSRF this would otherwise allow. It gets away with it only +// because it authenticates nobody (the form's username is free text). type consentHandler struct { srv *server.Server clients storage.ClientRepository diff --git a/server/doc.go b/server/doc.go index 6ced1b1a..0c5441f2 100644 --- a/server/doc.go +++ b/server/doc.go @@ -46,8 +46,9 @@ // rotated — FAPI 2.0 Security Profile Final §5.3.2.1 says an // authorization server "shall not use refresh token rotation except // in extraordinary circumstances" — so it stays valid for repeated -// use until it expires or is revoked, and is bound to its client -// and (under DPoP) to the key it was issued under. +// use until it expires or is revoked, and is bound to the client it +// was issued to — only that client, authenticating itself, can +// redeem it. // - AuthorizationAction (from BeginAuthorization) and AuthorizationResult // (from CompleteAuthorization) are closed sum types, not structs with // optional fields, so a caller can never mistake a local error for a @@ -75,9 +76,13 @@ // ExchangeAuthorizationCode and RefreshAccessToken each require a // valid DPoP proof bound to the token endpoint and reject a replayed // proof jti the same way they reject a replayed client assertion or -// request object, and a refresh token is bound to the DPoP key it -// was issued under: RefreshAccessToken rejects a proof from any -// other key, even one belonging to the same client. Bearer +// request object. RefreshAccessToken does not require the proof to +// come from the key the refresh token was first issued under: every +// client here is confidential and authenticates at the token +// endpoint, and RFC 9449 §5 binds a confidential client's refresh +// token to that client authentication rather than to a DPoP key. +// Each new access token is bound to the key of the proof presented +// with it. Bearer // (non-sender-constrained) tokens are not supported. // - An ID token is issued alongside the access token exactly when the // (possibly refresh-narrowed) granted scope includes "openid"; diff --git a/server/interaction.go b/server/interaction.go index 291cfbef..62384064 100644 --- a/server/interaction.go +++ b/server/interaction.go @@ -25,6 +25,22 @@ const interactionHandleSize = 32 // confused with, or substituted for, a request_uri or an authorization // code: the login UI receives only this handle, never a storage // primary key or the underlying PAR reference. +// +// Bind it to the browser that started the interaction. The handle +// identifies a pending authorization, not who may complete it, and +// CompleteAuthorization accepts it from whoever presents it. If the +// login UI round-trips it through a form field and authenticates the +// user from an existing session cookie, a malicious client can start +// its own authorization, place its handle in a form on its own page +// that submits itself, and have a victim's already-signed-in browser +// approve that authorization, so the client receives a code for the +// victim's account (consent CSRF, the authorization-server side of RFC +// 9700 §4.7's login CSRF). Keep the handle in an HttpOnly, Secure, +// SameSite cookie set when the browser reaches the authorization +// endpoint, read it back from that cookie (ParseInteractionHandle) when +// the login or consent form is submitted, and protect that form with +// the application's usual CSRF defence. Never accept the handle from +// a form field or query parameter alone. type InteractionHandle struct { value string }