Fix open redirect bypass in saferedirect
Relative redirect URLs are now normalized before validation. Paths containing backslashes (including percent-encoded %5c) are rejected, closing a bypass where /../\evil.com passed checks but http.Redirect normalized to /\evil.com. Reported by Fushuling and RacerZ. Signed-off-by: Bryan Frimin <bryan@probo.com>
This commit is contained in:
@@ -3,6 +3,23 @@
|
|||||||
User-facing notes on security-relevant changes to Probo. For the
|
User-facing notes on security-relevant changes to Probo. For the
|
||||||
vulnerability reporting process, see [SECURITY.md](SECURITY.md).
|
vulnerability reporting process, see [SECURITY.md](SECURITY.md).
|
||||||
|
|
||||||
|
## Open redirect bypass in saferedirect
|
||||||
|
|
||||||
|
_2026-05-26, Auth_
|
||||||
|
|
||||||
|
Relative redirect URLs are now normalized before validation. Paths
|
||||||
|
containing backslashes (including percent-encoded `%5c`) are rejected,
|
||||||
|
and the cleaned path is checked for protocol-relative and backslash
|
||||||
|
prefixes.
|
||||||
|
|
||||||
|
Previously, `Validate` only inspected the second character of the raw
|
||||||
|
input. A path like `/../\evil.com` passed validation because the second
|
||||||
|
character is `.`, but Go's `http.Redirect` normalized it to `/\evil.com`,
|
||||||
|
which browsers can treat as an external redirect.
|
||||||
|
|
||||||
|
Reported by [Fushuling](https://github.com/Fushuling) and
|
||||||
|
[RacerZ](https://github.com/RacerZ-fighting).
|
||||||
|
|
||||||
## Password changes invalidate existing sessions
|
## Password changes invalidate existing sessions
|
||||||
|
|
||||||
_2026-04-29, IAM_
|
_2026-04-29, IAM_
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
"net/url"
|
||||||
|
"path"
|
||||||
"strings"
|
"strings"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -54,11 +55,12 @@ func (sr *SafeRedirect) Validate(ctx context.Context, redirectURL string) (strin
|
|||||||
}
|
}
|
||||||
|
|
||||||
if strings.HasPrefix(redirectURL, "/") {
|
if strings.HasPrefix(redirectURL, "/") {
|
||||||
if len(redirectURL) > 1 && (redirectURL[1] == '/' || redirectURL[1] == '\\') {
|
safePath, ok := normalizeRelativePath(redirectURL)
|
||||||
|
if !ok {
|
||||||
return "", false
|
return "", false
|
||||||
}
|
}
|
||||||
|
|
||||||
return redirectURL, true
|
return safePath, true
|
||||||
}
|
}
|
||||||
|
|
||||||
parsedURL, err := url.Parse(redirectURL)
|
parsedURL, err := url.Parse(redirectURL)
|
||||||
@@ -93,3 +95,28 @@ func (sr *SafeRedirect) Redirect(w http.ResponseWriter, r *http.Request, redirec
|
|||||||
safeURL := sr.GetSafeRedirectURL(r.Context(), redirectURL, fallbackURL)
|
safeURL := sr.GetSafeRedirectURL(r.Context(), redirectURL, fallbackURL)
|
||||||
http.Redirect(w, r, safeURL, statusCode)
|
http.Redirect(w, r, safeURL, statusCode)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func normalizeRelativePath(redirectURL string) (string, bool) {
|
||||||
|
if strings.HasPrefix(redirectURL, "//") {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
|
||||||
|
if strings.Contains(redirectURL, `\`) || strings.Contains(strings.ToLower(redirectURL), "%5c") {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
|
||||||
|
cleaned := path.Clean(redirectURL)
|
||||||
|
if !strings.HasPrefix(cleaned, "/") {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(cleaned) > 1 && (cleaned[1] == '/' || cleaned[1] == '\\') {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
|
||||||
|
if strings.Contains(cleaned, `\`) {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
|
||||||
|
return cleaned, true
|
||||||
|
}
|
||||||
|
|||||||
@@ -103,6 +103,34 @@ func TestSafeRedirect_Validate(t *testing.T) {
|
|||||||
expectedURL: "",
|
expectedURL: "",
|
||||||
expectedIsValid: false,
|
expectedIsValid: false,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal backslash bypass",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/../\\evil.com/phishing",
|
||||||
|
expectedURL: "",
|
||||||
|
expectedIsValid: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "embedded backslash",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/foo/..\\evil.com/phishing",
|
||||||
|
expectedURL: "",
|
||||||
|
expectedIsValid: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "percent-encoded backslash",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/%5cevil.com/phishing",
|
||||||
|
expectedURL: "",
|
||||||
|
expectedIsValid: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "path normalization",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/foo/../dashboard",
|
||||||
|
expectedURL: "/dashboard",
|
||||||
|
expectedIsValid: true,
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
@@ -168,6 +196,13 @@ func TestSafeRedirect_GetSafeRedirectURL(t *testing.T) {
|
|||||||
fallbackURL: "/home",
|
fallbackURL: "/home",
|
||||||
expectedURL: "/home",
|
expectedURL: "/home",
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal backslash bypass",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/../\\evil.com/phishing",
|
||||||
|
fallbackURL: "/home",
|
||||||
|
expectedURL: "/home",
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
@@ -235,6 +270,14 @@ func TestSafeRedirect_Redirect(t *testing.T) {
|
|||||||
expectedStatus: http.StatusFound,
|
expectedStatus: http.StatusFound,
|
||||||
expectedURL: "/home",
|
expectedURL: "/home",
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal backslash bypass",
|
||||||
|
allowedHost: saferedirect.StaticHosts("example.com"),
|
||||||
|
redirectURL: "/../\\evil.com/phishing",
|
||||||
|
fallbackURL: "/home",
|
||||||
|
expectedStatus: http.StatusFound,
|
||||||
|
expectedURL: "/home",
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
|
|||||||
Reference in New Issue
Block a user