From 02474498b1eb88604a1e356d9fbb780560a7d9d2 Mon Sep 17 00:00:00 2001 From: Archer Date: Thu, 2 May 2024 19:05:59 +0200 Subject: [PATCH 1/3] Prevent automatic OAuth grants for public clients (#30790) This commit forces the resource owner (user) to always approve OAuth 2.0 authorization requests if the client is public (e.g. native applications). As detailed in [RFC 6749 Section 10.2](https://www.rfc-editor.org/rfc/rfc6749.html#section-10.2), > The authorization server SHOULD NOT process repeated authorization requests automatically (without active resource owner interaction) without authenticating the client or relying on other measures to ensure that the repeated request comes from the original client and not an impersonator. With the implementation prior to this patch, attackers with access to the redirect URI (e.g., the loopback interface for `git-credential-oauth`) can get access to the user account without any user interaction if they can redirect the user to the `/login/oauth/authorize` endpoint somehow (e.g., with `xdg-open` on Linux). Fixes #25061. Co-authored-by: wxiaoguang (cherry picked from commit 5c542ca94caa3587329167cfe9e949357ca15cf1) (cherry picked from commit 1b088fade6c69e63843d1bdf402454c363b22ce2) --- routers/web/auth/oauth.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 6fa60282fe..c003b99db1 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -468,8 +468,9 @@ func AuthorizeOAuth(ctx *context.Context) { return } - // Redirect if user already granted access - if grant != nil { + // Redirect if user already granted access and the application is confidential. + // I.e. always require authorization for public clients as recommended by RFC 6749 Section 10.2 + if app.ConfidentialClient && grant != nil { code, err := grant.GenerateNewAuthorizationCode(ctx, form.RedirectURI, form.CodeChallenge, form.CodeChallengeMethod) if err != nil { handleServerError(ctx, form.State, form.RedirectURI) From 97a0d90c39c134cc6536137e1b181e3327330110 Mon Sep 17 00:00:00 2001 From: Denys Konovalov Date: Tue, 21 May 2024 18:23:49 +0200 Subject: [PATCH 2/3] use existing oauth grant for public client (#31015) Do not try to create a new authorization grant when one exists already, thus preventing a DB-related authorization issue. Fix https://github.com/go-gitea/gitea/pull/30790#issuecomment-2118812426 --------- Co-authored-by: Lunny Xiao (cherry picked from commit 9c8c9ff6d10b35de8d2d7eae0fc2646ad9bbe94a) (cherry picked from commit 07fe5a8b1347bf62b3507c87d28b95ef58d6a162) --- routers/web/auth/oauth.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index c003b99db1..baaf075cb8 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -544,15 +544,30 @@ func GrantApplicationOAuth(ctx *context.Context) { ctx.ServerError("GetOAuth2ApplicationByClientID", err) return } - grant, err := app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + grant, err := app.GetGrantByUserID(ctx, ctx.Doer.ID) if err != nil { + handleServerError(ctx, form.State, form.RedirectURI) + return + } + if grant == nil { + grant, err = app.CreateGrant(ctx, ctx.Doer.ID, form.Scope) + if err != nil { + handleAuthorizeError(ctx, AuthorizeError{ + State: form.State, + ErrorDescription: "cannot create grant for user", + ErrorCode: ErrorCodeServerError, + }, form.RedirectURI) + return + } + } else if grant.Scope != form.Scope { handleAuthorizeError(ctx, AuthorizeError{ State: form.State, - ErrorDescription: "cannot create grant for user", + ErrorDescription: "a grant exists with different scope", ErrorCode: ErrorCodeServerError, }, form.RedirectURI) return } + if len(form.Nonce) > 0 { err := grant.SetNonce(ctx, form.Nonce) if err != nil { From edf9c23d3a9d4d86f1a1393f2eb48259aee8489d Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 5 Jun 2024 17:50:15 +0200 Subject: [PATCH 3/3] test(oauth): RFC 6749 Section 10.2 conformance See: 1b088fade6c69e63843d1bdf402454c363b22ce2 Prevent automatic OAuth grants for public clients 07fe5a8b1347bf62b3507c87d28b95ef58d6a162 use existing oauth grant for public client (cherry picked from commit 592469464b74d3cd97d4c9ec0ee9dd392bb5b426) --- models/fixtures/oauth2_application.yml | 2 +- tests/integration/oauth_test.go | 61 +++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/models/fixtures/oauth2_application.yml b/models/fixtures/oauth2_application.yml index 2f38cb58b6..beae9137ad 100644 --- a/models/fixtures/oauth2_application.yml +++ b/models/fixtures/oauth2_application.yml @@ -14,7 +14,7 @@ name: "Test native app" client_id: "ce5a1322-42a7-11ed-b878-0242ac120002" client_secret: "$2a$10$UYRgUSgekzBp6hYe8pAdc.cgB4Gn06QRKsORUnIYTYQADs.YR/uvi" # bcrypt of "4MK8Na6R55smdCY0WuCCumZ6hjRPnGY5saWVRHHjJiA= - redirect_uris: '["http://127.0.0.1"]' + redirect_uris: '["b", "http://127.0.0.1"]' created_unix: 1546869730 updated_unix: 1546869730 confidential_client: false diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 1da1c6f9c0..b198e6479b 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -79,6 +79,65 @@ func TestAuthorizeShow(t *testing.T) { htmlDoc.GetCSRF() } +func TestOAuth_AuthorizeConfidentialTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // da7da3ba-9a13-4167-856f-3899de0b0138 a confidential client in models/fixtures/oauth2_application.yml + + // request authorization for the first time shows the grant page ... + authorizeURL := "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate" + req := NewRequest(t, "GET", authorizeURL) + ctx := loginUser(t, "user4") + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + // ... and the user grants the authorization + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "da7da3ba-9a13-4167-856f-3899de0b0138", + "redirect_uri": "a", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + + // request authorization the second time and the grant page is not shown again, redirection happens immediately + req = NewRequest(t, "GET", authorizeURL) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") +} + +func TestOAuth_AuthorizePublicTwice(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // ce5a1322-42a7-11ed-b878-0242ac120002 is a public client in models/fixtures/oauth2_application.yml + authorizeURL := "/login/oauth/authorize?client_id=ce5a1322-42a7-11ed-b878-0242ac120002&redirect_uri=b&response_type=code&code_challenge_method=plain&code_challenge=CODE&state=thestate" + ctx := loginUser(t, "user4") + // a public client must be authorized every time + for _, name := range []string{"First", "Second"} { + t.Run(name, func(t *testing.T) { + req := NewRequest(t, "GET", authorizeURL) + resp := ctx.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, "#authorize-app", true) + + req = NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "client_id": "ce5a1322-42a7-11ed-b878-0242ac120002", + "redirect_uri": "b", + "state": "thestate", + "granted": "true", + }) + resp = ctx.MakeRequest(t, req, http.StatusSeeOther) + assert.Contains(t, test.RedirectURL(resp), "code=") + }) + } +} + func TestAuthorizeRedirectWithExistingGrant(t *testing.T) { defer tests.PrepareTestEnv(t)() req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=https%3A%2F%2Fexample.com%2Fxyzzy&response_type=code&state=thestate") @@ -480,7 +539,7 @@ func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) { gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName)) // - // Create a user as if it had been previously been created by the GitLab + // Create a user as if it had been previously created by the GitLab // authentication source. // userGitLabUserID := "5678"