From 5a606b82e6698fb1b56c82a3b6cdb552d325d065 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 17 Dec 2021 18:15:47 +0100 Subject: [PATCH 1/8] contrib/labstack/echo.v4: add support for http.request.path_params address Add an "optional" parameter to httpsec.MakeHandlerOperationArgs to keep track of the name/values map for path params since the data is not retrievable through http.Request. Also edit waf_test.go to add a small test to check e2e soundness. --- contrib/labstack/echo.v4/echotrace.go | 8 +++++++- internal/appsec/dyngo/instrumentation/httpsec/http.go | 9 ++++++--- internal/appsec/waf.go | 4 ++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index 6435adf0da..43803a0e04 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -75,9 +75,15 @@ func withAppSec(next echo.HandlerFunc) echo.HandlerFunc { if !ok { return next(c) } + httpsec.SetAppSecTags(span) - args := httpsec.MakeHandlerOperationArgs(req) + 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 { diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index e5675ef521..79a0b1a2fb 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. @@ -47,7 +49,7 @@ type ( func WrapHandler(handler http.Handler, span ddtrace.Span) http.Handler { SetAppSecTags(span) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - args := MakeHandlerOperationArgs(r) + args := MakeHandlerOperationArgs(r, nil) 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, params 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: params, } } diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 7bb8ef4280..ea5d50d468 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -91,6 +91,8 @@ func newWAFEventListener(handle *waf.Handle, addresses []string, timeout time.Du values[serverRequestQueryAddr] = args.Query case serverResponseStatusAddr: values[serverResponseStatusAddr] = res.Status + case serverRequestPathParams: + values[serverRequestPathParams] = args.PathParams } } matches := runWAF(wafCtx, values, timeout) @@ -126,6 +128,7 @@ const ( serverRequestCookiesAddr = "server.request.cookies" serverRequestQueryAddr = "server.request.query" serverResponseStatusAddr = "server.response.status" + serverRequestPathParams = "server.request.path_params" ) // List of rule addresses currently supported by the WAF @@ -135,6 +138,7 @@ var supportedAddressesList = []string{ serverRequestCookiesAddr, serverRequestQueryAddr, serverResponseStatusAddr, + serverRequestPathParams, } func init() { From fd5a49e21e0056a77828bb3b895fb7b8fb80b771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 17 Dec 2021 18:15:47 +0100 Subject: [PATCH 2/8] contrib/gorilla/mux: add support for http.request.path_params address Add an AppSecParam field to router trace config to pass parameters to AppSec wrap handler that can't be retrieved from request only. This allows in this case to pass down path parameters. Also add waf_test.go to check e2e soundness. --- contrib/gorilla/mux/mux.go | 4 ++ contrib/gorilla/mux/waf_test.go | 67 +++++++++++++++++++ contrib/internal/httputil/trace.go | 3 +- .../dyngo/instrumentation/httpsec/http.go | 11 ++- 4 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 contrib/gorilla/mux/waf_test.go diff --git a/contrib/gorilla/mux/mux.go b/contrib/gorilla/mux/mux.go index 86181225ba..7b060e833d 100644 --- a/contrib/gorilla/mux/mux.go +++ b/contrib/gorilla/mux/mux.go @@ -15,6 +15,7 @@ import ( "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/dyngo/instrumentation/httpsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" "github.com/gorilla/mux" @@ -124,6 +125,9 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) { FinishOpts: r.config.finishOpts, SpanOpts: spanopts, QueryParams: r.config.queryParams, + AppSecParams: httpsec.AppSecParams{ + PathParams: match.Vars, + }, }) } diff --git a/contrib/gorilla/mux/waf_test.go b/contrib/gorilla/mux/waf_test.go new file mode 100644 index 0000000000..33b516f2f8 --- /dev/null +++ b/contrib/gorilla/mux/waf_test.go @@ -0,0 +1,67 @@ +// 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. + +//go:build appsec +// +build appsec + +package mux_test + +import ( + "io/ioutil" + "net/http" + "net/http/httptest" + "strings" + "testing" + + muxtrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/gorilla/mux" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" + + "github.com/stretchr/testify/require" +) + +func TestAppSec(t *testing.T) { + t.Run("path-params", func(t *testing.T) { + // Start the tracer along with the fake agent HTTP server + mt := mocktracer.Start() + defer mt.Stop() + + appsec.Start() + defer appsec.Stop() + + if !appsec.Enabled() { + t.Skip("appsec disabled") + } + + // Start and trace an HTTP server + router := muxtrace.NewRouter() + router.Handle("/{pathParam}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("OK!\n")) + })) + + srv := httptest.NewServer(router) + defer srv.Close() + + // Forge HTTP request with path parameter + req, err := http.NewRequest("POST", srv.URL+"/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, "OK!\n", string(b)) + + spans := mt.FinishedSpans() + // The request should have the attack attempt event (appsec rule id crs-913-120). + event := spans[0].Tag("_dd.appsec.json") + require.NotNil(t, event) + require.True(t, strings.Contains(event.(string), "crs-913-120")) + }) +} diff --git a/contrib/internal/httputil/trace.go b/contrib/internal/httputil/trace.go index 52c2c39565..552e749626 100644 --- a/contrib/internal/httputil/trace.go +++ b/contrib/internal/httputil/trace.go @@ -28,6 +28,7 @@ type TraceConfig struct { QueryParams bool // specifies that request query parameters should be appended to http.url tag FinishOpts []ddtrace.FinishOption // span finish options to be applied SpanOpts []ddtrace.StartSpanOption // additional span options to be applied + AppSecParams httpsec.AppSecParams // extra parameters that AppSec may need while wrapping the request handler } // TraceAndServe will apply tracing to the given http.Handler using the passed tracer under the given service and resource. @@ -55,7 +56,7 @@ func TraceAndServe(h http.Handler, cfg *TraceConfig) { defer span.Finish(cfg.FinishOpts...) if appsec.Enabled() { - h = httpsec.WrapHandler(h, span) + h = httpsec.WrapHandler(h, span, cfg.AppSecParams) } h.ServeHTTP(wrapResponseWriter(cfg.ResponseWriter, span), cfg.Request.WithContext(ctx)) } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 79a0b1a2fb..a0ad917309 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -42,14 +42,21 @@ type ( // Status corresponds to the address `server.response.status`. Status int } + + // AppSecParams is the extra parameters that need to be passed over to the AppSec WrapHandler (i.e params + // not retrievable from the http.Request, such as path parameters) + AppSecParams struct { + PathParams map[string]string + } ) // 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, params AppSecParams) http.Handler { SetAppSecTags(span) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - args := MakeHandlerOperationArgs(r, nil) + args := MakeHandlerOperationArgs(r, params.PathParams) op := StartOperation( args, nil, From bc9b9b27e9517f6d0e36f6d92a609e1f4d7b989b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 12 Jan 2022 10:53:51 +0100 Subject: [PATCH 3/8] contrib/gorilla/mux: simplify AppSecParams struct to PathParams string map --- contrib/gorilla/mux/mux.go | 5 +---- contrib/internal/httputil/trace.go | 4 ++-- .../appsec/dyngo/instrumentation/httpsec/http.go | 14 ++++---------- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/contrib/gorilla/mux/mux.go b/contrib/gorilla/mux/mux.go index 7b060e833d..81d3c3b223 100644 --- a/contrib/gorilla/mux/mux.go +++ b/contrib/gorilla/mux/mux.go @@ -15,7 +15,6 @@ import ( "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/dyngo/instrumentation/httpsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" "github.com/gorilla/mux" @@ -125,9 +124,7 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) { FinishOpts: r.config.finishOpts, SpanOpts: spanopts, QueryParams: r.config.queryParams, - AppSecParams: httpsec.AppSecParams{ - PathParams: match.Vars, - }, + PathParams: match.Vars, }) } diff --git a/contrib/internal/httputil/trace.go b/contrib/internal/httputil/trace.go index 552e749626..9d4a6ca9a7 100644 --- a/contrib/internal/httputil/trace.go +++ b/contrib/internal/httputil/trace.go @@ -28,7 +28,7 @@ type TraceConfig struct { QueryParams bool // specifies that request query parameters should be appended to http.url tag FinishOpts []ddtrace.FinishOption // span finish options to be applied SpanOpts []ddtrace.StartSpanOption // additional span options to be applied - AppSecParams httpsec.AppSecParams // extra parameters that AppSec may need while wrapping the request handler + PathParams map[string]string // path parameters needed for AppSec checks } // TraceAndServe will apply tracing to the given http.Handler using the passed tracer under the given service and resource. @@ -56,7 +56,7 @@ func TraceAndServe(h http.Handler, cfg *TraceConfig) { defer span.Finish(cfg.FinishOpts...) if appsec.Enabled() { - h = httpsec.WrapHandler(h, span, cfg.AppSecParams) + h = httpsec.WrapHandler(h, span, cfg.PathParams) } h.ServeHTTP(wrapResponseWriter(cfg.ResponseWriter, span), cfg.Request.WithContext(ctx)) } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index a0ad917309..758460456b 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -42,21 +42,15 @@ type ( // Status corresponds to the address `server.response.status`. Status int } - - // AppSecParams is the extra parameters that need to be passed over to the AppSec WrapHandler (i.e params - // not retrievable from the http.Request, such as path parameters) - AppSecParams struct { - PathParams map[string]string - } ) // WrapHandler wraps the given HTTP handler with the abstract HTTP operation defined by HandlerOperationArgs and // HandlerOperationRes. -func WrapHandler(handler http.Handler, span ddtrace.Span, params AppSecParams) 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, params.PathParams) + args := MakeHandlerOperationArgs(r, pathParams) op := StartOperation( args, nil, @@ -84,7 +78,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, params AppSecParams) h // 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, params map[string]string) 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 { @@ -104,7 +98,7 @@ func MakeHandlerOperationArgs(r *http.Request, params map[string]string) Handler // 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(), - PathParams: params, + PathParams: pathParams, } } From fcd965b4e8d40472ab7c0346432c4ea689df9ed4 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 13:12:33 +0100 Subject: [PATCH 4/8] pr review --- contrib/gorilla/mux/mux_test.go | 82 ++++++++++++++ contrib/gorilla/mux/waf_test.go | 67 ------------ contrib/internal/httputil/trace.go | 2 +- contrib/labstack/echo.v4/appsec.go | 43 ++++++++ contrib/labstack/echo.v4/echotrace.go | 32 ------ contrib/labstack/echo.v4/echotrace_test.go | 103 ++++++++++++++---- .../dyngo/instrumentation/httpsec/http.go | 1 - 7 files changed, 205 insertions(+), 125 deletions(-) delete mode 100644 contrib/gorilla/mux/waf_test.go create mode 100644 contrib/labstack/echo.v4/appsec.go diff --git a/contrib/gorilla/mux/mux_test.go b/contrib/gorilla/mux/mux_test.go index bfc7a03ba8..4e501bb9e8 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.Handle("/path0.0/{myPathParam0}/path0.1/{myPathParam1}/path0.2/{myPathParam2}/path0.3/{myPathParam3}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte("Hello World!\n")) + require.NoError(t, err) + })) + router.Handle("/", http.HandlerFunc(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/gorilla/mux/waf_test.go b/contrib/gorilla/mux/waf_test.go deleted file mode 100644 index 33b516f2f8..0000000000 --- a/contrib/gorilla/mux/waf_test.go +++ /dev/null @@ -1,67 +0,0 @@ -// 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. - -//go:build appsec -// +build appsec - -package mux_test - -import ( - "io/ioutil" - "net/http" - "net/http/httptest" - "strings" - "testing" - - muxtrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/gorilla/mux" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" - "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" - - "github.com/stretchr/testify/require" -) - -func TestAppSec(t *testing.T) { - t.Run("path-params", func(t *testing.T) { - // Start the tracer along with the fake agent HTTP server - mt := mocktracer.Start() - defer mt.Stop() - - appsec.Start() - defer appsec.Stop() - - if !appsec.Enabled() { - t.Skip("appsec disabled") - } - - // Start and trace an HTTP server - router := muxtrace.NewRouter() - router.Handle("/{pathParam}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("OK!\n")) - })) - - srv := httptest.NewServer(router) - defer srv.Close() - - // Forge HTTP request with path parameter - req, err := http.NewRequest("POST", srv.URL+"/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, "OK!\n", string(b)) - - spans := mt.FinishedSpans() - // The request should have the attack attempt event (appsec rule id crs-913-120). - event := spans[0].Tag("_dd.appsec.json") - require.NotNil(t, event) - require.True(t, strings.Contains(event.(string), "crs-913-120")) - }) -} diff --git a/contrib/internal/httputil/trace.go b/contrib/internal/httputil/trace.go index 9d4a6ca9a7..3773d59e24 100644 --- a/contrib/internal/httputil/trace.go +++ b/contrib/internal/httputil/trace.go @@ -26,9 +26,9 @@ type TraceConfig struct { Service string // service name Resource string // resource name QueryParams bool // specifies that request query parameters should be appended to http.url tag + PathParams map[string]string // specifies framework-specific path parameters FinishOpts []ddtrace.FinishOption // span finish options to be applied SpanOpts []ddtrace.StartSpanOption // additional span options to be applied - PathParams map[string]string // path parameters needed for AppSec checks } // TraceAndServe will apply tracing to the given http.Handler using the passed tracer under the given service and resource. diff --git a/contrib/labstack/echo.v4/appsec.go b/contrib/labstack/echo.v4/appsec.go new file mode 100644 index 0000000000..f73b1bfad3 --- /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" + + "github.com/labstack/echo/v4" + + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" +) + +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 387f138f01..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,33 +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) - 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_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/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 3b6e840347..2a0ef1a94d 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -48,7 +48,6 @@ type ( // HandlerOperationRes. 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, pathParams) op := StartOperation( From f73dff91e2a170385be79c886edab829108d5244 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 13:58:03 +0100 Subject: [PATCH 5/8] self pr review --- contrib/labstack/echo.v4/appsec.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/labstack/echo.v4/appsec.go b/contrib/labstack/echo.v4/appsec.go index f73b1bfad3..e67698c332 100644 --- a/contrib/labstack/echo.v4/appsec.go +++ b/contrib/labstack/echo.v4/appsec.go @@ -8,10 +8,10 @@ package echo import ( "net" - "github.com/labstack/echo/v4" - "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 { From 70f1125c99b8fbb08dd1534a938d3caa2f8ee5ca Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 14:31:06 +0100 Subject: [PATCH 6/8] self pr review --- internal/appsec/waf.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 356c6d7b65..38536349c0 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -191,8 +191,8 @@ const ( serverRequestHeadersNoCookiesAddr = "server.request.headers.no_cookies" serverRequestCookiesAddr = "server.request.cookies" serverRequestQueryAddr = "server.request.query" - serverResponseStatusAddr = "server.response.status" serverRequestPathParams = "server.request.path_params" + serverResponseStatusAddr = "server.response.status" ) // List of HTTP rule addresses currently supported by the WAF @@ -201,8 +201,8 @@ var httpAddresses = []string{ serverRequestHeadersNoCookiesAddr, serverRequestCookiesAddr, serverRequestQueryAddr, - serverResponseStatusAddr, serverRequestPathParams, + serverResponseStatusAddr, } // gRPC rule addresses currently supported by the WAF From ae47a85c56d0aa9d7ed660f3d14f30b216a59d03 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 16:28:11 +0100 Subject: [PATCH 7/8] self pr review --- contrib/net/http/trace.go | 4 ++-- internal/appsec/waf.go | 20 ++++++++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/contrib/net/http/trace.go b/contrib/net/http/trace.go index a268e2b4ea..38a7242991 100644 --- a/contrib/net/http/trace.go +++ b/contrib/net/http/trace.go @@ -28,8 +28,8 @@ 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"}) + // 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 monitored by appsec. RouteParams map[string]string // FinishOpts specifies any options to be used when finishing the request span. FinishOpts []ddtrace.FinishOption diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 38536349c0..04bc88f636 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -101,15 +101,23 @@ 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 - case serverRequestPathParams: - values[serverRequestPathParams] = args.PathParams } } matches := runWAF(wafCtx, values, timeout) @@ -127,7 +135,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 From 7ff222dc151448766c0db35587188a05090466bd Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Tue, 18 Jan 2022 22:44:10 +0100 Subject: [PATCH 8/8] contrib/net/http/trace.go: update the RouteParams documentation --- contrib/net/http/trace.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/net/http/trace.go b/contrib/net/http/trace.go index 38a7242991..45b88711f0 100644 --- a/contrib/net/http/trace.go +++ b/contrib/net/http/trace.go @@ -29,7 +29,8 @@ type ServeConfig struct { // 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 monitored by appsec. + // 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