From bd7d1e2f169d6cdfecd952a1d3ed55d0f49f4104 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Fri, 28 Sep 2018 23:19:08 -0400 Subject: [PATCH] routes: fix open redirect vulnerability (#5355) Reported by @cezar97. --- gogs.go | 2 +- pkg/tool/path.go | 12 ++++++++++++ pkg/tool/path_test.go | 32 ++++++++++++++++++++++++++++++++ routes/repo/branch.go | 3 ++- routes/repo/repo.go | 6 +++--- routes/user/auth.go | 12 +++--------- routes/user/profile.go | 5 +++-- templates/.VERSION | 2 +- 8 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 pkg/tool/path.go create mode 100644 pkg/tool/path_test.go diff --git a/gogs.go b/gogs.go index 408aabd9c..a3ec36f05 100644 --- a/gogs.go +++ b/gogs.go @@ -16,7 +16,7 @@ import ( "github.com/gogs/gogs/pkg/setting" ) -const APP_VER = "0.11.66.0928" +const APP_VER = "0.11.67.0928" func init() { setting.AppVer = APP_VER diff --git a/pkg/tool/path.go b/pkg/tool/path.go new file mode 100644 index 000000000..e478abc52 --- /dev/null +++ b/pkg/tool/path.go @@ -0,0 +1,12 @@ +// Copyright 2018 The Gogs Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package tool + +// IsSameSiteURLPath returns true if the URL path belongs to the same site, false otherwise. +// False: //url, http://url, /\url +// True: /url +func IsSameSiteURLPath(url string) bool { + return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\' +} diff --git a/pkg/tool/path_test.go b/pkg/tool/path_test.go new file mode 100644 index 000000000..530238cef --- /dev/null +++ b/pkg/tool/path_test.go @@ -0,0 +1,32 @@ +// Copyright 2018 The Gogs Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package tool + +import ( + "testing" + + . "github.com/smartystreets/goconvey/convey" +) + +func Test_IsSameSiteURLPath(t *testing.T) { + Convey("Check if a path belongs to the same site", t, func() { + testCases := []struct { + url string + expect bool + }{ + {"//github.com", false}, + {"http://github.com", false}, + {"https://github.com", false}, + {"/\\github.com", false}, + + {"/admin", true}, + {"/user/repo", true}, + } + + for _, tc := range testCases { + So(IsSameSiteURLPath(tc.url), ShouldEqual, tc.expect) + } + }) +} diff --git a/routes/repo/branch.go b/routes/repo/branch.go index 432ebe861..e24df65ab 100644 --- a/routes/repo/branch.go +++ b/routes/repo/branch.go @@ -14,6 +14,7 @@ import ( "github.com/gogs/gogs/models" "github.com/gogs/gogs/pkg/context" + "github.com/gogs/gogs/pkg/tool" ) const ( @@ -112,7 +113,7 @@ func DeleteBranchPost(c *context.Context) { defer func() { redirectTo := c.Query("redirect_to") - if len(redirectTo) == 0 { + if !tool.IsSameSiteURLPath(redirectTo) { redirectTo = c.Repo.RepoLink } c.Redirect(redirectTo) diff --git a/routes/repo/repo.go b/routes/repo/repo.go index 26ebeca08..00dbc2f82 100644 --- a/routes/repo/repo.go +++ b/routes/repo/repo.go @@ -242,7 +242,7 @@ func Action(c *context.Context) { err = models.StarRepo(c.User.ID, c.Repo.Repository.ID, false) case "desc": // FIXME: this is not used if !c.Repo.IsOwner() { - c.Error(404) + c.NotFound() return } @@ -252,12 +252,12 @@ func Action(c *context.Context) { } if err != nil { - c.Handle(500, fmt.Sprintf("Action (%s)", c.Params(":action")), err) + c.ServerError(fmt.Sprintf("Action (%s)", c.Params(":action")), err) return } redirectTo := c.Query("redirect_to") - if len(redirectTo) == 0 { + if !tool.IsSameSiteURLPath(redirectTo) { redirectTo = c.Repo.RepoLink } c.Redirect(redirectTo) diff --git a/routes/user/auth.go b/routes/user/auth.go index 0f4a490fa..3140d9f99 100644 --- a/routes/user/auth.go +++ b/routes/user/auth.go @@ -17,6 +17,7 @@ import ( "github.com/gogs/gogs/pkg/form" "github.com/gogs/gogs/pkg/mailer" "github.com/gogs/gogs/pkg/setting" + "github.com/gogs/gogs/pkg/tool" ) const ( @@ -72,13 +73,6 @@ func AutoLogin(c *context.Context) (bool, error) { return true, nil } -// isValidRedirect returns false if the URL does not redirect to same site. -// False: //url, http://url, /\url -// True: /url -func isValidRedirect(url string) bool { - return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\' -} - func Login(c *context.Context) { c.Title("sign_in") @@ -97,7 +91,7 @@ func Login(c *context.Context) { } if isSucceed { - if isValidRedirect(redirectTo) { + if tool.IsSameSiteURLPath(redirectTo) { c.Redirect(redirectTo) } else { c.SubURLRedirect("/") @@ -143,7 +137,7 @@ func afterLogin(c *context.Context, u *models.User, remember bool) { redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")) c.SetCookie("redirect_to", "", -1, setting.AppSubURL) - if isValidRedirect(redirectTo) { + if tool.IsSameSiteURLPath(redirectTo) { c.Redirect(redirectTo) return } diff --git a/routes/user/profile.go b/routes/user/profile.go index e42836c6b..dfdc80d54 100644 --- a/routes/user/profile.go +++ b/routes/user/profile.go @@ -15,6 +15,7 @@ import ( "github.com/gogs/gogs/models/errors" "github.com/gogs/gogs/pkg/context" "github.com/gogs/gogs/pkg/setting" + "github.com/gogs/gogs/pkg/tool" "github.com/gogs/gogs/routes/repo" ) @@ -157,12 +158,12 @@ func Action(c *context.Context) { } if err != nil { - c.Handle(500, fmt.Sprintf("Action (%s)", c.Params(":action")), err) + c.ServerError(fmt.Sprintf("Action (%s)", c.Params(":action")), err) return } redirectTo := c.Query("redirect_to") - if len(redirectTo) == 0 { + if !tool.IsSameSiteURLPath(redirectTo) { redirectTo = u.HomeLink() } c.Redirect(redirectTo) diff --git a/templates/.VERSION b/templates/.VERSION index 3aa5fc38f..cc2f02e69 100644 --- a/templates/.VERSION +++ b/templates/.VERSION @@ -1 +1 @@ -0.11.66.0928 +0.11.67.0928