Chore: sanitize values before being logged from request headers (#49245)

* Chore: sanitize values being logged directly from request headers

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()
+}

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)
+		})
+	}
+}

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
 }
 

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)
+			}
+		})
+	}
+}

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)