From e4c221afc40ef0a51855068e587271a71705c4f0 Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Tue, 19 Jul 2022 07:50:31 +0100 Subject: [PATCH] [v9.0.x] CSRF: Fix additional headers option (#52347) * merged and backport to 9.0.x * merged http_server * fix for provider being interface * fix for provider maybe * wire inject * wire inject * wire inject Co-authored-by: Emil Tullstedt --- pkg/api/http_server.go | 6 +- pkg/middleware/csrf/csrf.go | 161 ++++++++++++++++++++++++++++++++++++ pkg/middleware/csrf_test.go | 123 ++++++++++++++++++++++++++- pkg/server/wire.go | 2 + 4 files changed, 289 insertions(+), 3 deletions(-) create mode 100644 pkg/middleware/csrf/csrf.go diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 498d28dfc14..a2d2e798de2 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -14,6 +14,7 @@ import ( "sync" "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/middleware/csrf" "github.com/grafana/grafana/pkg/api/avatar" "github.com/grafana/grafana/pkg/api/routing" @@ -159,6 +160,7 @@ type HTTPServer struct { dashboardPermissionsService accesscontrol.DashboardPermissionsService starService star.Service CoremodelRegistry *coremodel.Registry + Csrf csrf.Service } type ServerOptions struct { @@ -193,6 +195,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi avatarCacheServer *avatar.AvatarCacheServer, preferenceService pref.Service, entityEventsService store.EntityEventsService, teamsPermissionsService accesscontrol.TeamPermissionsService, folderPermissionsService accesscontrol.FolderPermissionsService, dashboardPermissionsService accesscontrol.DashboardPermissionsService, starService star.Service, coremodelRegistry *coremodel.Registry, + csrf csrf.Service, ) (*HTTPServer, error) { web.Env = cfg.Env m := web.New() @@ -272,6 +275,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi dashboardPermissionsService: dashboardPermissionsService, starService: starService, CoremodelRegistry: coremodelRegistry, + Csrf: csrf, } if hs.Listener != nil { hs.log.Debug("Using provided listener") @@ -499,7 +503,7 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() { } m.Use(middleware.Recovery(hs.Cfg)) - m.UseMiddleware(middleware.CSRF(hs.Cfg.LoginCookieName, hs.log)) + m.UseMiddleware(hs.Csrf.Middleware()) hs.mapStatic(m, hs.Cfg.StaticRootPath, "build", "public/build") hs.mapStatic(m, hs.Cfg.StaticRootPath, "", "public", "/public/views/swagger.html") diff --git a/pkg/middleware/csrf/csrf.go b/pkg/middleware/csrf/csrf.go new file mode 100644 index 00000000000..978faa93bc4 --- /dev/null +++ b/pkg/middleware/csrf/csrf.go @@ -0,0 +1,161 @@ +package csrf + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "reflect" + + "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" +) + +type Service interface { + Middleware() func(http.Handler) http.Handler + TrustOrigin(origin string) + AddAdditionalHeaders(headerName string) + AddSafeEndpoint(endpoint string) +} + +type CSRF struct { + Cfg *setting.Cfg + + TrustedOrigins map[string]struct{} + Headers map[string]struct{} + SafeEndpoints map[string]struct{} +} + +func ProvideCSRFFilter(cfg *setting.Cfg) Service { + c := &CSRF{ + Cfg: cfg, + TrustedOrigins: map[string]struct{}{}, + Headers: map[string]struct{}{}, + SafeEndpoints: map[string]struct{}{}, + } + + additionalHeaders := cfg.SectionWithEnvOverrides("security").Key("csrf_additional_headers").Strings(" ") + trustedOrigins := cfg.SectionWithEnvOverrides("security").Key("csrf_trusted_origins").Strings(" ") + + for _, header := range additionalHeaders { + c.Headers[header] = struct{}{} + } + for _, origin := range trustedOrigins { + c.TrustedOrigins[origin] = struct{}{} + } + + return c +} + +func (c *CSRF) Middleware() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + e := &ErrorWithStatus{} + + err := c.Check(r) + if err != nil { + if !errors.As(err, &e) { + http.Error(w, fmt.Sprintf("internal server error: expected error type errorWithStatus, got %s. Error: %v", reflect.TypeOf(err), err), http.StatusInternalServerError) + } + http.Error(w, err.Error(), e.HTTPStatus) + return + } + + next.ServeHTTP(w, r) + }) + } +} + +func (c *CSRF) Check(r *http.Request) error { + // As per RFC 7231/4.2.2 these methods are idempotent: + // (GET is excluded because it may have side effects in some APIs) + safeMethods := []string{"HEAD", "OPTIONS", "TRACE"} + + // If request has no login cookie - skip CSRF checks + if _, err := r.Cookie(c.Cfg.LoginCookieName); errors.Is(err, http.ErrNoCookie) { + return nil + } + // Skip CSRF checks for "safe" methods + for _, method := range safeMethods { + if r.Method == method { + return nil + } + } + // Skip CSRF checks for "safe" endpoints + for safeEndpoint := range c.SafeEndpoints { + if r.URL.Path == safeEndpoint { + return nil + } + } + // Otherwise - verify that Origin matches the server origin + netAddr, err := util.SplitHostPortDefault(r.Host, "", "0") // we ignore the port + if err != nil { + return &ErrorWithStatus{Underlying: err, HTTPStatus: http.StatusBadRequest} + } + + o := r.Header.Get("Origin") + + // No Origin header sent, skip CSRF check. + if o == "" { + return nil + } + + originURL, err := url.Parse(o) + if err != nil { + return &ErrorWithStatus{Underlying: err, HTTPStatus: http.StatusBadRequest} + } + origin := originURL.Hostname() + + trustedOrigin := false + for h := range c.Headers { + customHost := r.Header.Get(h) + addr, err := util.SplitHostPortDefault(customHost, "", "0") // we ignore the port + if err != nil { + return &ErrorWithStatus{Underlying: err, HTTPStatus: http.StatusBadRequest} + } + if addr.Host == origin { + trustedOrigin = true + break + } + } + + for o := range c.TrustedOrigins { + if o == origin { + trustedOrigin = true + break + } + } + + hostnameMatches := origin == netAddr.Host + if netAddr.Host == "" || !trustedOrigin && !hostnameMatches { + return &ErrorWithStatus{Underlying: errors.New("origin not allowed"), HTTPStatus: http.StatusForbidden} + } + + return nil +} + +func (c *CSRF) TrustOrigin(origin string) { + c.TrustedOrigins[origin] = struct{}{} +} + +func (c *CSRF) AddAdditionalHeaders(headerName string) { + c.Headers[headerName] = struct{}{} +} + +// AddSafeEndpoint is used for endpoints requests to skip CSRF check +func (c *CSRF) AddSafeEndpoint(endpoint string) { + c.SafeEndpoints[endpoint] = struct{}{} +} + +type ErrorWithStatus struct { + Underlying error + HTTPStatus int +} + +func (e ErrorWithStatus) Error() string { + return e.Underlying.Error() +} + +func (e ErrorWithStatus) Unwrap() error { + return e.Underlying +} diff --git a/pkg/middleware/csrf_test.go b/pkg/middleware/csrf_test.go index 312356cce3f..11217faf12e 100644 --- a/pkg/middleware/csrf_test.go +++ b/pkg/middleware/csrf_test.go @@ -1,12 +1,17 @@ package middleware import ( + "errors" "net/http" "net/http/httptest" + "strings" "testing" - "github.com/grafana/grafana/pkg/infra/log" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/middleware/csrf" + "github.com/grafana/grafana/pkg/setting" ) func TestMiddlewareCSRF(t *testing.T) { @@ -98,6 +103,117 @@ func TestMiddlewareCSRF(t *testing.T) { } } +func TestCSRF_Check(t *testing.T) { + tests := []struct { + name string + request *http.Request + addtHeader map[string]struct{} + trustedOrigins map[string]struct{} + safeEndpoints map[string]struct{} + expectedOK bool + expectedStatus int + }{ + { + name: "base case", + request: postRequest(t, "", nil), + expectedOK: true, + }, + { + name: "base with null origin header", + request: postRequest(t, "", map[string]string{"Origin": "null"}), + expectedStatus: http.StatusForbidden, + }, + { + name: "grafana.org", + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.org"}), + expectedOK: true, + }, + { + name: "grafana.org with X-Forwarded-Host", + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}), + expectedStatus: http.StatusForbidden, + }, + { + name: "grafana.org with X-Forwarded-Host and header trusted", + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.org"}), + addtHeader: map[string]struct{}{"X-Forwarded-Host": {}}, + expectedOK: true, + }, + { + name: "grafana.org from grafana.com", + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}), + expectedStatus: http.StatusForbidden, + }, + { + name: "grafana.org from grafana.com explicit trust for grafana.com", + request: postRequest(t, "grafana.org", map[string]string{"Origin": "https://grafana.com"}), + trustedOrigins: map[string]struct{}{"grafana.com": {}}, + expectedOK: true, + }, + { + name: "grafana.org from grafana.com with X-Forwarded-Host and header trusted", + request: postRequest(t, "grafana.localhost", map[string]string{"X-Forwarded-Host": "grafana.org", "Origin": "https://grafana.com"}), + addtHeader: map[string]struct{}{"X-Forwarded-Host": {}}, + trustedOrigins: map[string]struct{}{"grafana.com": {}}, + expectedOK: true, + }, + { + name: "safe endpoint", + request: postRequest(t, "example.org/foo/bar", map[string]string{"Origin": "null"}), + safeEndpoints: map[string]struct{}{"foo/bar": {}}, + expectedOK: true, + }, + } + + for _, tc := range tests { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + c := csrf.CSRF{ + Cfg: setting.NewCfg(), + TrustedOrigins: tc.trustedOrigins, + Headers: tc.addtHeader, + SafeEndpoints: tc.safeEndpoints, + } + c.Cfg.LoginCookieName = "LoginCookie" + + err := c.Check(tc.request) + if tc.expectedOK { + require.NoError(t, err) + } else { + require.Error(t, err) + var actual *csrf.ErrorWithStatus + require.True(t, errors.As(err, &actual)) + assert.EqualValues(t, tc.expectedStatus, actual.HTTPStatus) + } + }) + } +} + +func postRequest(t testing.TB, hostname string, headers map[string]string) *http.Request { + t.Helper() + urlParts := strings.SplitN(hostname, "/", 2) + + path := "/" + if len(urlParts) == 2 { + path = urlParts[1] + } + r, err := http.NewRequest(http.MethodPost, path, nil) + require.NoError(t, err) + + r.Host = urlParts[0] + + r.AddCookie(&http.Cookie{ + Name: "LoginCookie", + Value: "this should not be important", + }) + + for k, v := range headers { + r.Header.Set(k, v) + } + return r +} + func csrfScenario(t *testing.T, cookieName, method, origin, host string) *httptest.ResponseRecorder { req, err := http.NewRequest(method, "/", nil) if err != nil { @@ -118,7 +234,10 @@ func csrfScenario(t *testing.T, cookieName, method, origin, host string) *httpte }) rr := httptest.NewRecorder() - handler := CSRF(cookieName, log.New())(testHandler) + cfg := setting.NewCfg() + cfg.LoginCookieName = cookieName + service := csrf.ProvideCSRFFilter(cfg) + handler := service.Middleware()(testHandler) handler.ServeHTTP(rr, req) return rr } diff --git a/pkg/server/wire.go b/pkg/server/wire.go index f3b2db696bd..5b4be475a4f 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -28,6 +28,7 @@ import ( "github.com/grafana/grafana/pkg/infra/usagestats/statscollector" loginpkg "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/middleware/csrf" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/backendplugin/coreplugin" @@ -260,6 +261,7 @@ var wireBasicSet = wire.NewSet( ossaccesscontrol.ProvideDashboardPermissions, wire.Bind(new(accesscontrol.DashboardPermissionsService), new(*ossaccesscontrol.DashboardPermissionsService)), starimpl.ProvideService, + csrf.ProvideCSRFFilter, ) var wireSet = wire.NewSet(