From da02d30c5a12b94a1dc81410f06a51e1e9cd15a6 Mon Sep 17 00:00:00 2001 From: Eliott Bouhana <47679741+eliottness@users.noreply.github.com> Date: Fri, 16 Aug 2024 15:44:43 +0200 Subject: [PATCH] appsec: setup ossec package and OpenOperation (#2781) Signed-off-by: Eliott Bouhana --- .github/workflows/orchestrion.yml | 23 +++++ internal/appsec/dyngo/operation.go | 17 ++-- internal/appsec/emitter/ossec/lfi.go | 41 ++++++++ internal/appsec/listener/grpcsec/grpc.go | 10 +- internal/appsec/listener/httpsec/http.go | 8 +- .../appsec/listener/httpsec/roundtripper.go | 6 ++ internal/appsec/listener/ossec/lfi.go | 46 +++++++++ internal/appsec/waf_test.go | 98 +++++++++++++++++++ internal/log/log.go | 6 ++ internal/orchestrion/context_stack.go | 8 +- internal/stacktrace/stacktrace.go | 12 ++- 11 files changed, 259 insertions(+), 16 deletions(-) create mode 100644 .github/workflows/orchestrion.yml create mode 100644 internal/appsec/emitter/ossec/lfi.go create mode 100644 internal/appsec/listener/ossec/lfi.go diff --git a/.github/workflows/orchestrion.yml b/.github/workflows/orchestrion.yml new file mode 100644 index 0000000000..0603c8b139 --- /dev/null +++ b/.github/workflows/orchestrion.yml @@ -0,0 +1,23 @@ +name: Orchestrion +on: + workflow_dispatch: # manually + pull_request: + merge_group: + push: + branches: + - release-v* + +permissions: read-all + +concurrency: + # Automatically cancel previous runs if a new one is triggered to conserve resources. + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + test: + name: 'Run Tests' + uses: DataDog/orchestrion/.github/workflows/workflow_call.yml@main # we don't want to pin our own action + with: + dd-trace-go-ref: ${{ github.sha }} + runs-on: ubuntu-latest-16-cores diff --git a/internal/appsec/dyngo/operation.go b/internal/appsec/dyngo/operation.go index 884a36ccaf..b5c0a16831 100644 --- a/internal/appsec/dyngo/operation.go +++ b/internal/appsec/dyngo/operation.go @@ -22,13 +22,18 @@ package dyngo import ( "context" - "gopkg.in/DataDog/dd-trace-go.v1/internal/orchestrion" "sync" + "sync/atomic" - "go.uber.org/atomic" - "gopkg.in/DataDog/dd-trace-go.v1/internal/log" + "gopkg.in/DataDog/dd-trace-go.v1/internal/orchestrion" ) +// LogError is the function used to log errors in the dyngo package. +// This is required because we really want to be able to log errors from dyngo +// but the log package depend on too much packages that we want to instrument. +// So we need to do this to avoid dependency cycles. +var LogError = func(string, ...any) {} + // Operation interface type allowing to register event listeners to the // operation. The event listeners will be automatically removed from the // operation once it finishes so that it no longer can be called on finished @@ -179,7 +184,7 @@ func StartAndRegisterOperation[O Operation, E ArgOf[O]](ctx context.Context, op // should call this function to ensure the operation is properly linked in the context tree. func RegisterOperation(ctx context.Context, op Operation) context.Context { op.unwrap().inContext = true - return context.WithValue(ctx, contextKey{}, op) + return orchestrion.CtxWithValue(ctx, contextKey{}, op) } // FinishOperation finishes the operation along with its results and emits a @@ -317,7 +322,7 @@ func (b *dataBroadcaster) clear() { func emitData[T any](b *dataBroadcaster, v T) { defer func() { if r := recover(); r != nil { - log.Error("appsec: recovered from an unexpected panic from an event listener: %+v", r) + LogError("appsec: recovered from an unexpected panic from an event listener: %+v", r) } }() b.mu.RLock() @@ -348,7 +353,7 @@ func (r *eventRegister) clear() { func emitEvent[O Operation, T any](r *eventRegister, op O, v T) { defer func() { if r := recover(); r != nil { - log.Error("appsec: recovered from an unexpected panic from an event listener: %+v", r) + LogError("appsec: recovered from an unexpected panic from an event listener: %+v", r) } }() r.mu.RLock() diff --git a/internal/appsec/emitter/ossec/lfi.go b/internal/appsec/emitter/ossec/lfi.go new file mode 100644 index 0000000000..b769d4c40b --- /dev/null +++ b/internal/appsec/emitter/ossec/lfi.go @@ -0,0 +1,41 @@ +// 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 2024 Datadog, Inc. + +package ossec + +import ( + "io/fs" + + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" +) + +type ( + // OpenOperation type embodies any kind of function calls that will result in a call to an open(2) syscall + OpenOperation struct { + dyngo.Operation + blockErr error + } + + // OpenOperationArgs is the arguments for an open operation + OpenOperationArgs struct { + // Path is the path to the file to be opened + Path string + // Flags are the flags passed to the open(2) syscall + Flags int + // Perms are the permissions passed to the open(2) syscall if the creation of a file is required + Perms fs.FileMode + } + + // OpenOperationRes is the result of an open operation + OpenOperationRes[File any] struct { + // File is the file descriptor returned by the open(2) syscall + File *File + // Err is the error returned by the function + Err *error + } +) + +func (OpenOperationArgs) IsArgOf(*OpenOperation) {} +func (OpenOperationRes[File]) IsResultOf(*OpenOperation) {} diff --git a/internal/appsec/listener/grpcsec/grpc.go b/internal/appsec/listener/grpcsec/grpc.go index 279bdb870b..861021e86f 100644 --- a/internal/appsec/listener/grpcsec/grpc.go +++ b/internal/appsec/listener/grpcsec/grpc.go @@ -17,6 +17,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/sharedsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/httpsec" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/ossec" shared "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sharedsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sqlsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -41,6 +42,9 @@ var supportedAddresses = listener.AddressSet{ httpsec.HTTPClientIPAddr: {}, httpsec.UserIDAddr: {}, httpsec.ServerIoNetURLAddr: {}, + ossec.ServerIOFSFileAddr: {}, + sqlsec.ServerDBStatementAddr: {}, + sqlsec.ServerDBTypeAddr: {}, } // Install registers the gRPC WAF Event Listener on the given root operation. @@ -113,10 +117,14 @@ func (l *wafEventListener) onEvent(op *types.HandlerOperation, handlerArgs types return } - if l.isSecAddressListened(httpsec.ServerIoNetURLAddr) { + if httpsec.SSRFAddressesPresent(l.addresses) { httpsec.RegisterRoundTripperListener(op, &op.SecurityEventsHolder, wafCtx, l.limiter) } + if ossec.OSAddressesPresent(l.addresses) { + ossec.RegisterOpenListener(op, &op.SecurityEventsHolder, wafCtx, l.limiter) + } + if sqlsec.SQLAddressesPresent(l.addresses) { sqlsec.RegisterSQLListener(op, &op.SecurityEventsHolder, wafCtx, l.limiter) } diff --git a/internal/appsec/listener/httpsec/http.go b/internal/appsec/listener/httpsec/http.go index ec63e90e93..c458931336 100644 --- a/internal/appsec/listener/httpsec/http.go +++ b/internal/appsec/listener/httpsec/http.go @@ -16,6 +16,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/httpsec/types" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/sharedsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/ossec" shared "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sharedsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sqlsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -55,6 +56,7 @@ var supportedAddresses = listener.AddressSet{ HTTPClientIPAddr: {}, UserIDAddr: {}, ServerIoNetURLAddr: {}, + ossec.ServerIOFSFileAddr: {}, sqlsec.ServerDBStatementAddr: {}, sqlsec.ServerDBTypeAddr: {}, } @@ -111,12 +113,16 @@ func (l *wafEventListener) onEvent(op *types.Operation, args types.HandlerOperat return } - if _, ok := l.addresses[ServerIoNetURLAddr]; ok { + if SSRFAddressesPresent(l.addresses) { dyngo.On(op, shared.MakeWAFRunListener(&op.SecurityEventsHolder, wafCtx, l.limiter, func(args types.RoundTripOperationArgs) waf.RunAddressData { return waf.RunAddressData{Ephemeral: map[string]any{ServerIoNetURLAddr: args.URL}} })) } + if ossec.OSAddressesPresent(l.addresses) { + ossec.RegisterOpenListener(op, &op.SecurityEventsHolder, wafCtx, l.limiter) + } + if sqlsec.SQLAddressesPresent(l.addresses) { sqlsec.RegisterSQLListener(op, &op.SecurityEventsHolder, wafCtx, l.limiter) } diff --git a/internal/appsec/listener/httpsec/roundtripper.go b/internal/appsec/listener/httpsec/roundtripper.go index 0d30e952f5..0d5102466a 100644 --- a/internal/appsec/listener/httpsec/roundtripper.go +++ b/internal/appsec/listener/httpsec/roundtripper.go @@ -8,6 +8,7 @@ package httpsec import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/httpsec/types" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sharedsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/trace" @@ -21,3 +22,8 @@ func RegisterRoundTripperListener(op dyngo.Operation, events *trace.SecurityEven return waf.RunAddressData{Ephemeral: map[string]any{ServerIoNetURLAddr: args.URL}} })) } + +func SSRFAddressesPresent(addresses listener.AddressSet) bool { + _, urlAddr := addresses[ServerIoNetURLAddr] + return urlAddr +} diff --git a/internal/appsec/listener/ossec/lfi.go b/internal/appsec/listener/ossec/lfi.go new file mode 100644 index 0000000000..12b32e1bf8 --- /dev/null +++ b/internal/appsec/listener/ossec/lfi.go @@ -0,0 +1,46 @@ +// 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 2024 Datadog, Inc. + +package ossec + +import ( + "os" + + "github.com/DataDog/appsec-internal-go/limiter" + waf "github.com/DataDog/go-libddwaf/v3" + + "gopkg.in/DataDog/dd-trace-go.v1/appsec/events" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sharedsec" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/trace" +) + +const ( + ServerIOFSFileAddr = "server.io.fs.file" +) + +func RegisterOpenListener(op dyngo.Operation, eventsHolder *trace.SecurityEventsHolder, wafCtx *waf.Context, limiter limiter.Limiter) { + runWAF := sharedsec.MakeWAFRunListener(eventsHolder, wafCtx, limiter, func(args ossec.OpenOperationArgs) waf.RunAddressData { + return waf.RunAddressData{Ephemeral: map[string]any{ServerIOFSFileAddr: args.Path}} + }) + + dyngo.On(op, func(op *ossec.OpenOperation, args ossec.OpenOperationArgs) { + dyngo.OnData(op, func(e *events.BlockingSecurityEvent) { + dyngo.OnFinish(op, func(_ *ossec.OpenOperation, res ossec.OpenOperationRes[*os.File]) { + if res.Err != nil { + *res.Err = e + } + }) + }) + runWAF(op, args) + }) +} + +func OSAddressesPresent(addresses listener.AddressSet) bool { + _, fileAddr := addresses[ServerIOFSFileAddr] + return fileAddr +} diff --git a/internal/appsec/waf_test.go b/internal/appsec/waf_test.go index 972ec592e0..bc760ee20c 100644 --- a/internal/appsec/waf_test.go +++ b/internal/appsec/waf_test.go @@ -6,21 +6,25 @@ package appsec_test import ( + "context" "database/sql" "encoding/json" "fmt" "io" + "io/fs" "log" "math/rand" "net/http" "net/http/httptest" "net/url" "os" + "strconv" "strings" "testing" internal "github.com/DataDog/appsec-internal-go/appsec" waf "github.com/DataDog/go-libddwaf/v3" + pAppsec "gopkg.in/DataDog/dd-trace-go.v1/appsec" "gopkg.in/DataDog/dd-trace-go.v1/appsec/events" sqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql" @@ -28,6 +32,8 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/config" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/httpsec" _ "github.com/glebarez/go-sqlite" @@ -642,7 +648,99 @@ func TestRASPSQLi(t *testing.T) { }) } } +} + +func TestRASPLFI(t *testing.T) { + t.Setenv("DD_APPSEC_RULES", "testdata/rasp.json") + appsec.Start() + defer appsec.Stop() + + if !appsec.RASPEnabled() { + t.Skip("RASP needs to be enabled for this test") + } + + // Simulate what orchestrion does + WrappedOpen := func(ctx context.Context, path string, flags int) (file *os.File, err error) { + parent, _ := dyngo.FromContext(ctx) + op := &ossec.OpenOperation{ + Operation: dyngo.NewOperation(parent), + } + + dyngo.StartOperation(op, ossec.OpenOperationArgs{ + Path: path, + Flags: flags, + Perms: fs.FileMode(0), + }) + + defer dyngo.FinishOperation(op, ossec.OpenOperationRes[*os.File]{ + File: &file, + Err: &err, + }) + + return + } + + mux := httptrace.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + // Subsequent spans inherit their parent from context. + path := r.URL.Query().Get("path") + block := r.URL.Query().Get("block") + if block == "true" { + _, err := WrappedOpen(r.Context(), path, os.O_RDONLY) + require.ErrorIs(t, err, &events.BlockingSecurityEvent{}) + return + } + _, err := WrappedOpen(r.Context(), "/tmp/test", os.O_RDWR) + require.NoError(t, err) + w.WriteHeader(204) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + for _, tc := range []struct { + name string + path string + block bool + }{ + { + name: "no-error", + path: "", + block: false, + }, + { + name: "passwd", + path: "/etc/passwd", + block: true, + }, + { + name: "shadow", + path: "/etc/shadow", + block: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + req, err := http.NewRequest("GET", srv.URL+"?path="+tc.path+"&block="+strconv.FormatBool(tc.block), nil) + require.NoError(t, err) + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + spans := mt.FinishedSpans() + require.Len(t, spans, 1) + + if tc.block { + require.Equal(t, 403, res.StatusCode) + require.Contains(t, spans[0].Tag("_dd.appsec.json"), "rasp-930-100") + require.Contains(t, spans[0].Tags(), "_dd.stack") + } else { + require.Equal(t, 204, res.StatusCode) + } + }) + } } // BenchmarkSampleWAFContext benchmarks the creation of a WAF context and running the WAF on a request/response pair diff --git a/internal/log/log.go b/internal/log/log.go index c32b1ed9ba..16be1ac7d0 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/version" ) @@ -102,6 +103,11 @@ func init() { if v := os.Getenv("DD_LOGGING_RATE"); v != "" { setLoggingRate(v) } + + // This is required because we really want to be able to log errors from dyngo + // but the log package depend on too much packages that we want to instrument. + // So we need to do this to avoid dependency cycles. + dyngo.LogError = Error } func setLoggingRate(v string) { diff --git a/internal/orchestrion/context_stack.go b/internal/orchestrion/context_stack.go index 6b4e408acc..60dd1edcac 100644 --- a/internal/orchestrion/context_stack.go +++ b/internal/orchestrion/context_stack.go @@ -17,14 +17,14 @@ func getDDContextStack() *contextStack { return gls.(*contextStack) } - newStack := new(contextStack) + newStack := &contextStack{} setDDGLS(newStack) return newStack } // Peek returns the top context from the stack without removing it. func (s *contextStack) Peek(key any) any { - if s == nil { + if s == nil || *s == nil { return nil } @@ -38,7 +38,7 @@ func (s *contextStack) Peek(key any) any { // Push adds a context to the stack. func (s *contextStack) Push(key, val any) { - if s == nil { + if s == nil || *s == nil { return } @@ -47,7 +47,7 @@ func (s *contextStack) Push(key, val any) { // Pop removes the top context from the stack and returns it. func (s *contextStack) Pop(key any) any { - if s == nil { + if s == nil || *s == nil { return nil } diff --git a/internal/stacktrace/stacktrace.go b/internal/stacktrace/stacktrace.go index 368e41356b..9b9ec8c525 100644 --- a/internal/stacktrace/stacktrace.go +++ b/internal/stacktrace/stacktrace.go @@ -32,7 +32,7 @@ var ( "github.com/DataDog/go-libddwaf", "github.com/DataDog/datadog-agent", "github.com/DataDog/appsec-internal-go", - "github.com/DataDog/orchestrion", + "github.com/datadog/orchestrion", } ) @@ -219,7 +219,7 @@ func (it *framesIterator) Next() (StackFrame, bool) { return StackFrame{}, false } - if it.skipSymbol(frame.Function) { + if it.skipFrame(frame) { continue } @@ -237,9 +237,13 @@ func (it *framesIterator) Next() (StackFrame, bool) { } } -func (it *framesIterator) skipSymbol(symbol string) bool { +func (it *framesIterator) skipFrame(frame runtime.Frame) bool { + if frame.File == "" { // skip orchestrion generated code + return true + } + for _, prefix := range it.skipPrefixes { - if strings.HasPrefix(symbol, prefix) { + if strings.HasPrefix(frame.Function, prefix) { return true } }