From 9c00104cb2f0e56666951d22e31ab5c344de97f2 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sun, 7 Aug 2022 21:13:56 +1000 Subject: [PATCH] fix(utils): domain suffix improperly checked (#3799) --- internal/handlers/handler_logout.go | 2 +- internal/handlers/handler_sign_duo_test.go | 8 ++-- internal/handlers/handler_sign_totp_test.go | 6 +-- internal/handlers/response.go | 2 +- internal/mocks/authelia_ctx.go | 19 ++++----- internal/utils/const.go | 5 +++ internal/utils/safe_redirection.go | 31 --------------- internal/utils/safe_redirection_test.go | 42 -------------------- internal/utils/url.go | 30 ++++++++++++++ internal/utils/url_test.go | 43 +++++++++++++++++++++ 10 files changed, 97 insertions(+), 91 deletions(-) delete mode 100644 internal/utils/safe_redirection.go delete mode 100644 internal/utils/safe_redirection_test.go diff --git a/internal/handlers/handler_logout.go b/internal/handlers/handler_logout.go index b00f1d72a..45ae564ba 100644 --- a/internal/handlers/handler_logout.go +++ b/internal/handlers/handler_logout.go @@ -33,7 +33,7 @@ func LogoutPOST(ctx *middlewares.AutheliaCtx) { redirectionURL, err := url.Parse(body.TargetURL) if err == nil { - responseBody.SafeTargetURL = utils.IsRedirectionSafe(*redirectionURL, ctx.Configuration.Session.Domain) + responseBody.SafeTargetURL = utils.URLDomainHasSuffix(*redirectionURL, ctx.Configuration.Session.Domain) } if body.TargetURL != "" { diff --git a/internal/handlers/handler_sign_duo_test.go b/internal/handlers/handler_sign_duo_test.go index 8cae6ce6f..201bb521f 100644 --- a/internal/handlers/handler_sign_duo_test.go +++ b/internal/handlers/handler_sign_duo_test.go @@ -614,14 +614,14 @@ func (s *SecondFactorDuoPostSuite) TestShouldRedirectUserToSafeTargetURL() { duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil) bodyBytes, err := json.Marshal(signDuoRequestBody{ - TargetURL: "https://mydomain.local", + TargetURL: "https://example.com", }) s.Require().NoError(err) s.mock.Ctx.Request.SetBody(bodyBytes) DuoPOST(duoMock)(s.mock.Ctx) s.mock.Assert200OK(s.T(), redirectResponse{ - Redirect: "https://mydomain.local", + Redirect: "https://example.com", }) } @@ -663,7 +663,7 @@ func (s *SecondFactorDuoPostSuite) TestShouldNotRedirectToUnsafeURL() { duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil) bodyBytes, err := json.Marshal(signDuoRequestBody{ - TargetURL: "http://mydomain.local", + TargetURL: "http://example.com", }) s.Require().NoError(err) s.mock.Ctx.Request.SetBody(bodyBytes) @@ -710,7 +710,7 @@ func (s *SecondFactorDuoPostSuite) TestShouldRegenerateSessionForPreventingSessi duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil) bodyBytes, err := json.Marshal(signDuoRequestBody{ - TargetURL: "http://mydomain.local", + TargetURL: "http://example.com", }) s.Require().NoError(err) s.mock.Ctx.Request.SetBody(bodyBytes) diff --git a/internal/handlers/handler_sign_totp_test.go b/internal/handlers/handler_sign_totp_test.go index 762434ac0..e5e38e9f3 100644 --- a/internal/handlers/handler_sign_totp_test.go +++ b/internal/handlers/handler_sign_totp_test.go @@ -167,7 +167,7 @@ func (s *HandlerSignTOTPSuite) TestShouldRedirectUserToSafeTargetURL() { bodyBytes, err := json.Marshal(signTOTPRequestBody{ Token: "abc", - TargetURL: "https://mydomain.local", + TargetURL: "https://example.com", }) s.Require().NoError(err) @@ -175,7 +175,7 @@ func (s *HandlerSignTOTPSuite) TestShouldRedirectUserToSafeTargetURL() { TimeBasedOneTimePasswordPOST(s.mock.Ctx) s.mock.Assert200OK(s.T(), redirectResponse{ - Redirect: "https://mydomain.local", + Redirect: "https://example.com", }) } @@ -205,7 +205,7 @@ func (s *HandlerSignTOTPSuite) TestShouldNotRedirectToUnsafeURL() { bodyBytes, err := json.Marshal(signTOTPRequestBody{ Token: "abc", - TargetURL: "http://mydomain.local", + TargetURL: "http://example.com", }) s.Require().NoError(err) diff --git a/internal/handlers/response.go b/internal/handlers/response.go index c8fb3007e..2cd07ddbb 100644 --- a/internal/handlers/response.go +++ b/internal/handlers/response.go @@ -104,7 +104,7 @@ func Handle1FAResponse(ctx *middlewares.AutheliaCtx, targetURI, requestMethod st return } - if !utils.IsRedirectionSafe(*targetURL, ctx.Configuration.Session.Domain) { + if !utils.URLDomainHasSuffix(*targetURL, ctx.Configuration.Session.Domain) { ctx.Logger.Debugf("Redirection URL %s is not safe", targetURI) if !ctx.Providers.Authorizer.IsSecondFactorEnabled() && ctx.Configuration.DefaultRedirectionURL != "" { diff --git a/internal/mocks/authelia_ctx.go b/internal/mocks/authelia_ctx.go index bdcc76714..22c0f5003 100644 --- a/internal/mocks/authelia_ctx.go +++ b/internal/mocks/authelia_ctx.go @@ -67,11 +67,12 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx { datetime, _ := time.Parse("2006-Jan-02", "2013-Feb-03") mockAuthelia.Clock.Set(datetime) - configuration := schema.Configuration{} - configuration.Session.RememberMeDuration = schema.DefaultSessionConfiguration.RememberMeDuration - configuration.Session.Name = "authelia_session" - configuration.AccessControl.DefaultPolicy = "deny" - configuration.AccessControl.Rules = []schema.ACLRule{{ + config := schema.Configuration{} + config.Session.RememberMeDuration = schema.DefaultSessionConfiguration.RememberMeDuration + config.Session.Name = "authelia_session" + config.Session.Domain = "example.com" + config.AccessControl.DefaultPolicy = "deny" + config.AccessControl.Rules = []schema.ACLRule{{ Domains: []string{"bypass.example.com"}, Policy: "bypass", }, { @@ -106,12 +107,12 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx { providers.Notifier = mockAuthelia.NotifierMock providers.Authorizer = authorization.NewAuthorizer( - &configuration) + &config) providers.SessionProvider = session.NewProvider( - configuration.Session, nil) + config.Session, nil) - providers.Regulator = regulation.NewRegulator(configuration.Regulation, providers.StorageProvider, &mockAuthelia.Clock) + providers.Regulator = regulation.NewRegulator(config.Regulation, providers.StorageProvider, &mockAuthelia.Clock) mockAuthelia.TOTPMock = NewMockTOTP(mockAuthelia.Ctrl) providers.TOTP = mockAuthelia.TOTPMock @@ -126,7 +127,7 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx { // Set a cookie to identify this client throughout the test. // request.Request.Header.SetCookie("authelia_session", "client_cookie"). - ctx := middlewares.NewAutheliaCtx(request, configuration, providers) + ctx := middlewares.NewAutheliaCtx(request, config, providers) mockAuthelia.Ctx = ctx logger, hook := test.NewNullLogger() diff --git a/internal/utils/const.go b/internal/utils/const.go index b4b3f0158..b9f0b33fa 100644 --- a/internal/utils/const.go +++ b/internal/utils/const.go @@ -31,6 +31,11 @@ const ( unknown = "unknown" ) +const ( + period = "." + https = "https" +) + // X.509 consts. const ( BlockTypeRSAPrivateKey = "RSA PRIVATE KEY" diff --git a/internal/utils/safe_redirection.go b/internal/utils/safe_redirection.go deleted file mode 100644 index b7a95b9d0..000000000 --- a/internal/utils/safe_redirection.go +++ /dev/null @@ -1,31 +0,0 @@ -package utils - -import ( - "fmt" - "net/url" - "strings" -) - -// IsRedirectionSafe determines whether the URL is safe to be redirected to. -func IsRedirectionSafe(url url.URL, protectedDomain string) bool { - if url.Scheme != "https" { - return false - } - - if !strings.HasSuffix(url.Hostname(), protectedDomain) { - return false - } - - return true -} - -// IsRedirectionURISafe determines whether the URI is safe to be redirected to. -func IsRedirectionURISafe(uri, protectedDomain string) (bool, error) { - targetURL, err := url.ParseRequestURI(uri) - - if err != nil { - return false, fmt.Errorf("Unable to parse redirection URI %s: %w", uri, err) - } - - return targetURL != nil && IsRedirectionSafe(*targetURL, protectedDomain), nil -} diff --git a/internal/utils/safe_redirection_test.go b/internal/utils/safe_redirection_test.go deleted file mode 100644 index 87c58a7db..000000000 --- a/internal/utils/safe_redirection_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package utils - -import ( - "net/url" - "testing" - - "github.com/stretchr/testify/assert" -) - -func isURLSafe(requestURI string, domain string) bool { //nolint:unparam - url, _ := url.ParseRequestURI(requestURI) - return IsRedirectionSafe(*url, domain) -} - -func TestIsRedirectionSafe_ShouldReturnFalseOnBadScheme(t *testing.T) { - assert.False(t, isURLSafe("http://secure.example.com", "example.com")) - assert.False(t, isURLSafe("ftp://secure.example.com", "example.com")) - assert.True(t, isURLSafe("https://secure.example.com", "example.com")) -} - -func TestIsRedirectionSafe_ShouldReturnFalseOnBadDomain(t *testing.T) { - assert.False(t, isURLSafe("https://secure.example.com.c", "example.com")) - assert.False(t, isURLSafe("https://secure.example.comc", "example.com")) - assert.False(t, isURLSafe("https://secure.example.co", "example.com")) -} - -func TestIsRedirectionURISafe_CannotParseURI(t *testing.T) { - _, err := IsRedirectionURISafe("http//invalid", "example.com") - assert.EqualError(t, err, "Unable to parse redirection URI http//invalid: parse \"http//invalid\": invalid URI for request") -} - -func TestIsRedirectionURISafe_InvalidRedirectionURI(t *testing.T) { - valid, err := IsRedirectionURISafe("http://myurl.com/myresource", "example.com") - assert.NoError(t, err) - assert.False(t, valid) -} - -func TestIsRedirectionURISafe_ValidRedirectionURI(t *testing.T) { - valid, err := IsRedirectionURISafe("http://myurl.example.com/myresource", "example.com") - assert.NoError(t, err) - assert.False(t, valid) -} diff --git a/internal/utils/url.go b/internal/utils/url.go index c2a227a89..09d7e8e70 100644 --- a/internal/utils/url.go +++ b/internal/utils/url.go @@ -1,8 +1,10 @@ package utils import ( + "fmt" "net/url" "path" + "strings" ) // URLPathFullClean returns a URL path with the query parameters appended (full path) with the path portion parsed @@ -27,3 +29,31 @@ func URLPathFullClean(u *url.URL) (output string) { return path.Clean(u.Path) } } + +// URLDomainHasSuffix determines whether the uri has a suffix of the domain value. +func URLDomainHasSuffix(uri url.URL, domain string) bool { + if uri.Scheme != https { + return false + } + + if uri.Hostname() == domain { + return true + } + + if strings.HasSuffix(uri.Hostname(), period+domain) { + return true + } + + return false +} + +// IsRedirectionURISafe determines whether the URI is safe to be redirected to. +func IsRedirectionURISafe(uri, protectedDomain string) (safe bool, err error) { + var parsedURI *url.URL + + if parsedURI, err = url.ParseRequestURI(uri); err != nil { + return false, fmt.Errorf("failed to parse URI '%s': %w", uri, err) + } + + return parsedURI != nil && URLDomainHasSuffix(*parsedURI, protectedDomain), nil +} diff --git a/internal/utils/url_test.go b/internal/utils/url_test.go index a61cdc18c..0873b1b26 100644 --- a/internal/utils/url_test.go +++ b/internal/utils/url_test.go @@ -38,3 +38,46 @@ func TestURLPathFullClean(t *testing.T) { }) } } + +func isURLSafe(requestURI string, domain string) bool { //nolint:unparam + u, _ := url.ParseRequestURI(requestURI) + return URLDomainHasSuffix(*u, domain) +} + +func TestIsRedirectionSafe_ShouldReturnTrueOnExactDomain(t *testing.T) { + assert.True(t, isURLSafe("https://example.com", "example.com")) +} + +func TestIsRedirectionSafe_ShouldReturnFalseOnBadScheme(t *testing.T) { + assert.False(t, isURLSafe("http://secure.example.com", "example.com")) + assert.False(t, isURLSafe("ftp://secure.example.com", "example.com")) + assert.True(t, isURLSafe("https://secure.example.com", "example.com")) +} + +func TestIsRedirectionSafe_ShouldReturnFalseOnBadDomain(t *testing.T) { + assert.False(t, isURLSafe("https://secure.example.com.c", "example.com")) + assert.False(t, isURLSafe("https://secure.example.comc", "example.com")) + assert.False(t, isURLSafe("https://secure.example.co", "example.com")) + assert.False(t, isURLSafe("https://secure.notexample.com", "example.com")) +} + +func TestIsRedirectionURISafe_CannotParseURI(t *testing.T) { + _, err := IsRedirectionURISafe("http//invalid", "example.com") + assert.EqualError(t, err, "failed to parse URI 'http//invalid': parse \"http//invalid\": invalid URI for request") +} + +func TestIsRedirectionURISafe_InvalidRedirectionURI(t *testing.T) { + valid, err := IsRedirectionURISafe("http://myurl.com/myresource", "example.com") + assert.NoError(t, err) + assert.False(t, valid) +} + +func TestIsRedirectionURISafe_ValidRedirectionURI(t *testing.T) { + valid, err := IsRedirectionURISafe("http://myurl.example.com/myresource", "example.com") + assert.NoError(t, err) + assert.False(t, valid) + + valid, err = IsRedirectionURISafe("http://example.com/myresource", "example.com") + assert.NoError(t, err) + assert.False(t, valid) +}