internal/appsec: add support for http.request.path_params address (#1106

)

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

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

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

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

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

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

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,
 	}
 }