From 8e97a934e5887ebef76553b344ad4494ee7f07a1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 22 Mar 2022 19:39:22 +0000 Subject: [PATCH 1/3] Prevent redirect to Host (2) Unhelpfully Locations starting with `/\` will be converted by the browser to `//` because ... well I do not fully understand. Certainly the RFCs and MDN do not indicate that this would be expected. Providing "compatibility" with the (mis)behaviour of a certain proprietary OS is my suspicion. However, we clearly have to protect against this. Therefore we should reject redirection locations that match the regular expression: `^/[\\\\/]+` Reference #9678 Signed-off-by: Andrew Thornton --- modules/context/context.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/modules/context/context.go b/modules/context/context.go index 57448907e25ff..95e1ad26cb9e5 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -17,6 +17,7 @@ import ( "net/http" "net/url" "path" + "regexp" "strconv" "strings" "time" @@ -174,6 +175,8 @@ func (ctx *Context) HasValue(name string) bool { return ok } +var precedingSlashesRE = regexp.MustCompile(`^/[\\/]+`) + // RedirectToFirst redirects to first not empty URL func (ctx *Context) RedirectToFirst(location ...string) { for _, loc := range location { @@ -181,6 +184,10 @@ func (ctx *Context) RedirectToFirst(location ...string) { continue } + if precedingSlashesRE.MatchString(loc) { + continue + } + u, err := url.Parse(loc) if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(loc), strings.ToLower(setting.AppURL))) { continue From ca59d8834f3fd4dcd5f7c8a29b2ba1ae3585e5b1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 23 Mar 2022 00:19:09 +0000 Subject: [PATCH 2/3] simplify Signed-off-by: Andrew Thornton --- modules/context/context.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/modules/context/context.go b/modules/context/context.go index 95e1ad26cb9e5..7f653ec44dfaa 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -17,7 +17,6 @@ import ( "net/http" "net/url" "path" - "regexp" "strconv" "strings" "time" @@ -175,8 +174,6 @@ func (ctx *Context) HasValue(name string) bool { return ok } -var precedingSlashesRE = regexp.MustCompile(`^/[\\/]+`) - // RedirectToFirst redirects to first not empty URL func (ctx *Context) RedirectToFirst(location ...string) { for _, loc := range location { @@ -184,7 +181,7 @@ func (ctx *Context) RedirectToFirst(location ...string) { continue } - if precedingSlashesRE.MatchString(loc) { + if len(loc) > 1 && loc[0] == '/' && (loc[1] == '/' || loc[1] == '\\') { continue } From f907294a7a8146e0ecedcad46e451daa64142a5b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 23 Mar 2022 11:17:15 +0000 Subject: [PATCH 3/3] as per review Signed-off-by: Andrew Thornton --- modules/context/context.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/context/context.go b/modules/context/context.go index 6fafdd51eb929..6cd503984f546 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -181,6 +181,8 @@ func (ctx *Context) RedirectToFirst(location ...string) { continue } + // Unfortunately browsers consider a redirect Location with preceding "//" and "/\" as meaning redirect to "http(s)://REST_OF_PATH" + // Therefore we should ignore these redirect locations to prevent open redirects if len(loc) > 1 && loc[0] == '/' && (loc[1] == '/' || loc[1] == '\\') { continue }