From 898f5d7febb9c0522e031dc6c6924afcbb66bc0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 19 Jan 2022 15:19:47 +0100 Subject: [PATCH] internal/appsec: add support for http.request.path_params address (#1106) --- contrib/gorilla/mux/mux.go | 1 + contrib/gorilla/mux/mux_test.go | 82 ++++++++++++++ contrib/labstack/echo.v4/appsec.go | 43 ++++++++ contrib/labstack/echo.v4/echotrace.go | 26 ----- contrib/labstack/echo.v4/echotrace_test.go | 103 ++++++++++++++---- contrib/net/http/trace.go | 6 +- .../dyngo/instrumentation/httpsec/http.go | 11 +- internal/appsec/waf.go | 20 +++- 8 files changed, 233 insertions(+), 59 deletions(-) create mode 100644 contrib/labstack/echo.v4/appsec.go diff --git a/contrib/gorilla/mux/mux.go b/contrib/gorilla/mux/mux.go index 0006b73aa1..96284b0b82 100644 --- a/contrib/gorilla/mux/mux.go +++ b/contrib/gorilla/mux/mux.go @@ -122,6 +122,7 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) { FinishOpts: r.config.finishOpts, SpanOpts: spanopts, QueryParams: r.config.queryParams, + RouteParams: match.Vars, }) } diff --git a/contrib/gorilla/mux/mux_test.go b/contrib/gorilla/mux/mux_test.go index bfc7a03ba8..08844fedff 100644 --- a/contrib/gorilla/mux/mux_test.go +++ b/contrib/gorilla/mux/mux_test.go @@ -7,17 +7,21 @@ package mux import ( "fmt" + "io/ioutil" "net/http" "net/http/httptest" "strconv" + "strings" "testing" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestHttpTracer(t *testing.T) { @@ -303,3 +307,81 @@ func okHandler() http.Handler { w.Write([]byte("200!\n")) }) } + +func TestAppSec(t *testing.T) { + appsec.Start() + defer appsec.Stop() + + if !appsec.Enabled() { + t.Skip("appsec disabled") + } + + // Start and trace an HTTP server with some testing routes + router := NewRouter() + router.HandleFunc("/path0.0/{myPathParam0}/path0.1/{myPathParam1}/path0.2/{myPathParam2}/path0.3/{myPathParam3}", func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte("Hello World!\n")) + require.NoError(t, err) + }) + router.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte("Hello World!\n")) + require.NoError(t, err) + }) + + srv := httptest.NewServer(router) + defer srv.Close() + + // Test an LFI attack via path parameters + t.Run("request-uri", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + // Send an LFI attack (according to appsec rule id crs-930-100) + req, err := http.NewRequest("POST", srv.URL+"/../../../secret.txt", nil) + if err != nil { + panic(err) + } + res, err := srv.Client().Do(req) + require.NoError(t, err) + // Check that the server behaved as intended (404 after the 301) + require.Equal(t, http.StatusNotFound, res.StatusCode) + // The span should contain the security event + finished := mt.FinishedSpans() + require.Len(t, finished, 2) // 301 + 404 + + // The first 301 redirection should contain the attack via the request uri + event := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "server.request.uri.raw")) + require.True(t, strings.Contains(event, "crs-930-100")) + // The second request should contain the event via the referrer header + event = finished[1].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "server.request.headers.no_cookies")) + require.True(t, strings.Contains(event, "crs-930-100")) + }) + + // Test a security scanner attack via path parameters + t.Run("path-params", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + // Send a security scanner attack (according to appsec rule id crs-913-120) + req, err := http.NewRequest("POST", srv.URL+"/path0.0/param0/path0.1/param1/path0.2/appscan_fingerprint/path0.3/param3", nil) + if err != nil { + panic(err) + } + res, err := srv.Client().Do(req) + require.NoError(t, err) + // Check that the handler was properly called + b, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + require.Equal(t, "Hello World!\n", string(b)) + require.Equal(t, http.StatusOK, res.StatusCode) + // The span should contain the security event + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + event := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "crs-913-120")) + require.True(t, strings.Contains(event, "myPathParam2")) + require.True(t, strings.Contains(event, "server.request.path_params")) + }) +} diff --git a/contrib/labstack/echo.v4/appsec.go b/contrib/labstack/echo.v4/appsec.go new file mode 100644 index 0000000000..e67698c332 --- /dev/null +++ b/contrib/labstack/echo.v4/appsec.go @@ -0,0 +1,43 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016 Datadog, Inc. + +package echo + +import ( + "net" + + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" + + "github.com/labstack/echo/v4" +) + +func withAppSec(next echo.HandlerFunc) echo.HandlerFunc { + return func(c echo.Context) error { + req := c.Request() + span, ok := tracer.SpanFromContext(req.Context()) + if !ok { + return next(c) + } + httpsec.SetAppSecTags(span) + params := make(map[string]string) + for _, n := range c.ParamNames() { + params[n] = c.Param(n) + } + args := httpsec.MakeHandlerOperationArgs(req, params) + op := httpsec.StartOperation(args, nil) + defer func() { + events := op.Finish(httpsec.HandlerOperationRes{Status: c.Response().Status}) + if len(events) > 0 { + remoteIP, _, err := net.SplitHostPort(req.RemoteAddr) + if err != nil { + remoteIP = req.RemoteAddr + } + httpsec.SetSecurityEventTags(span, events, remoteIP, args.Headers, c.Response().Writer.Header()) + } + }() + return next(c) + } +} diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index c04e6e69cc..0b291de7f7 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -8,14 +8,12 @@ package echo import ( "math" - "net" "strconv" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" - "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" "github.com/labstack/echo/v4" ) @@ -71,27 +69,3 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { } } } - -func withAppSec(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { - req := c.Request() - span, ok := tracer.SpanFromContext(req.Context()) - if !ok { - return next(c) - } - httpsec.SetAppSecTags(span) - args := httpsec.MakeHandlerOperationArgs(req) - op := httpsec.StartOperation(args, nil) - defer func() { - events := op.Finish(httpsec.HandlerOperationRes{Status: c.Response().Status}) - if len(events) > 0 { - remoteIP, _, err := net.SplitHostPort(req.RemoteAddr) - if err != nil { - remoteIP = req.RemoteAddr - } - httpsec.SetSecurityEventTags(span, events, remoteIP, args.Headers, c.Response().Writer.Header()) - } - }() - return next(c) - } -} diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index eab88dd5ca..973b976f85 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -272,10 +272,6 @@ func TestNoDebugStack(t *testing.T) { } func TestAppSec(t *testing.T) { - // Start the tracer along with the fake agent HTTP server - mt := mocktracer.Start() - defer mt.Stop() - appsec.Start() defer appsec.Stop() @@ -287,30 +283,89 @@ func TestAppSec(t *testing.T) { e := echo.New() e.Use(Middleware()) - e.POST("/*tmp", func(c echo.Context) error { + // Add some testing routes + e.POST("/path0.0/:myPathParam0/path0.1/:myPathParam1/path0.2/:myPathParam2/path0.3/*myPathParam3", func(c echo.Context) error { + return c.String(200, "Hello World!\n") + }) + e.POST("/", func(c echo.Context) error { return c.String(200, "Hello World!\n") }) srv := httptest.NewServer(e) defer srv.Close() - // Send an LFI attack - req, err := http.NewRequest("POST", srv.URL+"/../../../secret.txt", nil) - if err != nil { - panic(err) - } - res, err := srv.Client().Do(req) - require.NoError(t, err) - - // Check that the handler was properly called - b, err := ioutil.ReadAll(res.Body) - require.NoError(t, err) - require.Equal(t, "Hello World!\n", string(b)) - - finished := mt.FinishedSpans() - require.Len(t, finished, 1) + // Test an LFI attack via path parameters + t.Run("request-uri", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + // Send an LFI attack (according to appsec rule id crs-930-100) + req, err := http.NewRequest("POST", srv.URL+"/../../../secret.txt", nil) + if err != nil { + panic(err) + } + res, err := srv.Client().Do(req) + require.NoError(t, err) + // Check that the server behaved as intended (no 301 but 404 directly) + require.Equal(t, http.StatusNotFound, res.StatusCode) + // The span should contain the security event + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + event := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "crs-930-100")) + require.True(t, strings.Contains(event, "server.request.uri.raw")) + }) - // The request should have the LFI attack attempt event (appsec rule id crs-930-100). - event := finished[0].Tag("_dd.appsec.json") - require.NotNil(t, event) - require.True(t, strings.Contains(event.(string), "crs-930-100")) + // Test a security scanner attack via path parameters + t.Run("path-params", func(t *testing.T) { + t.Run("regular", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + // Send a security scanner attack (according to appsec rule id crs-913-120) + req, err := http.NewRequest("POST", srv.URL+"/path0.0/param0/path0.1/param1/path0.2/appscan_fingerprint/path0.3/param3", nil) + if err != nil { + panic(err) + } + res, err := srv.Client().Do(req) + require.NoError(t, err) + // Check that the handler was properly called + b, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + require.Equal(t, "Hello World!\n", string(b)) + require.Equal(t, http.StatusOK, res.StatusCode) + // The span should contain the security event + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + event := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "crs-913-120")) + require.True(t, strings.Contains(event, "myPathParam2")) + require.True(t, strings.Contains(event, "server.request.path_params")) + }) + + t.Run("wildcard", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + // Send a security scanner attack (according to appsec rule id crs-913-120) + req, err := http.NewRequest("POST", srv.URL+"/path0.0/param0/path0.1/param1/path0.2/param2/path0.3/appscan_fingerprint", nil) + if err != nil { + panic(err) + } + res, err := srv.Client().Do(req) + require.NoError(t, err) + // Check that the handler was properly called + b, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + require.Equal(t, "Hello World!\n", string(b)) + require.Equal(t, http.StatusOK, res.StatusCode) + // The span should contain the security event + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + event := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "crs-913-120")) + // Wildcards are not named in echo + require.False(t, strings.Contains(event, "myPathParam3")) + require.True(t, strings.Contains(event, "server.request.path_params")) + }) + }) } diff --git a/contrib/net/http/trace.go b/contrib/net/http/trace.go index 7e5deb1894..45b88711f0 100644 --- a/contrib/net/http/trace.go +++ b/contrib/net/http/trace.go @@ -28,6 +28,10 @@ type ServeConfig struct { Resource string // QueryParams specifies any query parameters that be appended to the resulting "http.url" tag. QueryParams bool + // RouteParams specifies framework-specific route parameters (e.g. for route /user/:id coming + // in as /user/123 we'll have {"id": "123"}). This field is optional and is used for monitoring + // by AppSec. It is only taken into account when AppSec is enabled. + RouteParams map[string]string // FinishOpts specifies any options to be used when finishing the request span. FinishOpts []ddtrace.FinishOption // SpanOpts specifies any options to be applied to the request starting span. @@ -63,7 +67,7 @@ func TraceAndServe(h http.Handler, w http.ResponseWriter, r *http.Request, cfg * defer span.Finish(cfg.FinishOpts...) if appsec.Enabled() { - h = httpsec.WrapHandler(h, span) + h = httpsec.WrapHandler(h, span, cfg.RouteParams) } h.ServeHTTP(wrapResponseWriter(w, span), r.WithContext(ctx)) } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index e3351bb9f6..2a0ef1a94d 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -33,6 +33,8 @@ type ( Cookies []string // Query corresponds to the address `server.request.query` Query map[string][]string + // PathParams corresponds to the address `server.request.path_params` + PathParams map[string]string } // HandlerOperationRes is the HTTP handler operation results. @@ -44,10 +46,10 @@ type ( // WrapHandler wraps the given HTTP handler with the abstract HTTP operation defined by HandlerOperationArgs and // HandlerOperationRes. -func WrapHandler(handler http.Handler, span ddtrace.Span) http.Handler { +func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string) http.Handler { SetAppSecTags(span) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - args := MakeHandlerOperationArgs(r) + args := MakeHandlerOperationArgs(r, pathParams) op := StartOperation( args, nil, @@ -75,7 +77,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span) http.Handler { // MakeHandlerOperationArgs creates the HandlerOperationArgs out of a standard // http.Request along with the given current span. It returns an empty structure // when appsec is disabled. -func MakeHandlerOperationArgs(r *http.Request) HandlerOperationArgs { +func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) HandlerOperationArgs { headers := make(http.Header, len(r.Header)) var cookies []string for k, v := range r.Header { @@ -94,7 +96,8 @@ func MakeHandlerOperationArgs(r *http.Request) HandlerOperationArgs { Cookies: cookies, // TODO(Julio-Guerra): avoid actively parsing the query string and move to a lazy monitoring of this value with // the dynamic instrumentation of the Query() method. - Query: r.URL.Query(), + Query: r.URL.Query(), + PathParams: pathParams, } } diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 9dc3c6a996..3f218a27c9 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -98,11 +98,21 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim case serverRequestRawURIAddr: values[serverRequestRawURIAddr] = args.RequestURI case serverRequestHeadersNoCookiesAddr: - values[serverRequestHeadersNoCookiesAddr] = args.Headers + if headers := args.Headers; headers != nil { + values[serverRequestHeadersNoCookiesAddr] = headers + } case serverRequestCookiesAddr: - values[serverRequestCookiesAddr] = args.Cookies + if cookies := args.Cookies; cookies != nil { + values[serverRequestCookiesAddr] = cookies + } case serverRequestQueryAddr: - values[serverRequestQueryAddr] = args.Query + if query := args.Query; query != nil { + values[serverRequestQueryAddr] = query + } + case serverRequestPathParams: + if pathParams := args.PathParams; pathParams != nil { + values[serverRequestPathParams] = pathParams + } case serverResponseStatusAddr: values[serverResponseStatusAddr] = res.Status } @@ -122,7 +132,7 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim // newGRPCWAFEventListener returns the WAF event listener to register in order // to enable it. -func newGRPCWAFEventListener(handle *waf.Handle, addresses []string, timeout time.Duration) dyngo.EventListener { +func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Duration) dyngo.EventListener { return grpcsec.OnHandlerOperationStart(func(op *grpcsec.HandlerOperation, _ grpcsec.HandlerOperationArgs) { // Limit the maximum number of security events, as a streaming RPC could // receive unlimited number of messages where we could find security events @@ -186,6 +196,7 @@ const ( serverRequestHeadersNoCookiesAddr = "server.request.headers.no_cookies" serverRequestCookiesAddr = "server.request.cookies" serverRequestQueryAddr = "server.request.query" + serverRequestPathParams = "server.request.path_params" serverResponseStatusAddr = "server.response.status" ) @@ -195,6 +206,7 @@ var httpAddresses = []string{ serverRequestHeadersNoCookiesAddr, serverRequestCookiesAddr, serverRequestQueryAddr, + serverRequestPathParams, serverResponseStatusAddr, }