diff --git a/internal/http_handlers/oauth_callback.go b/internal/http_handlers/oauth_callback.go index c3460e44..db5482bc 100644 --- a/internal/http_handlers/oauth_callback.go +++ b/internal/http_handlers/oauth_callback.go @@ -73,11 +73,23 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { ctx.JSON(400, gin.H{"error": "invalid oauth state"}) return } - // `sessionState` is the oauth provider saved during `/oauth_login/:oauth_provider`. - // Ensure the callback route's provider matches what was originally requested. - if sessionState != provider { + // The flow's parameters are read from the store, NOT parsed out of the + // value the provider echoed back. They were validated by /oauth_login + // and never left this server, so nothing in transit can alter them. + // Fails closed on anything this server did not write — including an + // entry from a previous release, whose value was the bare provider name. + statePayload, err := unmarshalOAuthState(sessionState) + if err != nil { + log.Debug().Err(err).Msg("Failed to decode oauth state payload") + ctx.JSON(400, gin.H{"error": "invalid oauth state"}) + return + } + // Ensure the callback route's provider matches what was originally + // requested, so a code obtained at one provider cannot be redeemed at + // another. + if statePayload.Provider != provider { log.Debug(). - Str("expected_provider", sessionState). + Str("expected_provider", statePayload.Provider). Str("callback_provider", provider). Msg("OAuth provider mismatch for state") ctx.JSON(400, gin.H{"error": "invalid oauth state"}) @@ -100,26 +112,18 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { } cookie.DeleteOAuthState(ctx, h.Config.AppCookieSecure) - // contains random token, redirect url, role - sessionSplit := strings.Split(state, "___") - - if len(sessionSplit) < 4 { - log.Debug().Msg("Invalid state: expected at least 4 segments") - ctx.JSON(400, gin.H{"error": "invalid oauth state"}) - return - } // remove state from store _ = h.MemoryStoreProvider.RemoveState(state) - stateValue := sessionSplit[0] - redirectURL := sessionSplit[1] + stateValue := statePayload.State + redirectURL := statePayload.RedirectURI hostname := parsers.GetHost(ctx) if !validators.IsValidRedirectURI(redirectURL, h.Config.AllowedOrigins, hostname) { log.Debug().Msg("Invalid redirect URI in OAuth state") ctx.JSON(400, gin.H{"error": "invalid redirect uri"}) return } - inputRoles := strings.Split(sessionSplit[2], ",") - scopeString := sessionSplit[3] + inputRoles := strings.Split(statePayload.Roles, ",") + scopeString := statePayload.Scope scopes := parseScopes(scopeString) var user *schemas.User // providerEmailVerified is the provider's own assertion that the diff --git a/internal/http_handlers/oauth_login.go b/internal/http_handlers/oauth_login.go index 2c4617a9..fa7cab6b 100644 --- a/internal/http_handlers/oauth_login.go +++ b/internal/http_handlers/oauth_login.go @@ -83,8 +83,6 @@ func (h *httpProvider) OAuthLoginHandler() gin.HandlerFunc { roles = strings.Join(h.Config.DefaultRoles, ",") } - oauthStateString := state + "___" + redirectURI + "___" + roles + "___" + strings.Join(scope, " ") - provider := c.Param("oauth_provider") log := log.With().Str("provider", provider).Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(c, provider) @@ -95,11 +93,37 @@ func (h *httpProvider) OAuthLoginHandler() gin.HandlerFunc { }) return } + // The value sent to the provider is an opaque handle, not the flow's + // parameters. Nothing the caller supplied travels off this server, so + // there is no format for a caller's `state` to collide with on the way + // back — see internal/http_handlers/oauth_state.go. + oauthStateString, err := newOAuthStateHandle() + if err != nil { + log.Debug().Err(err).Msg("Error generating oauth state handle") + c.JSON(500, gin.H{ + "error": "internal server error", + }) + return + } + statePayload, err := marshalOAuthState(oauthStatePayload{ + Provider: provider, + State: state, + RedirectURI: redirectURI, + Roles: roles, + Scope: strings.Join(scope, " "), + }) + if err != nil { + log.Debug().Err(err).Msg("Error encoding oauth state") + c.JSON(500, gin.H{ + "error": "internal server error", + }) + return + } // Bind this flow to the browser that started it (RFC 9700 §4.7). Set // before the state is stored so a store failure cannot leave a usable // cookie behind. cookie.SetOAuthState(c, oauthStateString, h.Config.AppCookieSecure) - if err := h.MemoryStoreProvider.SetState(oauthStateString, provider); err != nil { + if err := h.MemoryStoreProvider.SetState(oauthStateString, statePayload); err != nil { log.Debug().Err(err).Msg("Error setting state") c.JSON(500, gin.H{ "error": "internal server error", diff --git a/internal/http_handlers/oauth_state.go b/internal/http_handlers/oauth_state.go new file mode 100644 index 00000000..139d247a --- /dev/null +++ b/internal/http_handlers/oauth_state.go @@ -0,0 +1,98 @@ +package http_handlers + +import ( + "encoding/json" + "errors" + + "github.com/authorizerdev/authorizer/internal/crypto" +) + +// The social-login `state` used to carry four values through the provider round +// trip, joined with "___" and split apart on the way back: +// +// state + "___" + redirectURI + "___" + roles + "___" + scope +// +// The FIRST of them is supplied by the caller, so a caller whose state contained +// the delimiter shifted every later field left. Sending +// `state=A___https://evil.example___admin___openid` made the callback read its +// redirect URI from the caller's segment and its ROLES from the next one — and +// the callback checks roles only against ProtectedRoles, not the allowed-roles +// list that /oauth_login enforces on the way in. The RFC 9700 browser binding +// does not help: an attacker crafting this is attacking their own signup, so the +// state is bound to their own browser. +// +// The benign face of the same bug was far more common. A caller's state is +// typically base64url, whose alphabet includes "_", so a token that merely ENDED +// in one produced "____", split a character early, and yielded a redirect URI +// with a leading underscore — a hard "invalid redirect uri" on roughly 1 in 64 +// social logins, for every provider. +// +// The fix is not a better delimiter. NOTHING the caller controls travels to the +// provider any more: the state is an opaque random handle, and the four values +// live server-side in the state store, which already held an entry per login. +// There is no format for a caller to collide with because there is nothing to +// parse on the way back. +// +// Encoding the fields instead (base64 per field, joined by a character outside +// the alphabet) would also have closed the injection, but it inflates the state +// by ~28% — and X/Twitter documents a 100-character limit on `state`, which a +// realistic redirect URI already approaches. A fixed-size handle is shorter than +// what this server sent before, so no provider limit gets closer. +// +// It is also less to leak: the redirect URI, roles and scope no longer pass +// through a third party at all. + +// oauthStateHandleBytes is the entropy behind the handle. 32 bytes is the same +// budget as the PKCE verifier and yields a 43-character base64url value — well +// inside every provider limit found, including X/Twitter's 100. +const oauthStateHandleBytes = 32 + +// errMalformedOAuthState means the store held something this server did not +// write. Callers must refuse rather than guess: a lenient parse is exactly how +// caller input became roles in the first place. +var errMalformedOAuthState = errors.New("malformed oauth state") + +// oauthStatePayload is what the handle resolves to. It never leaves this server. +type oauthStatePayload struct { + // Provider is the provider the flow was started for. The callback compares + // it against its own route parameter, so a code obtained at one provider + // cannot be redeemed at another. + Provider string `json:"provider"` + // State is the CALLER's opaque value, returned to them unchanged at the end + // of the flow. It is data here, never structure. + State string `json:"state"` + // RedirectURI, Roles and Scope were all validated by /oauth_login before + // this record was written, and nothing between here and the callback can + // alter them — they never travel to the provider. + RedirectURI string `json:"redirect_uri"` + Roles string `json:"roles"` + Scope string `json:"scope"` +} + +// newOAuthStateHandle returns the opaque value sent to the provider as `state`. +func newOAuthStateHandle() (string, error) { + return crypto.NewRandomString(oauthStateHandleBytes) +} + +// marshalOAuthState serialises the payload for the state store. +func marshalOAuthState(p oauthStatePayload) (string, error) { + b, err := json.Marshal(p) + if err != nil { + return "", err + } + return string(b), nil +} + +// unmarshalOAuthState reads a payload back, failing closed on anything this +// server did not write — including an entry left by a previous release, whose +// value was the bare provider name rather than JSON. +func unmarshalOAuthState(raw string) (oauthStatePayload, error) { + var p oauthStatePayload + if err := json.Unmarshal([]byte(raw), &p); err != nil { + return oauthStatePayload{}, errMalformedOAuthState + } + if p.Provider == "" { + return oauthStatePayload{}, errMalformedOAuthState + } + return p, nil +} diff --git a/internal/http_handlers/oauth_state_test.go b/internal/http_handlers/oauth_state_test.go new file mode 100644 index 00000000..25bdf9d5 --- /dev/null +++ b/internal/http_handlers/oauth_state_test.go @@ -0,0 +1,126 @@ +package http_handlers + +import ( + "encoding/base64" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestOAuthStateHandleCarriesNothingFromTheCaller is the regression guard for a +// field-shifting bug in the old "___"-joined state. +// +// The caller supplied the FIRST field, so any delimiter it contained moved every +// later field left — handing the callback its redirect URI and its ROLES from +// caller input. The callback validates roles only against ProtectedRoles, so an +// injected role that is merely non-protected was accepted. +// +// The property now is stronger than "the delimiter is escaped": the caller's +// value never travels to the provider at all. The handle is opaque and the +// parameters are read from the store, so each state below — every one of which +// broke the old format — cannot influence anything. +func TestOAuthStateHandleCarriesNothingFromTheCaller(t *testing.T) { + const ( + provider = "google" + redirectURI = "http://authorizer:8080/app" + roles = "user" + scope = "openid profile email" + ) + + for _, callerState := range []string{ + "UlFosaMoufBG7UKydMfQJDscQHkYwZA71kGGQgFmN", + // ~1 in 64 base64url tokens ends in "_". Merged with the old "___" it + // produced "____", split a character early, and left the redirect URI + // as "_http://..." — a hard failure on every provider. + "UlFosaMoufBG7UKydMfQJDscQHkYwZA71kGGQgFmN_", + "abc___def", + // The malicious shape: forging every field that follows. + "A___https://evil.example/steal___admin___openid", + "______", + "", + "état.测试.🔐", + "a.b.c.d.e", + } { + t.Run(callerState, func(t *testing.T) { + handle, err := newOAuthStateHandle() + require.NoError(t, err) + + // The handle is what the provider sees. It must contain nothing of + // the caller's, whatever they sent. (An empty state has no prefix to + // look for — there is nothing it could have leaked.) + if len(callerState) >= 8 { + assert.NotContains(t, handle, callerState[:8], + "the caller's value must not appear in the value sent to the provider") + } + + raw, err := marshalOAuthState(oauthStatePayload{ + Provider: provider, State: callerState, + RedirectURI: redirectURI, Roles: roles, Scope: scope, + }) + require.NoError(t, err) + + got, err := unmarshalOAuthState(raw) + require.NoError(t, err) + + assert.Equal(t, callerState, got.State, "the caller's state must round-trip unchanged") + assert.Equal(t, redirectURI, got.RedirectURI, + "the redirect URI must come from the server, never from the caller") + assert.Equal(t, roles, got.Roles, + "roles must come from the server, never from the caller") + assert.Equal(t, scope, got.Scope) + assert.Equal(t, provider, got.Provider) + }) + } +} + +// TestOAuthStateHandleIsOpaqueAndShort pins the two properties that made a +// handle preferable to encoding the fields inline. +// +// X/Twitter documents a 100-character limit on `state`, and a realistic +// redirect URI already pushed the old concatenated form past it. Encoding each +// field instead would have closed the injection but inflated the value by ~28%, +// making that worse. A fixed-size handle is shorter than what this server sent +// before, whatever the caller's state and redirect URI happen to be. +func TestOAuthStateHandleIsOpaqueAndShort(t *testing.T) { + seen := map[string]bool{} + for range 200 { + h, err := newOAuthStateHandle() + require.NoError(t, err) + + assert.LessOrEqual(t, len(h), 64, "must stay well inside the tightest provider limit (X/Twitter: 100)") + assert.False(t, seen[h], "handles must not repeat") + seen[h] = true + + // URL-safe and unreserved (RFC 3986 §2.3), so no provider has to + // percent-encode it and hand back something subtly different. + for _, r := range h { + assert.True(t, + (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || r == '-' || r == '_', + "unexpected character %q in handle", r) + } + _, err = base64.RawURLEncoding.DecodeString(h) + assert.NoError(t, err, "handle must be raw base64url") + } +} + +// TestOAuthStateRejectsForeignEntries pins that the callback fails closed on a +// store entry it did not write — including one left by the previous release, +// whose value was the bare provider name rather than JSON. Guessing at such an +// entry is precisely how caller input became privileges. +func TestOAuthStateRejectsForeignEntries(t *testing.T) { + for _, tc := range []struct{ name, raw string }{ + {"empty", ""}, + {"previous release's bare provider name", "google"}, + {"the old ___ format", "tok___http://x/app___user___openid"}, + {"json without a provider", `{"state":"x","redirect_uri":"http://x/app"}`}, + {"not json", "!!!not-json!!!"}, + {"json array", `["google","x"]`}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := unmarshalOAuthState(tc.raw) + assert.ErrorIs(t, err, errMalformedOAuthState) + }) + } +}