mirror of
https://github.com/go-gitea/gitea.git
synced 2026-05-16 23:27:40 +02:00
fix(oauth): strengthen PKCE validation and refresh token replay protection (#37706)
This PR tightens several OAuth validation paths related to PKCE handling, redirect URI normalization, and refresh-token replay safety. What it changes: - switch redirect URI comparison to ASCII-only normalization for exact-match checks, avoiding Unicode case-folding surprises - harden PKCE verification by: - allowing PKCE omission only when no challenge data was stored - rejecting exchanges with a missing verifier when PKCE was used - rejecting malformed challenge state where a challenge exists without a valid method - comparing derived challenges with constant-time string matching - make refresh-token invalidation counter updates conditional on the previously observed counter value, so stale refresh state cannot be accepted after the grant changes Why: These checks close gaps where: - redirect URI comparisons could rely on broader Unicode normalization than intended - malformed or incomplete PKCE state could be treated too permissively - concurrent or stale refresh-token use could advance the same grant more than once --------- Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com> Co-authored-by: Nicolas <bircni@icloud.com>
This commit is contained in:
Lunny Xiao
GitHub
parent
33923a4d7c
commit
ae69aec295
@ -5,9 +5,8 @@ package auth
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"crypto/subtle"
|
||||
"encoding/base32"
|
||||
"encoding/base64"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net"
|
||||
@ -24,6 +23,7 @@ import (
|
||||
|
||||
uuid "github.com/google/uuid"
|
||||
"golang.org/x/crypto/bcrypt"
|
||||
"golang.org/x/oauth2"
|
||||
"xorm.io/builder"
|
||||
"xorm.io/xorm"
|
||||
)
|
||||
@ -31,7 +31,10 @@ import (
|
||||
// Authorization codes should expire within 10 minutes per https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2
|
||||
const oauth2AuthorizationCodeValidity = 10 * time.Minute
|
||||
|
||||
var ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated")
|
||||
var (
|
||||
ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated")
|
||||
ErrOAuth2GrantStaleCounter = errors.New("oauth2 grant state changed during token refresh")
|
||||
)
|
||||
|
||||
// OAuth2Application represents an OAuth2 client (RFC 6749)
|
||||
type OAuth2Application struct {
|
||||
@ -151,30 +154,40 @@ func (app *OAuth2Application) ContainsRedirectURI(redirectURI string) bool {
|
||||
// https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3
|
||||
// https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
|
||||
// https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-12#section-3.1
|
||||
contains := func(s string) bool {
|
||||
s = strings.TrimSuffix(strings.ToLower(s), "/")
|
||||
for _, u := range app.RedirectURIs {
|
||||
if strings.TrimSuffix(strings.ToLower(u), "/") == s {
|
||||
redirectCandidates := []string{redirectURI}
|
||||
if !app.ConfidentialClient {
|
||||
loopbackRedirect, ok := normalizePublicClientRedirectURI(redirectURI)
|
||||
if ok {
|
||||
redirectCandidates = append(redirectCandidates, loopbackRedirect)
|
||||
}
|
||||
}
|
||||
|
||||
for _, candidate := range redirectCandidates {
|
||||
normalizedCandidate := normalizeRedirectURIForComparison(candidate)
|
||||
for _, registeredURI := range app.RedirectURIs {
|
||||
if normalizeRedirectURIForComparison(registeredURI) == normalizedCandidate {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
if !app.ConfidentialClient {
|
||||
uri, err := url.Parse(redirectURI)
|
||||
// ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3
|
||||
if err == nil && uri.Scheme == "http" && uri.Port() != "" {
|
||||
ip := net.ParseIP(uri.Hostname())
|
||||
if ip != nil && ip.IsLoopback() {
|
||||
// strip port
|
||||
uri.Host = uri.Hostname()
|
||||
if contains(uri.String()) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func normalizeRedirectURIForComparison(redirectURI string) string {
|
||||
return strings.TrimSuffix(util.ToLowerASCII(redirectURI), "/")
|
||||
}
|
||||
|
||||
func normalizePublicClientRedirectURI(redirectURI string) (string, bool) {
|
||||
parsedURI, err := url.Parse(redirectURI)
|
||||
if err != nil || parsedURI.Scheme != "http" || parsedURI.Port() == "" {
|
||||
return "", false
|
||||
}
|
||||
return contains(redirectURI)
|
||||
if ip := net.ParseIP(parsedURI.Hostname()); ip == nil || !ip.IsLoopback() {
|
||||
return "", false
|
||||
}
|
||||
parsedURI.Host = parsedURI.Hostname()
|
||||
return parsedURI.String(), true
|
||||
}
|
||||
|
||||
// Base32 characters, but lowercased.
|
||||
@ -424,22 +437,34 @@ func (code *OAuth2AuthorizationCode) Invalidate(ctx context.Context) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (code *OAuth2AuthorizationCode) requiresCodeVerifier() bool {
|
||||
return code.CodeChallengeMethod != "" || code.CodeChallenge != ""
|
||||
}
|
||||
|
||||
func deriveCodeChallenge(method, verifier string) (string, bool) {
|
||||
switch method {
|
||||
case "S256":
|
||||
return oauth2.S256ChallengeFromVerifier(verifier), true
|
||||
case "plain":
|
||||
return verifier, true
|
||||
default:
|
||||
return "", false
|
||||
}
|
||||
}
|
||||
|
||||
// ValidateCodeChallenge validates the given verifier against the saved code challenge. This is part of the PKCE implementation.
|
||||
func (code *OAuth2AuthorizationCode) ValidateCodeChallenge(verifier string) bool {
|
||||
switch code.CodeChallengeMethod {
|
||||
case "S256":
|
||||
// base64url(SHA256(verifier)) see https://tools.ietf.org/html/rfc7636#section-4.6
|
||||
h := sha256.Sum256([]byte(verifier))
|
||||
hashedVerifier := base64.RawURLEncoding.EncodeToString(h[:])
|
||||
return hashedVerifier == code.CodeChallenge
|
||||
case "plain":
|
||||
return verifier == code.CodeChallenge
|
||||
case "":
|
||||
if !code.requiresCodeVerifier() {
|
||||
return true
|
||||
default:
|
||||
// unsupported method -> return false
|
||||
}
|
||||
if verifier == "" || code.CodeChallengeMethod == "" {
|
||||
return false
|
||||
}
|
||||
expectedChallenge, ok := deriveCodeChallenge(code.CodeChallengeMethod, verifier)
|
||||
if !ok {
|
||||
return false
|
||||
}
|
||||
return subtle.ConstantTimeCompare([]byte(expectedChallenge), []byte(code.CodeChallenge)) == 1
|
||||
}
|
||||
|
||||
// GetOAuth2AuthorizationByCode returns an authorization by its code
|
||||
@ -504,15 +529,18 @@ func (grant *OAuth2Grant) GenerateNewAuthorizationCode(ctx context.Context, redi
|
||||
|
||||
// IncreaseCounter increases the counter and updates the grant
|
||||
func (grant *OAuth2Grant) IncreaseCounter(ctx context.Context) error {
|
||||
_, err := db.GetEngine(ctx).ID(grant.ID).Incr("counter").Update(new(OAuth2Grant))
|
||||
affected, err := db.GetEngine(ctx).
|
||||
Where("id = ?", grant.ID).
|
||||
And("counter = ?", grant.Counter).
|
||||
Incr("counter").
|
||||
Update(new(OAuth2Grant))
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
updatedGrant, err := GetOAuth2GrantByID(ctx, grant.ID)
|
||||
if err != nil {
|
||||
return err
|
||||
if affected == 0 {
|
||||
return ErrOAuth2GrantStaleCounter
|
||||
}
|
||||
grant.Counter = updatedGrant.Counter
|
||||
grant.Counter++
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@ -13,6 +13,7 @@ import (
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/oauth2"
|
||||
)
|
||||
|
||||
func TestOAuth2AuthorizationCode(t *testing.T) {
|
||||
@ -116,6 +117,47 @@ func TestOAuth2Application_ContainsRedirect_Slash(t *testing.T) {
|
||||
assert.False(t, app.ContainsRedirectURI("http://127.0.0.1/other"))
|
||||
}
|
||||
|
||||
func TestOAuth2Application_ContainsRedirectURI_ASCIIOnlyNormalization(t *testing.T) {
|
||||
testCases := []struct {
|
||||
name string
|
||||
registered []string
|
||||
redirectURI string
|
||||
allowed bool
|
||||
}{
|
||||
{
|
||||
name: "exact-match",
|
||||
registered: []string{"https://signin.example.test/callback"},
|
||||
redirectURI: "https://signin.example.test/callback",
|
||||
allowed: true,
|
||||
},
|
||||
{
|
||||
name: "ascii-case-insensitive",
|
||||
registered: []string{"https://signin.example.test/callback"},
|
||||
redirectURI: "https://signIN.example.test/callback",
|
||||
allowed: true,
|
||||
},
|
||||
{
|
||||
name: "non-ascii-not-folded",
|
||||
registered: []string{"https://signin.example.test/callback"},
|
||||
redirectURI: "https://signİn.example.test/callback",
|
||||
allowed: false,
|
||||
},
|
||||
{
|
||||
name: "loopback-strips-port",
|
||||
registered: []string{"http://127.0.0.1/callback"},
|
||||
redirectURI: "http://127.0.0.1:12345/callback",
|
||||
allowed: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
app := &auth_model.OAuth2Application{RedirectURIs: tc.registered}
|
||||
assert.Equal(t, tc.allowed, app.ContainsRedirectURI(tc.redirectURI))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestOAuth2Application_ValidateClientSecret(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
app := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Application{ID: 1})
|
||||
@ -193,6 +235,16 @@ func TestOAuth2Grant_IncreaseCounter(t *testing.T) {
|
||||
unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 2})
|
||||
}
|
||||
|
||||
func TestOAuth2Grant_IncreaseCounterRejectsStaleCounter(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 1})
|
||||
stale := *grant
|
||||
|
||||
assert.NoError(t, grant.IncreaseCounter(t.Context()))
|
||||
err := stale.IncreaseCounter(t.Context())
|
||||
assert.ErrorIs(t, err, auth_model.ErrOAuth2GrantStaleCounter)
|
||||
}
|
||||
|
||||
func TestOAuth2Grant_ScopeContains(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Scope: "openid profile"})
|
||||
@ -237,35 +289,38 @@ func TestRevokeOAuth2Grant(t *testing.T) {
|
||||
//////////////////// Authorization Code
|
||||
|
||||
func TestOAuth2AuthorizationCode_ValidateCodeChallenge(t *testing.T) {
|
||||
// test plain
|
||||
code := &auth_model.OAuth2AuthorizationCode{
|
||||
CodeChallengeMethod: "plain",
|
||||
CodeChallenge: "test123",
|
||||
}
|
||||
assert.True(t, code.ValidateCodeChallenge("test123"))
|
||||
assert.False(t, code.ValidateCodeChallenge("ierwgjoergjio"))
|
||||
s256Verifier := "s256-verifier"
|
||||
s256Challenge := oauth2.S256ChallengeFromVerifier(s256Verifier)
|
||||
missingVerifierChallenge := oauth2.S256ChallengeFromVerifier("verifier-not-supplied")
|
||||
|
||||
// test S256
|
||||
code = &auth_model.OAuth2AuthorizationCode{
|
||||
CodeChallengeMethod: "S256",
|
||||
CodeChallenge: "CjvyTLSdR47G5zYenDA-eDWW4lRrO8yvjcWwbD_deOg",
|
||||
testCases := []struct {
|
||||
name string
|
||||
method string
|
||||
challenge string
|
||||
verifier string
|
||||
valid bool
|
||||
}{
|
||||
{"plain-success", "plain", "plain-secret", "plain-secret", true},
|
||||
{"plain-failure", "plain", "plain-secret", "ierwgjoergjio", false},
|
||||
{"s256-success", "S256", s256Challenge, s256Verifier, true},
|
||||
{"s256-failure", "S256", s256Challenge, "wiogjerogorewngoenrgoiuenorg", false},
|
||||
{"unsupported-method", "monkey", "foiwgjioriogeiogjerger", "foiwgjioriogeiogjerger", false},
|
||||
{"no-pkce-configured", "", "", "", true},
|
||||
{"s256-missing-verifier", "S256", missingVerifierChallenge, "", false},
|
||||
{"plain-missing-verifier", "plain", "plain-secret", "", false},
|
||||
{"missing-method-with-challenge", "", "foierjiogerogerg", "", false},
|
||||
{"missing-method-rejects-even-matching-verifier", "", "foierjiogerogerg", "foierjiogerogerg", false},
|
||||
}
|
||||
assert.True(t, code.ValidateCodeChallenge("N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt"))
|
||||
assert.False(t, code.ValidateCodeChallenge("wiogjerogorewngoenrgoiuenorg"))
|
||||
|
||||
// test unknown
|
||||
code = &auth_model.OAuth2AuthorizationCode{
|
||||
CodeChallengeMethod: "monkey",
|
||||
CodeChallenge: "foiwgjioriogeiogjerger",
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
code := &auth_model.OAuth2AuthorizationCode{
|
||||
CodeChallengeMethod: tc.method,
|
||||
CodeChallenge: tc.challenge,
|
||||
}
|
||||
assert.Equal(t, tc.valid, code.ValidateCodeChallenge(tc.verifier))
|
||||
})
|
||||
}
|
||||
assert.False(t, code.ValidateCodeChallenge("foiwgjioriogeiogjerger"))
|
||||
|
||||
// test no code challenge
|
||||
code = &auth_model.OAuth2AuthorizationCode{
|
||||
CodeChallengeMethod: "",
|
||||
CodeChallenge: "foierjiogerogerg",
|
||||
}
|
||||
assert.True(t, code.ValidateCodeChallenge(""))
|
||||
}
|
||||
|
||||
func TestOAuth2AuthorizationCode_GenerateRedirectURI(t *testing.T) {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user