Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit328e696

Browse files
authored
fix: limit OAuth redirects to local paths (#14585)
- This prevents a malicious user from crafting a redirect URL to a nefarious site under their control.
1 parent2a9234e commit328e696

File tree

8 files changed

+183
-20
lines changed

8 files changed

+183
-20
lines changed

‎coderd/coderdtest/coderdtest.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1143,7 +1143,7 @@ func MustWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID)
11431143

11441144
// RequestExternalAuthCallback makes a request with the proper OAuth2 state cookie
11451145
// to the external auth callback endpoint.
1146-
funcRequestExternalAuthCallback(t testing.TB,providerIDstring,client*codersdk.Client)*http.Response {
1146+
funcRequestExternalAuthCallback(t testing.TB,providerIDstring,client*codersdk.Client,opts...func(*http.Request))*http.Response {
11471147
client.HTTPClient.CheckRedirect=func(req*http.Request,via []*http.Request)error {
11481148
returnhttp.ErrUseLastResponse
11491149
}
@@ -1160,6 +1160,9 @@ func RequestExternalAuthCallback(t testing.TB, providerID string, client *coders
11601160
Name:codersdk.SessionTokenCookie,
11611161
Value:client.SessionToken(),
11621162
})
1163+
for_,opt:=rangeopts {
1164+
opt(req)
1165+
}
11631166
res,err:=client.HTTPClient.Do(req)
11641167
require.NoError(t,err)
11651168
t.Cleanup(func() {

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,6 @@ func (f *FakeIDP) AttemptLogin(t testing.TB, client *codersdk.Client, idTokenCla
479479
// This is a niche case, but it is needed for testing ConvertLoginType.
480480
func (f*FakeIDP)LoginWithClient(t testing.TB,client*codersdk.Client,idTokenClaims jwt.MapClaims,opts...func(r*http.Request)) (*codersdk.Client,*http.Response) {
481481
t.Helper()
482-
483482
path:="/api/v2/users/oidc/callback"
484483
iff.callbackPath!="" {
485484
path=f.callbackPath
@@ -489,13 +488,23 @@ func (f *FakeIDP) LoginWithClient(t testing.TB, client *codersdk.Client, idToken
489488
f.SetRedirect(t,coderOauthURL.String())
490489

491490
cli:=f.HTTPClient(client.HTTPClient)
492-
cli.CheckRedirect=func(req*http.Request,via []*http.Request)error {
491+
redirectFn:=cli.CheckRedirect
492+
checkRedirect:=func(req*http.Request,via []*http.Request)error {
493493
// Store the idTokenClaims to the specific state request. This ties
494494
// the claims 1:1 with a given authentication flow.
495-
state:=req.URL.Query().Get("state")
496-
f.stateToIDTokenClaims.Store(state,idTokenClaims)
495+
ifstate:=req.URL.Query().Get("state");state!="" {
496+
f.stateToIDTokenClaims.Store(state,idTokenClaims)
497+
returnnil
498+
}
499+
// This is mainly intended to prevent the _last_ redirect
500+
// The one involving the state param is a core part of the
501+
// OIDC flow and shouldn't be redirected.
502+
ifredirectFn!=nil {
503+
returnredirectFn(req,via)
504+
}
497505
returnnil
498506
}
507+
cli.CheckRedirect=checkRedirect
499508

500509
req,err:=http.NewRequestWithContext(context.Background(),"GET",coderOauthURL.String(),nil)
501510
require.NoError(t,err)

‎coderd/externalauth.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"fmt"
77
"net/http"
8+
"net/url"
89

910
"github.com/sqlc-dev/pqtype"
1011
"golang.org/x/sync/errgroup"
@@ -306,6 +307,7 @@ func (api *API) externalAuthCallback(externalAuthConfig *externalauth.Config) ht
306307
// FE know not to enter the authentication loop again, and instead display an error.
307308
redirect=fmt.Sprintf("/external-auth/%s?redirected=true",externalAuthConfig.ID)
308309
}
310+
redirect=uriFromURL(redirect)
309311
http.Redirect(rw,r,redirect,http.StatusTemporaryRedirect)
310312
}
311313
}
@@ -401,3 +403,12 @@ func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvi
401403
AllowValidate:cfg.ValidateURL!="",
402404
}
403405
}
406+
407+
funcuriFromURL(ustring)string {
408+
uri,err:=url.Parse(u)
409+
iferr!=nil {
410+
return"/"
411+
}
412+
413+
returnuri.RequestURI()
414+
}

‎coderd/externalauth_test.go

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,12 @@ func TestExternalAuthManagement(t *testing.T) {
207207
constgitlabID="fake-gitlab"
208208

209209
githubCalled:=false
210-
githubApp:=oidctest.NewFakeIDP(t,oidctest.WithServing(),oidctest.WithRefresh(func(emailstring)error {
210+
githubApp:=oidctest.NewFakeIDP(t,oidctest.WithServing(),oidctest.WithRefresh(func(_string)error {
211211
githubCalled=true
212212
returnnil
213213
}))
214214
gitlabCalled:=false
215-
gitlab:=oidctest.NewFakeIDP(t,oidctest.WithServing(),oidctest.WithRefresh(func(emailstring)error {
215+
gitlab:=oidctest.NewFakeIDP(t,oidctest.WithServing(),oidctest.WithRefresh(func(_string)error {
216216
gitlabCalled=true
217217
returnnil
218218
}))
@@ -508,6 +508,35 @@ func TestExternalAuthCallback(t *testing.T) {
508508
resp=coderdtest.RequestExternalAuthCallback(t,"github",client)
509509
require.Equal(t,http.StatusTemporaryRedirect,resp.StatusCode)
510510
})
511+
512+
t.Run("CustomRedirect",func(t*testing.T) {
513+
t.Parallel()
514+
client:=coderdtest.New(t,&coderdtest.Options{
515+
IncludeProvisionerDaemon:true,
516+
ExternalAuthConfigs: []*externalauth.Config{{
517+
InstrumentedOAuth2Config:&testutil.OAuth2Config{},
518+
ID:"github",
519+
Regex:regexp.MustCompile(`github\.com`),
520+
Type:codersdk.EnhancedExternalAuthProviderGitHub.String(),
521+
}},
522+
})
523+
maliciousHost:="https://malicious.com"
524+
expectedURI:="/some/path?param=1"
525+
_=coderdtest.CreateFirstUser(t,client)
526+
resp:=coderdtest.RequestExternalAuthCallback(t,"github",client,func(req*http.Request) {
527+
req.AddCookie(&http.Cookie{
528+
Name:codersdk.OAuth2RedirectCookie,
529+
Value:maliciousHost+expectedURI,
530+
})
531+
})
532+
require.Equal(t,http.StatusTemporaryRedirect,resp.StatusCode)
533+
location,err:=resp.Location()
534+
require.NoError(t,err)
535+
require.Equal(t,expectedURI,location.RequestURI())
536+
require.Equal(t,client.URL.Host,location.Host)
537+
require.NotContains(t,location.String(),maliciousHost)
538+
})
539+
511540
t.Run("ValidateURL",func(t*testing.T) {
512541
t.Parallel()
513542
ctx:=testutil.Context(t,testutil.WaitLong)

‎coderd/httpmw/oauth2.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7+
"net/url"
78
"reflect"
89

910
"github.com/go-chi/chi/v5"
@@ -85,6 +86,15 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
8586

8687
code:=r.URL.Query().Get("code")
8788
state:=r.URL.Query().Get("state")
89+
redirect:=r.URL.Query().Get("redirect")
90+
ifredirect!="" {
91+
// We want to ensure that we're only ever redirecting to the application.
92+
// We could be more strict here and check to see if the host matches
93+
// the host of the AccessURL but ultimately as long as our redirect
94+
// url omits a host we're ensuring that we're routing to a path
95+
// local to the application.
96+
redirect=uriFromURL(redirect)
97+
}
8898

8999
ifcode=="" {
90100
// If the code isn't provided, we'll redirect!
@@ -119,7 +129,7 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
119129
// an old redirect could apply!
120130
http.SetCookie(rw,&http.Cookie{
121131
Name:codersdk.OAuth2RedirectCookie,
122-
Value:r.URL.Query().Get("redirect"),
132+
Value:redirect,
123133
Path:"/",
124134
HttpOnly:true,
125135
SameSite:http.SameSiteLaxMode,
@@ -150,7 +160,6 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, authURLOp
150160
return
151161
}
152162

153-
varredirectstring
154163
stateRedirect,err:=r.Cookie(codersdk.OAuth2RedirectCookie)
155164
iferr==nil {
156165
redirect=stateRedirect.Value
@@ -302,3 +311,12 @@ func ExtractOAuth2ProviderAppSecret(db database.Store) func(http.Handler) http.H
302311
})
303312
}
304313
}
314+
315+
funcuriFromURL(ustring)string {
316+
uri,err:=url.Parse(u)
317+
iferr!=nil {
318+
return"/"
319+
}
320+
321+
returnuri.RequestURI()
322+
}

‎coderd/httpmw/oauth2_test.go

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,31 @@ func TestOAuth2(t *testing.T) {
6767
cookie:=res.Result().Cookies()[1]
6868
require.Equal(t,"/dashboard",cookie.Value)
6969
})
70+
t.Run("OnlyPathBaseRedirect",func(t*testing.T) {
71+
t.Parallel()
72+
// Construct a URI to a potentially malicious
73+
// site and assert that we omit the host
74+
// when redirecting the request.
75+
uri:=&url.URL{
76+
Scheme:"https",
77+
Host:"some.bad.domain.com",
78+
Path:"/sadf/asdfasdf",
79+
RawQuery:"foo=hello&bar=world",
80+
}
81+
expectedValue:=uri.Path+"?"+uri.RawQuery
82+
req:=httptest.NewRequest("GET","/?redirect="+url.QueryEscape(uri.String()),nil)
83+
res:=httptest.NewRecorder()
84+
tp:=newTestOAuth2Provider(t,oauth2.AccessTypeOffline)
85+
httpmw.ExtractOAuth2(tp,nil,nil)(nil).ServeHTTP(res,req)
86+
location:=res.Header().Get("Location")
87+
if!assert.NotEmpty(t,location) {
88+
return
89+
}
90+
require.Len(t,res.Result().Cookies(),2)
91+
cookie:=res.Result().Cookies()[1]
92+
require.Equal(t,expectedValue,cookie.Value)
93+
})
94+
7095
t.Run("NoState",func(t*testing.T) {
7196
t.Parallel()
7297
req:=httptest.NewRequest("GET","/?code=something",nil)
@@ -108,7 +133,7 @@ func TestOAuth2(t *testing.T) {
108133
})
109134
res:=httptest.NewRecorder()
110135
tp:=newTestOAuth2Provider(t,oauth2.AccessTypeOffline)
111-
httpmw.ExtractOAuth2(tp,nil,nil)(http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
136+
httpmw.ExtractOAuth2(tp,nil,nil)(http.HandlerFunc(func(_ http.ResponseWriter,r*http.Request) {
112137
state:=httpmw.OAuth2(r)
113138
require.Equal(t,"/dashboard",state.Redirect)
114139
})).ServeHTTP(res,req)

‎coderd/userauth.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -707,9 +707,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
707707
http.SetCookie(rw,cookie)
708708
}
709709

710-
ifredirect=="" {
711-
redirect="/"
712-
}
710+
redirect=uriFromURL(redirect)
713711
http.Redirect(rw,r,redirect,http.StatusTemporaryRedirect)
714712
}
715713

@@ -1085,9 +1083,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10851083
}
10861084

10871085
redirect:=state.Redirect
1088-
ifredirect=="" {
1089-
redirect="/"
1090-
}
1086+
// Strip the hostifit exists on the URL to prevent
1087+
// any nefarious redirects.
1088+
redirect=uriFromURL(redirect)
10911089
http.Redirect(rw,r,redirect,http.StatusTemporaryRedirect)
10921090
}
10931091

@@ -1687,7 +1685,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data
16871685
}
16881686
}
16891687
varclaimsOAuthConvertStateClaims
1690-
token,err:=jwt.ParseWithClaims(jwtCookie.Value,&claims,func(token*jwt.Token) (interface{},error) {
1688+
token,err:=jwt.ParseWithClaims(jwtCookie.Value,&claims,func(_*jwt.Token) (interface{},error) {
16911689
returnapi.OAuthSigningKey[:],nil
16921690
})
16931691
ifxerrors.Is(err,jwt.ErrSignatureInvalid)||!token.Valid {

‎coderd/userauth_test.go

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,11 +354,25 @@ func TestUserOAuth2Github(t *testing.T) {
354354
})
355355
numLogs:=len(auditor.AuditLogs())
356356

357-
resp:=oauth2Callback(t,client)
357+
// Validate that attempting to redirect away from the
358+
// site does not work.
359+
maliciousHost:="https://malicious.com"
360+
expectedPath:="/my/path"
361+
resp:=oauth2Callback(t,client,func(req*http.Request) {
362+
// Add the cookie to bypass the parsing in httpmw/oauth2.go
363+
req.AddCookie(&http.Cookie{
364+
Name:codersdk.OAuth2RedirectCookie,
365+
Value:maliciousHost+expectedPath,
366+
})
367+
})
358368
numLogs++// add an audit log for login
359369

360370
require.Equal(t,http.StatusTemporaryRedirect,resp.StatusCode)
361-
371+
redirect,err:=resp.Location()
372+
require.NoError(t,err)
373+
require.Equal(t,expectedPath,redirect.Path)
374+
require.Equal(t,client.URL.Host,redirect.Host)
375+
require.NotContains(t,redirect.String(),maliciousHost)
362376
client.SetSessionToken(authCookieValue(resp.Cookies()))
363377
user,err:=client.User(context.Background(),"me")
364378
require.NoError(t,err)
@@ -1436,6 +1450,59 @@ func TestUserOIDC(t *testing.T) {
14361450
_,resp:=fake.AttemptLogin(t,client, jwt.MapClaims{})
14371451
require.Equal(t,http.StatusBadRequest,resp.StatusCode)
14381452
})
1453+
1454+
t.Run("StripRedirectHost",func(t*testing.T) {
1455+
t.Parallel()
1456+
ctx:=testutil.Context(t,testutil.WaitShort)
1457+
1458+
expectedRedirect:="/foo/bar?hello=world&bar=baz"
1459+
redirectURL:="https://malicious"+expectedRedirect
1460+
1461+
callbackPath:=fmt.Sprintf("/api/v2/users/oidc/callback?redirect=%s",url.QueryEscape(redirectURL))
1462+
fake:=oidctest.NewFakeIDP(t,
1463+
oidctest.WithRefresh(func(_string)error {
1464+
returnxerrors.New("refreshing token should never occur")
1465+
}),
1466+
oidctest.WithServing(),
1467+
oidctest.WithCallbackPath(callbackPath),
1468+
)
1469+
cfg:=fake.OIDCConfig(t,nil,func(cfg*coderd.OIDCConfig) {
1470+
cfg.AllowSignups=true
1471+
})
1472+
1473+
client:=coderdtest.New(t,&coderdtest.Options{
1474+
OIDCConfig:cfg,
1475+
})
1476+
1477+
client.HTTPClient.Transport=http.DefaultTransport
1478+
1479+
client.HTTPClient.CheckRedirect=func(*http.Request, []*http.Request)error {
1480+
returnhttp.ErrUseLastResponse
1481+
}
1482+
1483+
claims:= jwt.MapClaims{
1484+
"email":"user@example.com",
1485+
"email_verified":true,
1486+
}
1487+
1488+
// Perform the login
1489+
loginClient,resp:=fake.LoginWithClient(t,client,claims)
1490+
require.Equal(t,http.StatusTemporaryRedirect,resp.StatusCode)
1491+
1492+
// Get the location from the response
1493+
location,err:=resp.Location()
1494+
require.NoError(t,err)
1495+
1496+
// Check that the redirect URL has been stripped of its malicious host
1497+
require.Equal(t,expectedRedirect,location.RequestURI())
1498+
require.Equal(t,client.URL.Host,location.Host)
1499+
require.NotContains(t,location.String(),"malicious")
1500+
1501+
// Verify the user was created
1502+
user,err:=loginClient.User(ctx,"me")
1503+
require.NoError(t,err)
1504+
require.Equal(t,"user@example.com",user.Email)
1505+
})
14391506
}
14401507

14411508
funcTestUserLogout(t*testing.T) {
@@ -1587,7 +1654,7 @@ func TestOIDCSkipIssuer(t *testing.T) {
15871654
require.Equal(t,found.LoginType,codersdk.LoginTypeOIDC)
15881655
}
15891656

1590-
funcoauth2Callback(t*testing.T,client*codersdk.Client)*http.Response {
1657+
funcoauth2Callback(t*testing.T,client*codersdk.Client,opts...func(*http.Request))*http.Response {
15911658
client.HTTPClient.CheckRedirect=func(req*http.Request,via []*http.Request)error {
15921659
returnhttp.ErrUseLastResponse
15931660
}
@@ -1597,6 +1664,9 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
15971664
require.NoError(t,err)
15981665
req,err:=http.NewRequestWithContext(context.Background(),"GET",oauthURL.String(),nil)
15991666
require.NoError(t,err)
1667+
for_,opt:=rangeopts {
1668+
opt(req)
1669+
}
16001670
req.AddCookie(&http.Cookie{
16011671
Name:codersdk.OAuth2StateCookie,
16021672
Value:state,

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp