From dfab100dc7ca40541f2c43db85c480fe62fac84f Mon Sep 17 00:00:00 2001 From: Ezequiel Victorero Date: Mon, 23 May 2022 14:18:33 -0300 Subject: [PATCH] Chore: sanitize values before being logged from request headers (#49245) * Chore: sanitize values being logged directly from request headers --- pkg/middleware/logger.go | 17 +++++- pkg/middleware/logger_test.go | 57 ++++++++++++++++++++ pkg/web/context.go | 14 +++++ pkg/web/context_test.go | 97 +++++++++++++++++++++++++++++++++++ pkg/web/macaron.go | 3 ++ 5 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 pkg/middleware/logger_test.go create mode 100644 pkg/web/context_test.go diff --git a/pkg/middleware/logger.go b/pkg/middleware/logger.go index c2f127319cb..6e5acd0ea40 100644 --- a/pkg/middleware/logger.go +++ b/pkg/middleware/logger.go @@ -17,9 +17,11 @@ package middleware import ( "net/http" + "net/url" "time" "github.com/grafana/grafana/pkg/infra/tracing" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" @@ -55,7 +57,7 @@ func Logger(cfg *setting.Cfg) web.Handler { "time_ms", int64(timeTaken), "duration", duration, "size", rw.Size(), - "referer", req.Referer(), + "referer", sanitizeURL(ctx, req.Referer()), } traceID := tracing.TraceIDFromContext(ctx.Req.Context(), false) @@ -71,3 +73,16 @@ func Logger(cfg *setting.Cfg) web.Handler { } } } + +func sanitizeURL(ctx *models.ReqContext, s string) string { + if s == "" { + return s + } + + u, err := url.ParseRequestURI(s) + if err != nil { + ctx.Logger.Warn("Received invalid referer in request headers, removed for log forgery prevention") + return "" + } + return u.String() +} diff --git a/pkg/middleware/logger_test.go b/pkg/middleware/logger_test.go new file mode 100644 index 00000000000..e1e6459a0ce --- /dev/null +++ b/pkg/middleware/logger_test.go @@ -0,0 +1,57 @@ +package middleware + +import ( + "testing" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" + "github.com/stretchr/testify/assert" +) + +func Test_sanitizeURL(t *testing.T) { + type args struct { + ctx *models.ReqContext + s string + } + tests := []struct { + name string + args args + want string + }{ + { + name: "Receiving empty string should return it", + args: args{ + ctx: &models.ReqContext{ + Logger: log.New("test.logger"), + }, + s: "", + }, + want: "", + }, + { + name: "Receiving valid URL string should return it parsed", + args: args{ + ctx: &models.ReqContext{ + Logger: log.New("test.logger"), + }, + s: "https://grafana.com/", + }, + want: "https://grafana.com/", + }, + { + name: "Receiving invalid URL string should return empty string", + args: args{ + ctx: &models.ReqContext{ + Logger: log.New("test.logger"), + }, + s: "this is not a valid URL", + }, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, sanitizeURL(tt.args.ctx, tt.args.s), "sanitizeURL(%v, %v)", tt.args.ctx, tt.args.s) + }) + } +} diff --git a/pkg/web/context.go b/pkg/web/context.go index 1fd32340bc8..8cea4cec9cb 100644 --- a/pkg/web/context.go +++ b/pkg/web/context.go @@ -17,11 +17,14 @@ package web import ( "encoding/json" "html/template" + "net" "net/http" "net/url" "reflect" "strconv" "strings" + + "github.com/grafana/grafana/pkg/infra/log" ) // ContextInvoker is an inject.FastInvoker wrapper of func(ctx *Context). @@ -44,6 +47,7 @@ type Context struct { Req *http.Request Resp ResponseWriter template *template.Template + logger log.Logger } func (ctx *Context) handler() Handler { @@ -84,12 +88,22 @@ func (ctx *Context) RemoteAddr() string { addr = strings.TrimSpace(strings.Split(ctx.Req.Header.Get("X-Forwarded-For"), ",")[0]) } + // parse user inputs from headers to prevent log forgery + if len(addr) > 0 { + if parsedIP := net.ParseIP(addr); parsedIP == nil { + // if parsedIP is nil we clean addr and populate with RemoteAddr below + ctx.logger.Warn("Received invalid IP address in request headers, removed for log forgery prevention") + addr = "" + } + } + if len(addr) == 0 { addr = ctx.Req.RemoteAddr if i := strings.LastIndex(addr, ":"); i > -1 { addr = addr[:i] } } + return addr } diff --git a/pkg/web/context_test.go b/pkg/web/context_test.go new file mode 100644 index 00000000000..5e013c9a5a5 --- /dev/null +++ b/pkg/web/context_test.go @@ -0,0 +1,97 @@ +package web + +import ( + "net/http" + "testing" + + "github.com/grafana/grafana/pkg/infra/log" +) + +func TestContext_RemoteAddr(t *testing.T) { + type fields struct { + Req *http.Request + logger log.Logger + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "Receive invalid ip address in headers should return RemoteAddr", + fields: fields{ + logger: log.New("test.logger"), + Req: &http.Request{ + RemoteAddr: "255.255.255.255", + Header: http.Header{ + "X-Real-Ip": []string{"this is not a valid IP"}, + "X-Forwarded-For": []string{"192.168.1.1"}, + }, + }, + }, + want: "255.255.255.255", + }, + { + name: "Receive valid ip address in X-Real-Ip should return it", + fields: fields{ + logger: log.New("test.logger"), + Req: &http.Request{ + RemoteAddr: "255.255.255.255", + Header: http.Header{ + "X-Real-Ip": []string{"192.168.1.1"}, + "X-Forwarded-For": []string{"this is not a valid IP"}, + }, + }, + }, + want: "192.168.1.1", + }, + { + name: "Receive valid ip addresses in X-Forwarded-For should return the first one", + fields: fields{ + logger: log.New("test.logger"), + Req: &http.Request{ + RemoteAddr: "255.255.255.255", + Header: http.Header{ + "X-Forwarded-For": []string{"192.168.1.1,255.255.255.255"}, + }, + }, + }, + want: "192.168.1.1", + }, + { + name: "Receive valid ip addresses IPV6 in X-Forwarded-For should return it", + fields: fields{ + logger: log.New("test.logger"), + Req: &http.Request{ + RemoteAddr: "255.255.255.255", + Header: http.Header{ + "X-Forwarded-For": []string{"2001:db8:85a3:8d3:1319:8a2e:370:7348"}, + }, + }, + }, + want: "2001:db8:85a3:8d3:1319:8a2e:370:7348", + }, + { + name: "When no header is informed, should return remote_addr without port", + fields: fields{ + logger: log.New("test.logger"), + Req: &http.Request{ + RemoteAddr: "[::1]:51299", + Header: http.Header{}, + }, + }, + want: "[::1]", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := &Context{ + Req: tt.fields.Req, + logger: tt.fields.logger, + } + if got := ctx.RemoteAddr(); got != tt.want { + t.Errorf("RemoteAddr() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/web/macaron.go b/pkg/web/macaron.go index 071ea67e9c4..e6d4a90a7d8 100644 --- a/pkg/web/macaron.go +++ b/pkg/web/macaron.go @@ -24,6 +24,8 @@ import ( "context" "net/http" "strings" + + "github.com/grafana/grafana/pkg/infra/log" ) const _VERSION = "1.3.4.0805" @@ -156,6 +158,7 @@ func (m *Macaron) createContext(rw http.ResponseWriter, req *http.Request) *Cont index: 0, Router: m.Router, Resp: NewResponseWriter(req.Method, rw), + logger: log.New("macaron.context"), } req = req.WithContext(context.WithValue(req.Context(), macaronContextKey{}, c)) c.Map(c)