From 7ed1753fcced351c81961bf520a7bfb2caac6e88 Mon Sep 17 00:00:00 2001 From: Xe Iaso Date: Wed, 29 Oct 2025 16:07:31 -0400 Subject: [PATCH] fix(lib): close open redirect when in subrequest mode (#1222) * test(nginx-external-auth): bring up to code standards Signed-off-by: Xe Iaso * fix(lib): close open redirect when in subrequest mode Closes GHSA-cf57-c578-7jvv Previously Anubis had an open redirect in subrequest auth mode due to an insufficent fix in GHSA-jhjj-2g64-px7c. This patch adds additional validation at several steps of the flow to prevent open redirects in subrequest auth mode as well as implements automated testing to prevent this from occuring in the future. * docs: update CHANGELOG Signed-off-by: Xe Iaso --------- Signed-off-by: Xe Iaso --- docs/docs/CHANGELOG.md | 1 + lib/http.go | 31 +- lib/redirect_security_test.go | 296 ++++++++++++++++++++ test/nginx-external-auth/deployment.yaml | 68 +++-- test/nginx-external-auth/ingress.yaml | 26 +- test/nginx-external-auth/kustomization.yaml | 2 +- test/nginx-external-auth/service.yaml | 8 +- test/nginx-external-auth/start.sh | 16 +- 8 files changed, 384 insertions(+), 64 deletions(-) create mode 100644 lib/redirect_security_test.go diff --git a/docs/docs/CHANGELOG.md b/docs/docs/CHANGELOG.md index 7092a87..39d09d0 100644 --- a/docs/docs/CHANGELOG.md +++ b/docs/docs/CHANGELOG.md @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Allow multiple consecutive slashes in a row in application paths ([#754](https://github.com/TecharoHQ/anubis/issues/754)). - Add option to set `targetSNI` to special keyword 'auto' to indicate that it should be automatically set to the request Host name ([424](https://github.com/TecharoHQ/anubis/issues/424)). - The Preact challenge has been removed from the default configuration. It will be deprecated in the future. +- An open redirect when in subrequest mode has been fixed. ### Potentially breaking changes diff --git a/lib/http.go b/lib/http.go index 65f9640..60e51d4 100644 --- a/lib/http.go +++ b/lib/http.go @@ -296,6 +296,16 @@ func (s *Server) constructRedirectURL(r *http.Request) (string, error) { if proto == "" || host == "" || uri == "" { return "", errors.New(localizer.T("missing_required_forwarded_headers")) } + + switch proto { + case "http", "https": + // allowed + default: + lg := internal.GetRequestLogger(s.logger, r) + lg.Warn("invalid protocol in X-Forwarded-Proto", "proto", proto) + return "", errors.New(localizer.T("invalid_redirect")) + } + // Check if host is allowed in RedirectDomains (supports '*' via glob) if len(s.opts.RedirectDomains) > 0 && !matchRedirectDomain(s.opts.RedirectDomains, host) { lg := internal.GetRequestLogger(s.logger, r) @@ -369,16 +379,31 @@ func (s *Server) ServeHTTPNext(w http.ResponseWriter, r *http.Request) { localizer := localization.GetLocalizer(r) redir := r.FormValue("redir") - urlParsed, err := r.URL.Parse(redir) + urlParsed, err := url.ParseRequestURI(redir) if err != nil { - s.respondWithStatus(w, r, localizer.T("redirect_not_parseable"), makeCode(err), http.StatusBadRequest) + // if ParseRequestURI fails, try as relative URL + urlParsed, err = r.URL.Parse(redir) + if err != nil { + s.respondWithStatus(w, r, localizer.T("redirect_not_parseable"), makeCode(err), http.StatusBadRequest) + return + } + } + + // validate URL scheme to prevent javascript:, data:, file:, tel:, etc. + switch urlParsed.Scheme { + case "", "http", "https": + // allowed: empty scheme means relative URL + default: + lg := internal.GetRequestLogger(s.logger, r) + lg.Warn("XSS attempt blocked, invalid redirect scheme", "scheme", urlParsed.Scheme, "redir", redir) + s.respondWithStatus(w, r, localizer.T("invalid_redirect"), "", http.StatusBadRequest) return } hostNotAllowed := len(urlParsed.Host) > 0 && len(s.opts.RedirectDomains) != 0 && !matchRedirectDomain(s.opts.RedirectDomains, urlParsed.Host) - hostMismatch := r.URL.Host != "" && urlParsed.Host != r.URL.Host + hostMismatch := r.URL.Host != "" && urlParsed.Host != "" && urlParsed.Host != r.URL.Host if hostNotAllowed || hostMismatch { lg := internal.GetRequestLogger(s.logger, r) diff --git a/lib/redirect_security_test.go b/lib/redirect_security_test.go new file mode 100644 index 0000000..e16206a --- /dev/null +++ b/lib/redirect_security_test.go @@ -0,0 +1,296 @@ +package lib + +import ( + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/TecharoHQ/anubis/lib/policy" +) + +func TestRedirectSecurity(t *testing.T) { + tests := []struct { + name string + testType string // "constructRedirectURL", "serveHTTPNext", "renderIndex" + + // For constructRedirectURL tests + xForwardedProto string + xForwardedHost string + xForwardedUri string + + // For serveHTTPNext tests + redirParam string + reqHost string + + // For renderIndex tests + returnHTTPStatusOnly bool + + // Expected results + expectedStatus int + shouldError bool + shouldNotRedirect bool + shouldBlock bool + errorContains string + }{ + // constructRedirectURL tests - X-Forwarded-Proto validation + { + name: "constructRedirectURL: javascript protocol should be rejected", + testType: "constructRedirectURL", + xForwardedProto: "javascript", + xForwardedHost: "example.com", + xForwardedUri: "alert(1)", + shouldError: true, + errorContains: "invalid", + }, + { + name: "constructRedirectURL: data protocol should be rejected", + testType: "constructRedirectURL", + xForwardedProto: "data", + xForwardedHost: "text/html", + xForwardedUri: ",", + shouldError: true, + errorContains: "invalid", + }, + { + name: "constructRedirectURL: file protocol should be rejected", + testType: "constructRedirectURL", + xForwardedProto: "file", + xForwardedHost: "", + xForwardedUri: "/etc/passwd", + shouldError: true, + errorContains: "invalid", + }, + { + name: "constructRedirectURL: ftp protocol should be rejected", + testType: "constructRedirectURL", + xForwardedProto: "ftp", + xForwardedHost: "example.com", + xForwardedUri: "/file.txt", + shouldError: true, + errorContains: "invalid", + }, + { + name: "constructRedirectURL: https protocol should be allowed", + testType: "constructRedirectURL", + xForwardedProto: "https", + xForwardedHost: "example.com", + xForwardedUri: "/foo", + shouldError: false, + }, + { + name: "constructRedirectURL: http protocol should be allowed", + testType: "constructRedirectURL", + xForwardedProto: "http", + xForwardedHost: "example.com", + xForwardedUri: "/bar", + shouldError: false, + }, + + // serveHTTPNext tests - redir parameter validation + { + name: "serveHTTPNext: javascript: URL should be rejected", + testType: "serveHTTPNext", + redirParam: "javascript:alert(1)", + reqHost: "example.com", + expectedStatus: http.StatusBadRequest, + shouldNotRedirect: true, + }, + { + name: "serveHTTPNext: data: URL should be rejected", + testType: "serveHTTPNext", + redirParam: "data:text/html,", + reqHost: "example.com", + expectedStatus: http.StatusBadRequest, + shouldNotRedirect: true, + }, + { + name: "serveHTTPNext: file: URL should be rejected", + testType: "serveHTTPNext", + redirParam: "file:///etc/passwd", + reqHost: "example.com", + expectedStatus: http.StatusBadRequest, + shouldNotRedirect: true, + }, + { + name: "serveHTTPNext: vbscript: URL should be rejected", + testType: "serveHTTPNext", + redirParam: "vbscript:msgbox(1)", + reqHost: "example.com", + expectedStatus: http.StatusBadRequest, + shouldNotRedirect: true, + }, + { + name: "serveHTTPNext: valid https URL should work", + testType: "serveHTTPNext", + redirParam: "https://example.com/foo", + reqHost: "example.com", + expectedStatus: http.StatusFound, + }, + { + name: "serveHTTPNext: valid relative URL should work", + testType: "serveHTTPNext", + redirParam: "/foo/bar", + reqHost: "example.com", + expectedStatus: http.StatusFound, + }, + { + name: "serveHTTPNext: external domain should be blocked", + testType: "serveHTTPNext", + redirParam: "https://evil.com/phishing", + reqHost: "example.com", + expectedStatus: http.StatusBadRequest, + shouldBlock: true, + }, + { + name: "serveHTTPNext: relative path should work", + testType: "serveHTTPNext", + redirParam: "/safe/path", + reqHost: "example.com", + expectedStatus: http.StatusFound, + }, + { + name: "serveHTTPNext: empty redir should show success page", + testType: "serveHTTPNext", + redirParam: "", + reqHost: "example.com", + expectedStatus: http.StatusOK, + }, + + // renderIndex tests - full subrequest auth flow + { + name: "renderIndex: javascript protocol in X-Forwarded-Proto", + testType: "renderIndex", + xForwardedProto: "javascript", + xForwardedHost: "example.com", + xForwardedUri: "alert(1)", + returnHTTPStatusOnly: true, + expectedStatus: http.StatusBadRequest, + }, + { + name: "renderIndex: data protocol in X-Forwarded-Proto", + testType: "renderIndex", + xForwardedProto: "data", + xForwardedHost: "example.com", + xForwardedUri: "text/html,", + returnHTTPStatusOnly: true, + expectedStatus: http.StatusBadRequest, + }, + { + name: "renderIndex: valid https redirect", + testType: "renderIndex", + xForwardedProto: "https", + xForwardedHost: "example.com", + xForwardedUri: "/protected/page", + returnHTTPStatusOnly: true, + expectedStatus: http.StatusTemporaryRedirect, + }, + } + + s := &Server{ + opts: Options{ + PublicUrl: "https://anubis.example.com", + RedirectDomains: []string{}, + }, + logger: slog.Default(), + policy: &policy.ParsedConfig{}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + switch tt.testType { + case "constructRedirectURL": + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("X-Forwarded-Proto", tt.xForwardedProto) + req.Header.Set("X-Forwarded-Host", tt.xForwardedHost) + req.Header.Set("X-Forwarded-Uri", tt.xForwardedUri) + + redirectURL, err := s.constructRedirectURL(req) + + if tt.shouldError { + if err == nil { + t.Errorf("expected error containing %q, got nil", tt.errorContains) + t.Logf("got redirect URL: %s", redirectURL) + } else if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { + t.Logf("expected error containing %q, got: %v", tt.errorContains, err) + } + } else { + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + // Verify the redirect URL is safe + if redirectURL != "" { + parsed, err := url.Parse(redirectURL) + if err != nil { + t.Errorf("failed to parse redirect URL: %v", err) + } + redirParam := parsed.Query().Get("redir") + if redirParam != "" { + redirParsed, err := url.Parse(redirParam) + if err != nil { + t.Errorf("failed to parse redir parameter: %v", err) + } + if redirParsed.Scheme != "http" && redirParsed.Scheme != "https" { + t.Errorf("redir parameter has unsafe scheme: %s", redirParsed.Scheme) + } + } + } + } + + case "serveHTTPNext": + req := httptest.NewRequest("GET", "/.within.website/?redir="+url.QueryEscape(tt.redirParam), nil) + req.Host = tt.reqHost + req.URL.Host = tt.reqHost + rr := httptest.NewRecorder() + + s.ServeHTTPNext(rr, req) + + if rr.Code != tt.expectedStatus { + t.Errorf("expected status %d, got %d", tt.expectedStatus, rr.Code) + t.Logf("body: %s", rr.Body.String()) + } + + if tt.shouldNotRedirect { + location := rr.Header().Get("Location") + if location != "" { + t.Errorf("expected no redirect, but got Location header: %s", location) + } + } + + if tt.shouldBlock { + location := rr.Header().Get("Location") + if location != "" && strings.Contains(location, "evil.com") { + t.Errorf("redirect to evil.com was not blocked: %s", location) + } + } + + case "renderIndex": + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("X-Forwarded-Proto", tt.xForwardedProto) + req.Header.Set("X-Forwarded-Host", tt.xForwardedHost) + req.Header.Set("X-Forwarded-Uri", tt.xForwardedUri) + + rr := httptest.NewRecorder() + s.RenderIndex(rr, req, policy.CheckResult{}, nil, tt.returnHTTPStatusOnly) + + if rr.Code != tt.expectedStatus { + t.Errorf("expected status %d, got %d", tt.expectedStatus, rr.Code) + } + + if tt.expectedStatus == http.StatusTemporaryRedirect { + location := rr.Header().Get("Location") + if location == "" { + t.Error("expected Location header, got none") + } else { + // Verify the location doesn't contain javascript: + if strings.Contains(location, "javascript") { + t.Errorf("Location header contains 'javascript': %s", location) + } + } + } + } + }) + } +} diff --git a/test/nginx-external-auth/deployment.yaml b/test/nginx-external-auth/deployment.yaml index f4b408b..e702657 100644 --- a/test/nginx-external-auth/deployment.yaml +++ b/test/nginx-external-auth/deployment.yaml @@ -12,39 +12,37 @@ spec: app: nginx-external-auth spec: volumes: - - name: config - configMap: - name: nginx-cfg - containers: - - name: www - image: nginx:alpine - resources: - limits: - memory: "128Mi" - cpu: "500m" - requests: - memory: "128Mi" - cpu: "500m" - ports: - - containerPort: 80 - volumeMounts: - name: config - mountPath: /etc/nginx/conf.d - readOnly: true - - name: anubis - image: ttl.sh/techaro/anubis-external-auth:latest - imagePullPolicy: Always - resources: - limits: - cpu: 500m - memory: 128Mi - requests: - cpu: 250m - memory: 128Mi - env: - - name: TARGET - value: " " - - name: REDIRECT_DOMAINS - value: nginx.local.cetacean.club - - + configMap: + name: nginx-cfg + containers: + - name: www + image: nginx:alpine + resources: + limits: + memory: "128Mi" + cpu: "500m" + requests: + memory: "128Mi" + cpu: "500m" + ports: + - containerPort: 80 + volumeMounts: + - name: config + mountPath: /etc/nginx/conf.d + readOnly: true + - name: anubis + image: ttl.sh/techaro/anubis:latest + imagePullPolicy: Always + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 250m + memory: 128Mi + env: + - name: TARGET + value: " " + - name: REDIRECT_DOMAINS + value: nginx.local.cetacean.club diff --git a/test/nginx-external-auth/ingress.yaml b/test/nginx-external-auth/ingress.yaml index 6fc8737..2cf2c0a 100644 --- a/test/nginx-external-auth/ingress.yaml +++ b/test/nginx-external-auth/ingress.yaml @@ -9,17 +9,17 @@ metadata: spec: ingressClassName: traefik tls: - - hosts: - - nginx.local.cetacean.club - secretName: nginx-local-cetacean-club-public-tls + - hosts: + - nginx.local.cetacean.club + secretName: nginx-local-cetacean-club-public-tls rules: - - host: nginx.local.cetacean.club - http: - paths: - - pathType: Prefix - path: "/" - backend: - service: - name: nginx-external-auth - port: - name: http + - host: nginx.local.cetacean.club + http: + paths: + - pathType: Prefix + path: "/" + backend: + service: + name: nginx-external-auth + port: + name: http diff --git a/test/nginx-external-auth/kustomization.yaml b/test/nginx-external-auth/kustomization.yaml index 7410f97..b11a6e7 100644 --- a/test/nginx-external-auth/kustomization.yaml +++ b/test/nginx-external-auth/kustomization.yaml @@ -7,4 +7,4 @@ configMapGenerator: - name: nginx-cfg behavior: create files: - - ./conf.d/default.conf + - ./conf.d/default.conf diff --git a/test/nginx-external-auth/service.yaml b/test/nginx-external-auth/service.yaml index d2e018c..5ce5c24 100644 --- a/test/nginx-external-auth/service.yaml +++ b/test/nginx-external-auth/service.yaml @@ -6,8 +6,8 @@ spec: selector: app: nginx-external-auth ports: - - name: http - protocol: TCP - port: 80 - targetPort: 80 + - name: http + protocol: TCP + port: 80 + targetPort: 80 type: ClusterIP diff --git a/test/nginx-external-auth/start.sh b/test/nginx-external-auth/start.sh index 044238a..ec3faf0 100755 --- a/test/nginx-external-auth/start.sh +++ b/test/nginx-external-auth/start.sh @@ -4,20 +4,20 @@ set -euo pipefail # Build container image ( - cd ../.. \ - && npm ci \ - && npm run container -- \ - --docker-repo ttl.sh/techaro/anubis-external-auth \ - --docker-tags ttl.sh/techaro/anubis-external-auth:latest + cd ../.. && + npm ci && + npm run container -- \ + --docker-repo ttl.sh/techaro/anubis \ + --docker-tags ttl.sh/techaro/anubis:latest ) kubectl apply -k . echo "open https://nginx.local.cetacean.club, press control c when done" control_c() { - kubectl delete -k . - exit + kubectl delete -k . + exit } trap control_c SIGINT -sleep infinity \ No newline at end of file +sleep infinity